Messages in this thread | | | Date | Thu, 27 Mar 2014 14:26:24 +0000 | From | Zoltan Kiss <> | Subject | Re: [PATCH net-next] xen-netback: Grant copy the header instead of map and memcpy |
| |
On 27/03/14 12:54, Ian Campbell wrote: > On Thu, 2014-03-27 at 12:46 +0000, Zoltan Kiss wrote: >> On 27/03/14 11:35, Ian Campbell wrote: >>> On Wed, 2014-03-26 at 21:18 +0000, Zoltan Kiss wrote: >>>> An old inefficiency of the TX path that we are grant mapping the first slot, >>>> and then copy the header part to the linear area. Instead, doing a grant copy >>>> for that header straight on is more reasonable. Especially because there are >>>> ongoing efforts to make Xen avoiding TLB flush after unmap when the page were >>>> not touched in Dom0. In the original way the memcpy ruined that. >>>> The key changes: >>>> - the vif has a tx_copy_ops array again >>>> - xenvif_tx_build_gops sets up the grant copy operations >>>> - we don't have to figure out whether the header and first frag are on the same >>>> grant mapped page or not >>>> >>>> Unrelated, but I've made a small refactoring in xenvif_get_extras as well. >>> >>> Just a few thoughts, not really based on close review of the code. >>> >>> Do you have any measurements for this series or workloads where it is >>> particularly beneficial? >> My manual measurements showed that if I also use the Xen patch which >> avoids TLB flush when the pages were not touched, I can easily get 9 - >> 9.3 Gbps with iperf. But I'll run some more automated tests. > > I suppose these numbers are related because avoiding the memcpy avoids > touching the page? Yes
>>> Or (and I confess this is a skanky hack): we overlay tx_copy_ops and >>> tx_map_ops in a union, and insert one set of ops from the front and the >>> other from the end, taking great care around when and where they meet. >> Unfortunately the hypercalls expect an array of _one_ of them, so we >> can't put the 2 types into an union unless it's guaranteed that they >> have the same size. And they are expected to be > > (was there more of this last sentence?) > > I was thinking > union { > struct gnt_map maps[NR_...]; > struct gnt_copy copy[NR_...]; > } > > Then you fill copy from 0..N and maps from M..NR_ and maintain the > invariant that > (&maps[M] - &map[s0]) > ©[N] > > and pass to the grant ops (©[0], N) and (&maps[M], NR_... - M) > respectively. > > Too tricksy perhaps?
Yeah, this array is 30*256 bytes (7.5 kb) long, per vif, I don't think it worth the fuss.
>>> >>>> + vif->tx_copy_ops[*copy_ops].source.u.ref = txreq.gref; >>>> + vif->tx_copy_ops[*copy_ops].source.domid = vif->domid; >>>> + vif->tx_copy_ops[*copy_ops].source.offset = txreq.offset; >>>> + >>>> + vif->tx_copy_ops[*copy_ops].dest.u.gmfn = virt_to_mfn(skb->data); >>>> + vif->tx_copy_ops[*copy_ops].dest.domid = DOMID_SELF; >>>> + vif->tx_copy_ops[*copy_ops].dest.offset = offset_in_page(skb->data); >>>> + >>>> + vif->tx_copy_ops[*copy_ops].len = data_len; >>> >>> Do we want to copy the entire first frag even if it is e.g. a complete >>> page? >>> >>> I'm not sure where the tradeoff lies between doing a grant copy of more >>> than necessary and doing a map+memcpy when the map is already required >>> for the page frag anyway. >> I think we should only grant copy the parts which goes to the linear >> part, because we would memcpy it anyway. It's expected that the stack >> won't touch anything else while the packet passes through. data_len is >> the size of the linear buffer here. >>> >>> What about the case where the first frag is less than PKT_PROT_LEN? I >>> think you still do map+memcpy in that case? >> Then data_len will be txreq.size, we allocate the skb for that, and >> later on we call __pskb_pull_tail in xenvif_tx_submit to pull up for >> that (which is essentially map+memcpy). It's not optimal, but it's like >> that since the good old days. I agree it could be optimized, but let's >> change it in a different patch! > > OK. Can you clarify the title and/or the commit log to make it obvious > that we only copy (some portion of) the first frag and that we still map > +memcpy the remainder of PKT_PROT_LEN if the first frag is not large > enough. Ok
| |