| Message ID | f11d8399e17bc82f9ffcb613da0a457a96f56fec.1631289870.git.lorenzo@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v14,bpf-next,01/18] net: skbuff: add size metadata to skb_shared_info for xdp | expand |
Lorenzo Bianconi <lorenzo@kernel.org> writes: > ... > diff --git a/drivers/net/ethernet/marvell/mvneta.c > b/drivers/net/ethernet/marvell/mvneta.c > index 9d460a270601..0c7b84ca6efc 100644 > --- a/drivers/net/ethernet/marvell/mvneta.c > +++ b/drivers/net/ethernet/marvell/mvneta.c > ... > @@ -2320,8 +2325,12 @@ mvneta_swbm_build_skb(struct mvneta_port > *pp, struct page_pool *pool, > struct xdp_buff *xdp, u32 desc_status) > { > struct skb_shared_info *sinfo = > xdp_get_shared_info_from_buff(xdp); > - int i, num_frags = sinfo->nr_frags; > struct sk_buff *skb; > + u8 num_frags; > + int i; > + > + if (unlikely(xdp_buff_is_mb(xdp))) > + num_frags = sinfo->nr_frags; Hi, nit, it seems that the num_frags assignment can be moved after the other 'if' condition you added (right before the 'for' for num_frags), or even be eliminated completely so that sinfo->nr_frags is used directly. Either way it looks like you can remove one 'if'. Shay > > skb = build_skb(xdp->data_hard_start, PAGE_SIZE); > if (!skb) > @@ -2333,6 +2342,9 @@ mvneta_swbm_build_skb(struct mvneta_port > *pp, struct page_pool *pool, > skb_put(skb, xdp->data_end - xdp->data); > skb->ip_summed = mvneta_rx_csum(pp, desc_status); > > + if (likely(!xdp_buff_is_mb(xdp))) > + goto out; > + > for (i = 0; i < num_frags; i++) { > skb_frag_t *frag = &sinfo->frags[i]; > > @@ -2341,6 +2353,7 @@ mvneta_swbm_build_skb(struct mvneta_port > *pp, struct page_pool *pool, > skb_frag_size(frag), PAGE_SIZE); > } > > +out: > return skb; > }
> > Lorenzo Bianconi <lorenzo@kernel.org> writes: > > > ... > > diff --git a/drivers/net/ethernet/marvell/mvneta.c > > b/drivers/net/ethernet/marvell/mvneta.c > > index 9d460a270601..0c7b84ca6efc 100644 > > --- a/drivers/net/ethernet/marvell/mvneta.c > > +++ b/drivers/net/ethernet/marvell/mvneta.c > > ... > > @@ -2320,8 +2325,12 @@ mvneta_swbm_build_skb(struct mvneta_port *pp, > > struct page_pool *pool, > > struct xdp_buff *xdp, u32 desc_status) > > { > > struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp); > > - int i, num_frags = sinfo->nr_frags; > > struct sk_buff *skb; > > + u8 num_frags; > > + int i; > > + > > + if (unlikely(xdp_buff_is_mb(xdp))) > > + num_frags = sinfo->nr_frags; > > Hi, > nit, it seems that the num_frags assignment can be moved after the other > 'if' condition you added (right before the 'for' for num_frags), or even be > eliminated completely so that sinfo->nr_frags is used directly. > Either way it looks like you can remove one 'if'. > > Shay Hi Shay, we can't move nr_frags assignement after build_skb() since this field will be overwritten by that call. Regards, Lorenzo > > > skb = build_skb(xdp->data_hard_start, PAGE_SIZE); > > if (!skb) > > @@ -2333,6 +2342,9 @@ mvneta_swbm_build_skb(struct mvneta_port *pp, > > struct page_pool *pool, > > skb_put(skb, xdp->data_end - xdp->data); > > skb->ip_summed = mvneta_rx_csum(pp, desc_status); > > + if (likely(!xdp_buff_is_mb(xdp))) > > + goto out; > > + > > for (i = 0; i < num_frags; i++) { > > skb_frag_t *frag = &sinfo->frags[i]; > > @@ -2341,6 +2353,7 @@ mvneta_swbm_build_skb(struct mvneta_port *pp, > > struct page_pool *pool, > > skb_frag_size(frag), PAGE_SIZE); > > } > > +out: > > return skb; > > } >
Lorenzo Bianconi <lorenzo.bianconi@redhat.com> writes: >> >> Lorenzo Bianconi <lorenzo@kernel.org> writes: >> >> > ... >> > diff --git a/drivers/net/ethernet/marvell/mvneta.c >> > b/drivers/net/ethernet/marvell/mvneta.c >> > index 9d460a270601..0c7b84ca6efc 100644 >> > --- a/drivers/net/ethernet/marvell/mvneta.c >> > +++ b/drivers/net/ethernet/marvell/mvneta.c >> > ... >> > @@ -2320,8 +2325,12 @@ mvneta_swbm_build_skb(struct >> > mvneta_port *pp, >> > struct page_pool *pool, >> > struct xdp_buff *xdp, u32 desc_status) >> > { >> > struct skb_shared_info *sinfo = >> > xdp_get_shared_info_from_buff(xdp); >> > - int i, num_frags = sinfo->nr_frags; >> > struct sk_buff *skb; >> > + u8 num_frags; >> > + int i; >> > + >> > + if (unlikely(xdp_buff_is_mb(xdp))) >> > + num_frags = sinfo->nr_frags; >> >> Hi, >> nit, it seems that the num_frags assignment can be moved after >> the other >> 'if' condition you added (right before the 'for' for >> num_frags), or even be >> eliminated completely so that sinfo->nr_frags is used directly. >> Either way it looks like you can remove one 'if'. >> >> Shay > > Hi Shay, > > we can't move nr_frags assignement after build_skb() since this > field will be > overwritten by that call. > > Regards, > Lorenzo > Sorry, silly mistake of me. Guess this assignment can be done anyway since there doesn't seem to be new cache misses introduced by it. Anyway, nice catch, sorry for misleading you >> >> > skb = build_skb(xdp->data_hard_start, PAGE_SIZE); >> > if (!skb) >> > @@ -2333,6 +2342,9 @@ mvneta_swbm_build_skb(struct >> > mvneta_port *pp, >> > struct page_pool *pool, >> > skb_put(skb, xdp->data_end - xdp->data); >> > skb->ip_summed = mvneta_rx_csum(pp, desc_status); >> > + if (likely(!xdp_buff_is_mb(xdp))) >> > + goto out; >> > + >> > for (i = 0; i < num_frags; i++) { >> > skb_frag_t *frag = &sinfo->frags[i]; >> > @@ -2341,6 +2353,7 @@ mvneta_swbm_build_skb(struct >> > mvneta_port *pp, >> > struct page_pool *pool, >> > skb_frag_size(frag), PAGE_SIZE); >> > } >> > +out: >> > return skb; >> > } >>
> > Lorenzo Bianconi <lorenzo.bianconi@redhat.com> writes: > > > > > > > Lorenzo Bianconi <lorenzo@kernel.org> writes: > > > > > > > ... > > > > diff --git a/drivers/net/ethernet/marvell/mvneta.c > > > > b/drivers/net/ethernet/marvell/mvneta.c > > > > index 9d460a270601..0c7b84ca6efc 100644 > > > > --- a/drivers/net/ethernet/marvell/mvneta.c > > > > +++ b/drivers/net/ethernet/marvell/mvneta.c > > > > ... > > > > @@ -2320,8 +2325,12 @@ mvneta_swbm_build_skb(struct > mvneta_port > > > *pp, > > > > struct page_pool *pool, > > > > struct xdp_buff *xdp, u32 desc_status) > > > > { > > > > struct skb_shared_info *sinfo = > > > > xdp_get_shared_info_from_buff(xdp); > > > > - int i, num_frags = sinfo->nr_frags; > > > > struct sk_buff *skb; > > > > + u8 num_frags; > > > > + int i; > > > > + > > > > + if (unlikely(xdp_buff_is_mb(xdp))) > > > > + num_frags = sinfo->nr_frags; > > > > > > Hi, > > > nit, it seems that the num_frags assignment can be moved after the > > > other > > > 'if' condition you added (right before the 'for' for num_frags), or > > > even be > > > eliminated completely so that sinfo->nr_frags is used directly. > > > Either way it looks like you can remove one 'if'. > > > > > > Shay > > > > Hi Shay, > > > > we can't move nr_frags assignement after build_skb() since this field > > will be > > overwritten by that call. > > > > Regards, > > Lorenzo > > > > Sorry, silly mistake of me. > > Guess this assignment can be done anyway since there doesn't seem to be new > cache misses introduced by it. > Anyway, nice catch, sorry for misleading you actually we probably have a cache miss in this case for the single-buffer use case since skb_shared_info will not be in the same cache-line. Regards, Lorenzo > > > > > > > > skb = build_skb(xdp->data_hard_start, PAGE_SIZE); > > > > if (!skb) > > > > @@ -2333,6 +2342,9 @@ mvneta_swbm_build_skb(struct > mvneta_port > > > *pp, > > > > struct page_pool *pool, > > > > skb_put(skb, xdp->data_end - xdp->data); > > > > skb->ip_summed = mvneta_rx_csum(pp, desc_status); > > > > + if (likely(!xdp_buff_is_mb(xdp))) > > > > + goto out; > > > > + > > > > for (i = 0; i < num_frags; i++) { > > > > skb_frag_t *frag = &sinfo->frags[i]; > > > > @@ -2341,6 +2353,7 @@ mvneta_swbm_build_skb(struct > > > > mvneta_port *pp, > > > > struct page_pool *pool, > > > > skb_frag_size(frag), PAGE_SIZE); > > > > } > > > > +out: > > > > return skb; > > > > } > > > >
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c index 9d460a270601..0c7b84ca6efc 100644 --- a/drivers/net/ethernet/marvell/mvneta.c +++ b/drivers/net/ethernet/marvell/mvneta.c @@ -2037,9 +2037,14 @@ mvneta_xdp_put_buff(struct mvneta_port *pp, struct mvneta_rx_queue *rxq, { int i; + if (likely(!xdp_buff_is_mb(xdp))) + goto out; + for (i = 0; i < sinfo->nr_frags; i++) page_pool_put_full_page(rxq->page_pool, skb_frag_page(&sinfo->frags[i]), true); + +out: page_pool_put_page(rxq->page_pool, virt_to_head_page(xdp->data), sync_len, true); } @@ -2241,7 +2246,6 @@ mvneta_swbm_rx_frame(struct mvneta_port *pp, int data_len = -MVNETA_MH_SIZE, len; struct net_device *dev = pp->dev; enum dma_data_direction dma_dir; - struct skb_shared_info *sinfo; if (*size > MVNETA_MAX_RX_BUF_SIZE) { len = MVNETA_MAX_RX_BUF_SIZE; @@ -2261,11 +2265,9 @@ mvneta_swbm_rx_frame(struct mvneta_port *pp, /* Prefetch header */ prefetch(data); + xdp_buff_clear_mb(xdp); xdp_prepare_buff(xdp, data, pp->rx_offset_correction + MVNETA_MH_SIZE, data_len, false); - - sinfo = xdp_get_shared_info_from_buff(xdp); - sinfo->nr_frags = 0; } static void @@ -2299,6 +2301,9 @@ mvneta_swbm_add_rx_fragment(struct mvneta_port *pp, skb_frag_off_set(frag, pp->rx_offset_correction); skb_frag_size_set(frag, data_len); __skb_frag_set_page(frag, page); + + if (!xdp_buff_is_mb(xdp)) + xdp_buff_set_mb(xdp); } else { page_pool_put_full_page(rxq->page_pool, page, true); } @@ -2320,8 +2325,12 @@ mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool, struct xdp_buff *xdp, u32 desc_status) { struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp); - int i, num_frags = sinfo->nr_frags; struct sk_buff *skb; + u8 num_frags; + int i; + + if (unlikely(xdp_buff_is_mb(xdp))) + num_frags = sinfo->nr_frags; skb = build_skb(xdp->data_hard_start, PAGE_SIZE); if (!skb) @@ -2333,6 +2342,9 @@ mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool, skb_put(skb, xdp->data_end - xdp->data); skb->ip_summed = mvneta_rx_csum(pp, desc_status); + if (likely(!xdp_buff_is_mb(xdp))) + goto out; + for (i = 0; i < num_frags; i++) { skb_frag_t *frag = &sinfo->frags[i]; @@ -2341,6 +2353,7 @@ mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool, skb_frag_size(frag), PAGE_SIZE); } +out: return skb; }
Update multi-buffer bit (mb) in xdp_buff to notify XDP/eBPF layer and XDP remote drivers if this is a "non-linear" XDP buffer. Access skb_shared_info only if xdp_buff mb is set in order to avoid possible cache-misses. Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> --- drivers/net/ethernet/marvell/mvneta.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-)