Re: [PATCH v21 bpf-next 06/23] net: marvell: rely on xdp_update_skb_shared_info utility routine

[Andy Gospodarek <andrew.gospodarek@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 4890 bytes --]

On Tue, Jan 11, 2022 at 02:05:23PM +0100, Lorenzo Bianconi wrote:
> >
> > On Sat, Jan 08, 2022 at 12:53:09PM +0100, Lorenzo Bianconi wrote:
> > > Rely on xdp_update_skb_shared_info routine in order to avoid
> > > resetting frags array in skb_shared_info structure building
> > > the skb in mvneta_swbm_build_skb(). Frags array is expected to
> > > be initialized by the receiving driver building the xdp_buff
> > > and here we just need to update memory metadata.
> > >
> > > Acked-by: Toke Hoiland-Jorgensen <toke@redhat.com>
> > > Acked-by: John Fastabend <john.fastabend@gmail.com>
> > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > ---
> > >  drivers/net/ethernet/marvell/mvneta.c | 23 ++++++++++-------------
> > >  1 file changed, 10 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> > > index 775ffd91b741..267a306d9c75 100644
> > > --- a/drivers/net/ethernet/marvell/mvneta.c
> > > +++ b/drivers/net/ethernet/marvell/mvneta.c
> > > @@ -2332,8 +2332,12 @@ mvneta_swbm_add_rx_fragment(struct mvneta_port *pp,
> > >               skb_frag_size_set(frag, data_len);
> > >               __skb_frag_set_page(frag, page);
> > >
> > > -             if (!xdp_buff_is_mb(xdp))
> > > +             if (!xdp_buff_is_mb(xdp)) {
> > > +                     sinfo->xdp_frags_size = *size;
> > >                       xdp_buff_set_mb(xdp);
> > > +             }
> > > +             if (page_is_pfmemalloc(page))
> > > +                     xdp_buff_set_frag_pfmemalloc(xdp);
> > >       } else {
> > >               page_pool_put_full_page(rxq->page_pool, page, true);
> > >       }
> > > @@ -2347,7 +2351,6 @@ mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool,
> > >       struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
> > >       struct sk_buff *skb;
> > >       u8 num_frags;
> > > -     int i;
> > >
> > >       if (unlikely(xdp_buff_is_mb(xdp)))
> > >               num_frags = sinfo->nr_frags;
> > > @@ -2362,18 +2365,12 @@ 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];
> > > -
> > > -             skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
> > > -                             skb_frag_page(frag), skb_frag_off(frag),
> > > -                             skb_frag_size(frag), PAGE_SIZE);
> >
> > Maybe I'm missing something but I'm not sure you have a suitable
> > replacement for the 3 lines above this in your proposed change.
> >
> 
> Hi Andy,
> 
> mvneta_swbm_add_rx_fragment() initializes frags array in
> skb_shared_info for xdp whenever we receive a multi-descriptors frame.
> Since frags array is at the same offset for the xdp_buff and for the
> new skb and build_skb() in mvneta_swbm_build_skb() does not overwrite
> it, we do not need to initialize it again allocating the skb, just
> account metadata info running xdp_update_skb_shared_info(). Agree?
> 

Lorenzo,

Thanks for the explanation; I do agree.  I was thinking about this last night
and suspected this was the case.  My current implementation doesn't use
build_skb to reuse the DMA buffer; it allocates a new skb->data area for
passing up the stack.  This is why I needed the skb_frag_set_page calls and you
did not.  That may change, but the first implementation will probably continue
on that path.

Overall this set looks pretty solid to me.  Thanks to you and other
contributors for all the work on this!

-andy

> > > -     }
> > > +     if (unlikely(xdp_buff_is_mb(xdp)))
> > > +             xdp_update_skb_shared_info(skb, num_frags,
> > > +                                        sinfo->xdp_frags_size,
> > > +                                        num_frags * xdp->frame_sz,
> > > +                                        xdp_buff_is_frag_pfmemalloc(xdp));
> > >
> >
> > When I did an implementation of this on a different driver I also needed
> > to add:
> >
> >         for (i = 0; i < num_frags; i++)
> >                 skb_frag_set_page(skb, i, skb_frag_page(&sinfo->frags[i]));
> >
> > to make sure that frames that were given XDP_PASS were formatted
> > correctly so they could be handled by the stack.  Don't you need
> > something similar to make sure frags are properly set?
> >
> > Thanks,
> >
> > -andy
> >
> > P.S.  Sorry for noticing this so late in the process; I realize this version
> > was just a rebase of v20 and this would have been useful information
> > earlier if I'm correct.
> >
> 
> no worries :)
> 
> Regards,
> Lorenzo
> 
> > > -out:
> > >       return skb;
> > >  }
> > >
> > > --
> > > 2.33.1
> > >
> 

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4222 bytes --]