On 12/08/2025 18.48, Alexander Lobakin wrote:
From: Jakub Kicinski <kuba@xxxxxxxxxx>
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@xxxxxxxxxx>
---
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@xxxxxxxxxx
CC: daniel@xxxxxxxxxxxxx
CC: hawk@xxxxxxxxxx
CC: lorenzo@xxxxxxxxxx
CC: toke@xxxxxxxxxx
CC: john.fastabend@xxxxxxxxx
CC: sdf@xxxxxxxxxxx
CC: michael.chan@xxxxxxxxxxxx
CC: anthony.l.nguyen@xxxxxxxxx
CC: przemyslaw.kitszel@xxxxxxxxx
CC: marcin.s.wojtas@xxxxxxxxx
CC: tariqt@xxxxxxxxxx
CC: mbloch@xxxxxxxxxx
CC: eperezma@xxxxxxxxxx
CC: bpf@xxxxxxxxxxxxxxx
---
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(-)
