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

[Stanislav Fomichev <stfomichev@gmail.com>]

On 04/24, Toke Høiland-Jørgensen wrote:
> Stanislav Fomichev <stfomichev@gmail.com> writes:
> 
> > On 04/23, Arthur Fabre wrote:
> >> 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.
> >  
> > By keeping the traits at the tail we can just expose raw trait storage
> > and let userspace deal with it. But anyway, my main point is to avoid
> > having drivers to deal with two separate cases. As long as we can hide
> > everything behind a common call, we can change the placement later.
> 
> Being able to change the placement (and format) of the data store is the
> reason why we should absolutely *not* expose the internal trait storage
> to AF_XDP. Once we do that, we effectively make the whole thing UAPI.
> The trait get/set API very deliberately does not expose any details
> about the underlying storage for exactly this reason :)

I was under the impression that we want to eventually expose trait
blobs to the userspace via getsockopt (or some other similar means),
is it not the case? How is userspace supposed to consume it?