On 13/08/2025 23.44, Jakub Kicinski wrote:
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.
It makes sense to have a helper, as you argue.
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.
Fine with me. It was Olek's naming suggestions (and I liked both).
Thanks
--Jesper
