Re: [PATCH RFC bpf-next v2 10/17] bnxt: Propagate trait presence to skb

["Arthur Fabre" <arthur@arthurfabre.com>]

On Wed Apr 23, 2025 at 6:36 PM CEST, Stanislav Fomichev wrote:
> On 04/22, Arthur Fabre wrote:
> > Call the common xdp_buff_update_skb() helper.
> > 
> > Signed-off-by: Arthur Fabre <arthur@arthurfabre.com>
> > ---
> >  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > index c8e3468eee612ad622bfbecfd7cc1ae3396061fd..0eba3e307a3edbc5fe1abf2fa45e6256d98574c2 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > @@ -2297,6 +2297,10 @@ static int bnxt_rx_pkt(struct bnxt *bp, struct bnxt_cp_ring_info *cpr,
> >  			}
> >  		}
> >  	}
> > +
> > +	if (xdp_active)
> > +		xdp_buff_update_skb(&xdp, skb);
>
> For me, the preference for reusing existing metadata area was
> because of the patches 10-16: we now need to care about two types of
> metadata explicitly.

Having to update all the drivers is definitely not ideal. Motivation is:

1. Avoid trait_set() and xdp_adjust_meta() from corrupting each other's
   data. 
   But that's not a problem if we disallow trait_set() and
   xdp_adjust_meta() to be used at the same time, so maybe not a good
   reason anymore (except for maybe 3.)

2. Not have the traits at the "end" of the headroom (ie right next to
   actual packet data).
   If it's at the "end", we need to move all of it to make room for
   every xdp_adjust_head() call.
   It seems more intrusive to the current SKB API: several funcs assume
   that there is headroom directly before the packet.

3. I'm not sure how this should be exposed with AF_XDP yet. Either:
   * Expose raw trait storage, and having it at the "end" of the
     headroom is nice. But userspace would need to know how to parse the
	 header.
   * Require the XDP program to copy the traits it wants into the XDP
     metadata area, which is already exposed to userspace. That would
	 need traits and XDP metadata to coexist.

>
> If you insist on placing it into the headroom, can we at least have some
> common helper to finish xdp->skb conversion? It can call skb_ext_from_headroom
> and/or skb_metadata_set:
>
> xdp_buff_done(*xdp, *skb) {
> 	if (have traits) {
> 		skb_ext_from_headroom
> 		return
> 	}
>
> 	metasize = xdp->data - xdp->data_meta;
> 	if (metasize)
> 		skb_metadata_set
> }
>
> And then we'll have some common rules for the drivers: call xdp_buff_done
> when you're done with the xdp_buff to take care of metadata/traits. And
> it might be easier to review: you're gonna (mostly) change existing
> calls to skb_metadata_set to your new helper. Maybe we can even
> eventually fold all xdp_update_skb_shared_info stuff into that as
> well...

Yes! This is what I was going for with xdp_buff_update_skb() - it would be
nice for it handle all the SKB updating, including skb_metadata_set().

Should I do that first, and submit it as separate series()?