Messages in this thread | | | Date | Mon, 27 Mar 2023 12:07:01 +0200 | From | Juergen Gross <> | Subject | Re: [PATCH 1/2] xen/netback: don't do grant copy across page boundary |
| |
On 27.03.23 11:49, Jan Beulich wrote: > On 27.03.2023 10:36, Juergen Gross wrote: >> Fix xenvif_get_requests() not to do grant copy operations across local >> page boundaries. This requires to double the maximum number of copy >> operations per queue, as each copy could now be split into 2. >> >> Make sure that struct xenvif_tx_cb doesn't grow too large. >> >> Cc: stable@vger.kernel.org >> Fixes: ad7f402ae4f4 ("xen/netback: Ensure protocol headers don't fall in the non-linear area") >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> drivers/net/xen-netback/common.h | 2 +- >> drivers/net/xen-netback/netback.c | 25 +++++++++++++++++++++++-- >> 2 files changed, 24 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h >> index 3dbfc8a6924e..1fcbd83f7ff2 100644 >> --- a/drivers/net/xen-netback/common.h >> +++ b/drivers/net/xen-netback/common.h >> @@ -166,7 +166,7 @@ struct xenvif_queue { /* Per-queue data for xenvif */ >> struct pending_tx_info pending_tx_info[MAX_PENDING_REQS]; >> grant_handle_t grant_tx_handle[MAX_PENDING_REQS]; >> >> - struct gnttab_copy tx_copy_ops[MAX_PENDING_REQS]; >> + struct gnttab_copy tx_copy_ops[2 * MAX_PENDING_REQS]; >> struct gnttab_map_grant_ref tx_map_ops[MAX_PENDING_REQS]; >> struct gnttab_unmap_grant_ref tx_unmap_ops[MAX_PENDING_REQS]; >> /* passed to gnttab_[un]map_refs with pages under (un)mapping */ >> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c >> index 1b42676ca141..111c179f161b 100644 >> --- a/drivers/net/xen-netback/netback.c >> +++ b/drivers/net/xen-netback/netback.c >> @@ -334,6 +334,7 @@ static int xenvif_count_requests(struct xenvif_queue *queue, >> struct xenvif_tx_cb { >> u16 copy_pending_idx[XEN_NETBK_LEGACY_SLOTS_MAX + 1]; >> u8 copy_count; >> + u32 split_mask; >> }; >> >> #define XENVIF_TX_CB(skb) ((struct xenvif_tx_cb *)(skb)->cb) >> @@ -361,6 +362,8 @@ static inline struct sk_buff *xenvif_alloc_skb(unsigned int size) >> struct sk_buff *skb = >> alloc_skb(size + NET_SKB_PAD + NET_IP_ALIGN, >> GFP_ATOMIC | __GFP_NOWARN); >> + >> + BUILD_BUG_ON(sizeof(*XENVIF_TX_CB(skb)) > sizeof(skb->cb)); >> if (unlikely(skb == NULL)) >> return NULL; >> >> @@ -396,11 +399,13 @@ static void xenvif_get_requests(struct xenvif_queue *queue, >> nr_slots = shinfo->nr_frags + 1; >> >> copy_count(skb) = 0; >> + XENVIF_TX_CB(skb)->split_mask = 0; >> >> /* Create copy ops for exactly data_len bytes into the skb head. */ >> __skb_put(skb, data_len); >> while (data_len > 0) { >> int amount = data_len > txp->size ? txp->size : data_len; >> + bool split = false; >> >> cop->source.u.ref = txp->gref; >> cop->source.domid = queue->vif->domid; >> @@ -413,6 +418,13 @@ static void xenvif_get_requests(struct xenvif_queue *queue, >> cop->dest.u.gmfn = virt_to_gfn(skb->data + skb_headlen(skb) >> - data_len); >> >> + /* Don't cross local page boundary! */ >> + if (cop->dest.offset + amount > XEN_PAGE_SIZE) { >> + amount = XEN_PAGE_SIZE - cop->dest.offset; >> + XENVIF_TX_CB(skb)->split_mask |= 1U << copy_count(skb); > > Maybe worthwhile to add a BUILD_BUG_ON() somewhere to make sure this > shift won't grow too large a shift count. The number of slots accepted > could conceivably be grown past XEN_NETBK_LEGACY_SLOTS_MAX (i.e. > XEN_NETIF_NR_SLOTS_MIN) at some point.
This is basically impossible due to the size restriction of struct xenvif_tx_cb.
> >> + split = true; >> + } >> + >> cop->len = amount; >> cop->flags = GNTCOPY_source_gref; >> >> @@ -420,7 +432,8 @@ static void xenvif_get_requests(struct xenvif_queue *queue, >> pending_idx = queue->pending_ring[index]; >> callback_param(queue, pending_idx).ctx = NULL; >> copy_pending_idx(skb, copy_count(skb)) = pending_idx; >> - copy_count(skb)++; >> + if (!split) >> + copy_count(skb)++; >> >> cop++; >> data_len -= amount; >> @@ -441,7 +454,8 @@ static void xenvif_get_requests(struct xenvif_queue *queue, >> nr_slots--; >> } else { >> /* The copy op partially covered the tx_request. >> - * The remainder will be mapped. >> + * The remainder will be mapped or copied in the next >> + * iteration. >> */ >> txp->offset += amount; >> txp->size -= amount; >> @@ -539,6 +553,13 @@ static int xenvif_tx_check_gop(struct xenvif_queue *queue, >> pending_idx = copy_pending_idx(skb, i); >> >> newerr = (*gopp_copy)->status; >> + >> + /* Split copies need to be handled together. */ >> + if (XENVIF_TX_CB(skb)->split_mask & (1U << i)) { >> + (*gopp_copy)++; >> + if (!newerr) >> + newerr = (*gopp_copy)->status; >> + } > > It isn't guaranteed that a slot may be split only once, is it? Assuming a
I think it is guaranteed.
No slot can cover more than XEN_PAGE_SIZE bytes due to the grants being restricted to that size. There is no way how such a data packet could cross 2 page boundaries.
In the end the problem isn't the copies for the linear area not crossing multiple page boundaries, but the copies for a single request slot not doing so. And this can't happen IMO.
> near-64k packet with all tiny non-primary slots, that'll cause those tiny > slots to all be mapped, but due to > > if (ret >= XEN_NETBK_LEGACY_SLOTS_MAX - 1 && data_len < txreq.size) > data_len = txreq.size; > > will, afaict, cause a lot of copying for the primary slot. Therefore I > think you need a loop here, not just an if(). Plus tx_copy_ops[]'es > dimension also looks to need further growing to accommodate this. Or > maybe not - at least the extreme example given would still be fine; more > generally packets being limited to below 64k means 2*16 slots would > suffice at one end of the scale, while 2*MAX_PENDING_REQS would at the > other end (all tiny, including the primary slot). What I haven't fully > convinced myself of is whether there might be cases in the middle which > are yet worse.
See above reasoning. I think it is okay, but maybe I'm missing something.
Juergen
[unhandled content-type:application/pgp-keys][unhandled content-type:application/pgp-signature] | |