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

[Jesper Dangaard Brouer <hawk@kernel.org>]

On 12/08/2025 18.48, Alexander Lobakin wrote:
> From: Jakub Kicinski <kuba@kernel.org>
> Date: Tue, 12 Aug 2025 09:15:28 -0700
> 
>> xdp_update_skb_shared_info() needs to update skb state which
>> was maintained in xdp_buff / frame. Pass full flags into it,
>> instead of breaking it out bit by bit. We will need to add
>> a bit for unreadable frags (even tho XDP doesn't support
>> those the driver paths may be common), at which point almost
>> all call sites would become:
>>
>>      xdp_update_skb_shared_info(skb, num_frags,
>>                                 sinfo->xdp_frags_size,
>>                                 MY_PAGE_SIZE * num_frags,
>>                                 xdp_buff_is_frag_pfmemalloc(xdp),
>>                                 xdp_buff_is_frag_unreadable(xdp));
> 
> Yeah I think this doesn't make sense, it just doesn't scale. We can make
> more flags in future and adding a new argument for each is not a good
> idea, even if more drivers would switch to generic
> xdp_build_skb_from_buff().
> 

I agree. And good reminder that some driver have already switched to the
generic xdp_build_skb_from_buff().

>>
>> Keep a helper for accessing the flags, in case we need to
>> transform them somehow in the future (e.g. to cover up xdp_buff
>> vs xdp_frame differences).
>>
>> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>> ---
>> 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)


>> 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.).

--Jesper

>>
>> CC: ast@kernel.org
>> CC: daniel@iogearbox.net
>> CC: hawk@kernel.org
>> CC: lorenzo@kernel.org
>> CC: toke@redhat.com
>> CC: john.fastabend@gmail.com
>> CC: sdf@fomichev.me
>> CC: michael.chan@broadcom.com
>> CC: anthony.l.nguyen@intel.com
>> CC: przemyslaw.kitszel@intel.com
>> CC: marcin.s.wojtas@gmail.com
>> CC: tariqt@nvidia.com
>> CC: mbloch@nvidia.com
>> CC: eperezma@redhat.com
>> CC: bpf@vger.kernel.org
>> ---
>>   include/net/xdp.h                             | 21 +++++++++----------
>>   drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c |  2 +-
>>   drivers/net/ethernet/intel/i40e/i40e_txrx.c   |  4 ++--
>>   drivers/net/ethernet/intel/ice/ice_txrx.c     |  4 ++--
>>   drivers/net/ethernet/marvell/mvneta.c         |  2 +-
>>   .../net/ethernet/mellanox/mlx5/core/en_rx.c   |  7 +++----
>>   drivers/net/virtio_net.c                      |  2 +-
>>   net/core/xdp.c                                | 11 +++++-----
>>   8 files changed, 26 insertions(+), 27 deletions(-)