Re: [RFC] xdp: pass flags to xdp_update_skb_shared_info() directly

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 13 Aug 2025 10:43:21 +0200 Jesper Dangaard Brouer wrote:
> >> Does anyone prefer the current form of the API, or can we change
> >> as prosposed?
> 
> I like the proposed change.
> The only thing that confuses me was that the u32 flags is named
> "skb_flags" and not "xdp_flags".
> 
> @@ -314,7 +313,7 @@
>   static inline void
>   xdp_update_skb_shared_info(struct sk_buff *skb, u8 nr_frags,
>   			   unsigned int size, unsigned int truesize,
> -			   bool pfmemalloc)
> +			   u32 skb_flags)

It was matching the helper names: xdp_buff_get_skb_flags()

If we rename it to xdp_flags here do you want me to keep
the helpers (xdp_buff_get_flags()?) or access buf->flags
directly in the caller?

The idea was that the helper could filter / transform
the flags to whatever the update function takes. And the skb_ 
in the helper name was matching the skb_ of the arg.

> >> Bonus question: while Im messing with this API could I rename
> >> xdp_update_skb_shared_info()? Maybe to xdp_update_skb_state() ?
> >> Not sure why the function name has "shared_info" when most of
> >> what it updates is skb fields.  
> > 
> > I can only suspect that the author decided to name it this way due to
> > that it's only used when xdp_buff has frags (and frags are in shinfo).
> > But I agree it's not the best choice. xdp_update_skb_state() sounds fine
> > to me, but given that it's all about frags, maybe something like
> > xdp_update_skb_frags_info/state() or so?
> 
> Yes, function is only used when skb_shared_info have already been touched.
> 
> Performance wise it can be expensive to touch the cache-line for
> skb_shared_info, so the code carefully checks xdp_buff_has_frags() (flag
> XDP_FLAGS_HAS_FRAGS) before deref of skb_shared_info memory area.
> 
> Calling it xdp_update_skb_state() seems misleading. As Olek says, this
> is about updating the "skb_frags".  The original intent is that
> xdp_buff/xdp_frame is using same skb_shared_info area as SKB, and when
> transitioning to a "full" SKB then we need to do some adjustments.
> (Looking at function code, it is of-cause confusing that it doesn't
> touch sinfo->frags[] array, but that is because we don't need to, as
> non-linear XDP and SKB have same layout.).

Let's go with xdp_update_skb_frags_info(), then.