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

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

[Jakub Kicinski]

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

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?

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.

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

diff --git a/include/net/xdp.h b/include/net/xdp.h
index b40f1f96cb11..2ff47f53ba26 100644
--- a/include/net/xdp.h
+++ b/include/net/xdp.h
@@ -104,17 +104,16 @@ static __always_inline void xdp_buff_clear_frags_flag(struct xdp_buff *xdp)
 	xdp->flags &= ~XDP_FLAGS_HAS_FRAGS;
 }
 
-static __always_inline bool
-xdp_buff_is_frag_pfmemalloc(const struct xdp_buff *xdp)
-{
-	return !!(xdp->flags & XDP_FLAGS_FRAGS_PF_MEMALLOC);
-}
-
 static __always_inline void xdp_buff_set_frag_pfmemalloc(struct xdp_buff *xdp)
 {
 	xdp->flags |= XDP_FLAGS_FRAGS_PF_MEMALLOC;
 }
 
+static __always_inline u32 xdp_buff_get_skb_flags(const struct xdp_buff *xdp)
+{
+	return xdp->flags;
+}
+
 static __always_inline void
 xdp_init_buff(struct xdp_buff *xdp, u32 frame_sz, struct xdp_rxq_info *rxq)
 {
@@ -272,10 +271,10 @@ static __always_inline bool xdp_frame_has_frags(const struct xdp_frame *frame)
 	return !!(frame->flags & XDP_FLAGS_HAS_FRAGS);
 }
 
-static __always_inline bool
-xdp_frame_is_frag_pfmemalloc(const struct xdp_frame *frame)
+static __always_inline u32
+xdp_frame_get_skb_flags(const struct xdp_frame *frame)
 {
-	return !!(frame->flags & XDP_FLAGS_FRAGS_PF_MEMALLOC);
+	return frame->flags;
 }
 
 #define XDP_BULK_QUEUE_SIZE	16
@@ -314,7 +313,7 @@ static inline void xdp_scrub_frame(struct xdp_frame *frame)
 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)
 {
 	struct skb_shared_info *sinfo = skb_shinfo(skb);
 
@@ -328,7 +327,7 @@ xdp_update_skb_shared_info(struct sk_buff *skb, u8 nr_frags,
 	skb->len += size;
 	skb->data_len += size;
 	skb->truesize += truesize;
-	skb->pfmemalloc |= pfmemalloc;
+	skb->pfmemalloc |= skb_flags & XDP_FLAGS_FRAGS_PF_MEMALLOC;
 }
 
 /* Avoids inlining WARN macro in fast-path */
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
index 58d579dca3f1..b35d4a8a8dac 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
@@ -471,6 +471,6 @@ bnxt_xdp_build_skb(struct bnxt *bp, struct sk_buff *skb, u8 num_frags,
 	xdp_update_skb_shared_info(skb, num_frags,
 				   sinfo->xdp_frags_size,
 				   BNXT_RX_PAGE_SIZE * num_frags,
-				   xdp_buff_is_frag_pfmemalloc(xdp));
+				   xdp_buff_get_skb_flags(xdp));
 	return skb;
 }
diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index 048c33039130..9cbd614a0d57 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -2154,7 +2154,7 @@ static struct sk_buff *i40e_construct_skb(struct i40e_ring *rx_ring,
 		xdp_update_skb_shared_info(skb, skinfo->nr_frags + nr_frags,
 					   sinfo->xdp_frags_size,
 					   nr_frags * xdp->frame_sz,
-					   xdp_buff_is_frag_pfmemalloc(xdp));
+					   xdp_buff_get_skb_flags(xdp));
 
 		/* First buffer has already been processed, so bump ntc */
 		if (++rx_ring->next_to_clean == rx_ring->count)
@@ -2209,7 +2209,7 @@ static struct sk_buff *i40e_build_skb(struct i40e_ring *rx_ring,
 		xdp_update_skb_shared_info(skb, nr_frags,
 					   sinfo->xdp_frags_size,
 					   nr_frags * xdp->frame_sz,
-					   xdp_buff_is_frag_pfmemalloc(xdp));
+					   xdp_buff_get_skb_flags(xdp));
 
 		i40e_process_rx_buffs(rx_ring, I40E_XDP_PASS, xdp);
 	} else {
diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
index 29e0088ab6b2..014b321e487e 100644
--- a/drivers/net/ethernet/intel/ice/ice_txrx.c
+++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
@@ -1038,7 +1038,7 @@ ice_build_skb(struct ice_rx_ring *rx_ring, struct xdp_buff *xdp)
 		xdp_update_skb_shared_info(skb, nr_frags,
 					   sinfo->xdp_frags_size,
 					   nr_frags * xdp->frame_sz,
-					   xdp_buff_is_frag_pfmemalloc(xdp));
+					   xdp_buff_get_skb_flags(xdp));
 
 	return skb;
 }
@@ -1118,7 +1118,7 @@ ice_construct_skb(struct ice_rx_ring *rx_ring, struct xdp_buff *xdp)
 		xdp_update_skb_shared_info(skb, skinfo->nr_frags + nr_frags,
 					   sinfo->xdp_frags_size,
 					   nr_frags * xdp->frame_sz,
-					   xdp_buff_is_frag_pfmemalloc(xdp));
+					   xdp_buff_get_skb_flags(xdp));
 	}
 
 	return skb;
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 476e73e502fe..79a6bd530619 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -2419,7 +2419,7 @@ mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool,
 		xdp_update_skb_shared_info(skb, num_frags,
 					   sinfo->xdp_frags_size,
 					   num_frags * xdp->frame_sz,
-					   xdp_buff_is_frag_pfmemalloc(xdp));
+					   xdp_buff_get_skb_flags(xdp));
 
 	return skb;
 }
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
index b8c609d91d11..abbe24f71f6a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
@@ -1798,8 +1798,7 @@ mlx5e_skb_from_cqe_nonlinear(struct mlx5e_rq *rq, struct mlx5e_wqe_frag_info *wi
 		/* sinfo->nr_frags is reset by build_skb, calculate again. */
 		xdp_update_skb_shared_info(skb, wi - head_wi - 1,
 					   sinfo->xdp_frags_size, truesize,
-					   xdp_buff_is_frag_pfmemalloc(
-						&mxbuf->xdp));
+					   xdp_buff_get_skb_flags(&mxbuf->xdp));
 
 		for (struct mlx5e_wqe_frag_info *pwi = head_wi + 1; pwi < wi; pwi++)
 			pwi->frag_page->frags++;
@@ -2107,7 +2106,7 @@ mlx5e_skb_from_cqe_mpwrq_nonlinear(struct mlx5e_rq *rq, struct mlx5e_mpw_info *w
 			/* sinfo->nr_frags is reset by build_skb, calculate again. */
 			xdp_update_skb_shared_info(skb, frag_page - head_page,
 						   sinfo->xdp_frags_size, truesize,
-						   xdp_buff_is_frag_pfmemalloc(
+						   xdp_buff_get_skb_flags(
 							&mxbuf->xdp));
 
 			pagep = head_page;
@@ -2124,7 +2123,7 @@ mlx5e_skb_from_cqe_mpwrq_nonlinear(struct mlx5e_rq *rq, struct mlx5e_mpw_info *w
 
 			xdp_update_skb_shared_info(skb, sinfo->nr_frags,
 						   sinfo->xdp_frags_size, truesize,
-						   xdp_buff_is_frag_pfmemalloc(
+						   xdp_buff_get_skb_flags(
 							&mxbuf->xdp));
 
 			pagep = frag_page - sinfo->nr_frags;
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index d14e6d602273..152b0d5c2122 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2188,7 +2188,7 @@ static struct sk_buff *build_skb_from_xdp_buff(struct net_device *dev,
 		xdp_update_skb_shared_info(skb, nr_frags,
 					   sinfo->xdp_frags_size,
 					   xdp_frags_truesz,
-					   xdp_buff_is_frag_pfmemalloc(xdp));
+					   xdp_buff_get_skb_flags(xdp));
 
 	return skb;
 }
diff --git a/net/core/xdp.c b/net/core/xdp.c
index 491334b9b8be..789051763209 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -665,7 +665,7 @@ struct sk_buff *xdp_build_skb_from_buff(const struct xdp_buff *xdp)
 		tsize = sinfo->xdp_frags_truesize ? : nr_frags * xdp->frame_sz;
 		xdp_update_skb_shared_info(skb, nr_frags,
 					   sinfo->xdp_frags_size, tsize,
-					   xdp_buff_is_frag_pfmemalloc(xdp));
+					   xdp_buff_get_skb_flags(xdp));
 	}
 
 	skb->protocol = eth_type_trans(skb, rxq->dev);
@@ -692,7 +692,7 @@ static noinline bool xdp_copy_frags_from_zc(struct sk_buff *skb,
 	struct skb_shared_info *sinfo = skb_shinfo(skb);
 	const struct skb_shared_info *xinfo;
 	u32 nr_frags, tsize = 0;
-	bool pfmemalloc = false;
+	u32 flags = 0;
 
 	xinfo = xdp_get_shared_info_from_buff(xdp);
 	nr_frags = xinfo->nr_frags;
@@ -714,11 +714,12 @@ static noinline bool xdp_copy_frags_from_zc(struct sk_buff *skb,
 		__skb_fill_page_desc_noacc(sinfo, i, page, offset, len);
 
 		tsize += truesize;
-		pfmemalloc |= page_is_pfmemalloc(page);
+		if (page_is_pfmemalloc(page))
+			flags |= XDP_FLAGS_FRAGS_PF_MEMALLOC;
 	}
 
 	xdp_update_skb_shared_info(skb, nr_frags, xinfo->xdp_frags_size,
-				   tsize, pfmemalloc);
+				   tsize, flags);
 
 	return true;
 }
@@ -826,7 +827,7 @@ struct sk_buff *__xdp_build_skb_from_frame(struct xdp_frame *xdpf,
 		xdp_update_skb_shared_info(skb, nr_frags,
 					   sinfo->xdp_frags_size,
 					   nr_frags * xdpf->frame_sz,
-					   xdp_frame_is_frag_pfmemalloc(xdpf));
+					   xdp_frame_get_skb_flags(xdpf));
 
 	/* Essential SKB info: protocol and skb->dev */
 	skb->protocol = eth_type_trans(skb, dev);
-- 
2.50.1

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

[Jakub Kicinski]

On Tue, 12 Aug 2025 09:15:28 -0700 Jakub Kicinski wrote:
> 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));
> 
> 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).

CC Olek

https://lore.kernel.org/all/20250812161528.835855-1-kuba@kernel.org/

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

[Alexander Lobakin]

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

> 
> 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?
> 
> 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?

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

Thanks,
Olek

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

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 11358 bytes --]

On Aug 12, Jakub Kicinski wrote:
> 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));
> 
> 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 prefer the new proposed APIs if we are adding new bits there.

Regards,
Lorenzo

> 
> 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.
> 
> 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(-)
> 
> diff --git a/include/net/xdp.h b/include/net/xdp.h
> index b40f1f96cb11..2ff47f53ba26 100644
> --- a/include/net/xdp.h
> +++ b/include/net/xdp.h
> @@ -104,17 +104,16 @@ static __always_inline void xdp_buff_clear_frags_flag(struct xdp_buff *xdp)
>  	xdp->flags &= ~XDP_FLAGS_HAS_FRAGS;
>  }
>  
> -static __always_inline bool
> -xdp_buff_is_frag_pfmemalloc(const struct xdp_buff *xdp)
> -{
> -	return !!(xdp->flags & XDP_FLAGS_FRAGS_PF_MEMALLOC);
> -}
> -
>  static __always_inline void xdp_buff_set_frag_pfmemalloc(struct xdp_buff *xdp)
>  {
>  	xdp->flags |= XDP_FLAGS_FRAGS_PF_MEMALLOC;
>  }
>  
> +static __always_inline u32 xdp_buff_get_skb_flags(const struct xdp_buff *xdp)
> +{
> +	return xdp->flags;
> +}
> +
>  static __always_inline void
>  xdp_init_buff(struct xdp_buff *xdp, u32 frame_sz, struct xdp_rxq_info *rxq)
>  {
> @@ -272,10 +271,10 @@ static __always_inline bool xdp_frame_has_frags(const struct xdp_frame *frame)
>  	return !!(frame->flags & XDP_FLAGS_HAS_FRAGS);
>  }
>  
> -static __always_inline bool
> -xdp_frame_is_frag_pfmemalloc(const struct xdp_frame *frame)
> +static __always_inline u32
> +xdp_frame_get_skb_flags(const struct xdp_frame *frame)
>  {
> -	return !!(frame->flags & XDP_FLAGS_FRAGS_PF_MEMALLOC);
> +	return frame->flags;
>  }
>  
>  #define XDP_BULK_QUEUE_SIZE	16
> @@ -314,7 +313,7 @@ static inline void xdp_scrub_frame(struct xdp_frame *frame)
>  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)
>  {
>  	struct skb_shared_info *sinfo = skb_shinfo(skb);
>  
> @@ -328,7 +327,7 @@ xdp_update_skb_shared_info(struct sk_buff *skb, u8 nr_frags,
>  	skb->len += size;
>  	skb->data_len += size;
>  	skb->truesize += truesize;
> -	skb->pfmemalloc |= pfmemalloc;
> +	skb->pfmemalloc |= skb_flags & XDP_FLAGS_FRAGS_PF_MEMALLOC;
>  }
>  
>  /* Avoids inlining WARN macro in fast-path */
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
> index 58d579dca3f1..b35d4a8a8dac 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
> @@ -471,6 +471,6 @@ bnxt_xdp_build_skb(struct bnxt *bp, struct sk_buff *skb, u8 num_frags,
>  	xdp_update_skb_shared_info(skb, num_frags,
>  				   sinfo->xdp_frags_size,
>  				   BNXT_RX_PAGE_SIZE * num_frags,
> -				   xdp_buff_is_frag_pfmemalloc(xdp));
> +				   xdp_buff_get_skb_flags(xdp));
>  	return skb;
>  }
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> index 048c33039130..9cbd614a0d57 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> @@ -2154,7 +2154,7 @@ static struct sk_buff *i40e_construct_skb(struct i40e_ring *rx_ring,
>  		xdp_update_skb_shared_info(skb, skinfo->nr_frags + nr_frags,
>  					   sinfo->xdp_frags_size,
>  					   nr_frags * xdp->frame_sz,
> -					   xdp_buff_is_frag_pfmemalloc(xdp));
> +					   xdp_buff_get_skb_flags(xdp));
>  
>  		/* First buffer has already been processed, so bump ntc */
>  		if (++rx_ring->next_to_clean == rx_ring->count)
> @@ -2209,7 +2209,7 @@ static struct sk_buff *i40e_build_skb(struct i40e_ring *rx_ring,
>  		xdp_update_skb_shared_info(skb, nr_frags,
>  					   sinfo->xdp_frags_size,
>  					   nr_frags * xdp->frame_sz,
> -					   xdp_buff_is_frag_pfmemalloc(xdp));
> +					   xdp_buff_get_skb_flags(xdp));
>  
>  		i40e_process_rx_buffs(rx_ring, I40E_XDP_PASS, xdp);
>  	} else {
> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
> index 29e0088ab6b2..014b321e487e 100644
> --- a/drivers/net/ethernet/intel/ice/ice_txrx.c
> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
> @@ -1038,7 +1038,7 @@ ice_build_skb(struct ice_rx_ring *rx_ring, struct xdp_buff *xdp)
>  		xdp_update_skb_shared_info(skb, nr_frags,
>  					   sinfo->xdp_frags_size,
>  					   nr_frags * xdp->frame_sz,
> -					   xdp_buff_is_frag_pfmemalloc(xdp));
> +					   xdp_buff_get_skb_flags(xdp));
>  
>  	return skb;
>  }
> @@ -1118,7 +1118,7 @@ ice_construct_skb(struct ice_rx_ring *rx_ring, struct xdp_buff *xdp)
>  		xdp_update_skb_shared_info(skb, skinfo->nr_frags + nr_frags,
>  					   sinfo->xdp_frags_size,
>  					   nr_frags * xdp->frame_sz,
> -					   xdp_buff_is_frag_pfmemalloc(xdp));
> +					   xdp_buff_get_skb_flags(xdp));
>  	}
>  
>  	return skb;
> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index 476e73e502fe..79a6bd530619 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -2419,7 +2419,7 @@ mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool,
>  		xdp_update_skb_shared_info(skb, num_frags,
>  					   sinfo->xdp_frags_size,
>  					   num_frags * xdp->frame_sz,
> -					   xdp_buff_is_frag_pfmemalloc(xdp));
> +					   xdp_buff_get_skb_flags(xdp));
>  
>  	return skb;
>  }
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> index b8c609d91d11..abbe24f71f6a 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> @@ -1798,8 +1798,7 @@ mlx5e_skb_from_cqe_nonlinear(struct mlx5e_rq *rq, struct mlx5e_wqe_frag_info *wi
>  		/* sinfo->nr_frags is reset by build_skb, calculate again. */
>  		xdp_update_skb_shared_info(skb, wi - head_wi - 1,
>  					   sinfo->xdp_frags_size, truesize,
> -					   xdp_buff_is_frag_pfmemalloc(
> -						&mxbuf->xdp));
> +					   xdp_buff_get_skb_flags(&mxbuf->xdp));
>  
>  		for (struct mlx5e_wqe_frag_info *pwi = head_wi + 1; pwi < wi; pwi++)
>  			pwi->frag_page->frags++;
> @@ -2107,7 +2106,7 @@ mlx5e_skb_from_cqe_mpwrq_nonlinear(struct mlx5e_rq *rq, struct mlx5e_mpw_info *w
>  			/* sinfo->nr_frags is reset by build_skb, calculate again. */
>  			xdp_update_skb_shared_info(skb, frag_page - head_page,
>  						   sinfo->xdp_frags_size, truesize,
> -						   xdp_buff_is_frag_pfmemalloc(
> +						   xdp_buff_get_skb_flags(
>  							&mxbuf->xdp));
>  
>  			pagep = head_page;
> @@ -2124,7 +2123,7 @@ mlx5e_skb_from_cqe_mpwrq_nonlinear(struct mlx5e_rq *rq, struct mlx5e_mpw_info *w
>  
>  			xdp_update_skb_shared_info(skb, sinfo->nr_frags,
>  						   sinfo->xdp_frags_size, truesize,
> -						   xdp_buff_is_frag_pfmemalloc(
> +						   xdp_buff_get_skb_flags(
>  							&mxbuf->xdp));
>  
>  			pagep = frag_page - sinfo->nr_frags;
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index d14e6d602273..152b0d5c2122 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2188,7 +2188,7 @@ static struct sk_buff *build_skb_from_xdp_buff(struct net_device *dev,
>  		xdp_update_skb_shared_info(skb, nr_frags,
>  					   sinfo->xdp_frags_size,
>  					   xdp_frags_truesz,
> -					   xdp_buff_is_frag_pfmemalloc(xdp));
> +					   xdp_buff_get_skb_flags(xdp));
>  
>  	return skb;
>  }
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index 491334b9b8be..789051763209 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -665,7 +665,7 @@ struct sk_buff *xdp_build_skb_from_buff(const struct xdp_buff *xdp)
>  		tsize = sinfo->xdp_frags_truesize ? : nr_frags * xdp->frame_sz;
>  		xdp_update_skb_shared_info(skb, nr_frags,
>  					   sinfo->xdp_frags_size, tsize,
> -					   xdp_buff_is_frag_pfmemalloc(xdp));
> +					   xdp_buff_get_skb_flags(xdp));
>  	}
>  
>  	skb->protocol = eth_type_trans(skb, rxq->dev);
> @@ -692,7 +692,7 @@ static noinline bool xdp_copy_frags_from_zc(struct sk_buff *skb,
>  	struct skb_shared_info *sinfo = skb_shinfo(skb);
>  	const struct skb_shared_info *xinfo;
>  	u32 nr_frags, tsize = 0;
> -	bool pfmemalloc = false;
> +	u32 flags = 0;
>  
>  	xinfo = xdp_get_shared_info_from_buff(xdp);
>  	nr_frags = xinfo->nr_frags;
> @@ -714,11 +714,12 @@ static noinline bool xdp_copy_frags_from_zc(struct sk_buff *skb,
>  		__skb_fill_page_desc_noacc(sinfo, i, page, offset, len);
>  
>  		tsize += truesize;
> -		pfmemalloc |= page_is_pfmemalloc(page);
> +		if (page_is_pfmemalloc(page))
> +			flags |= XDP_FLAGS_FRAGS_PF_MEMALLOC;
>  	}
>  
>  	xdp_update_skb_shared_info(skb, nr_frags, xinfo->xdp_frags_size,
> -				   tsize, pfmemalloc);
> +				   tsize, flags);
>  
>  	return true;
>  }
> @@ -826,7 +827,7 @@ struct sk_buff *__xdp_build_skb_from_frame(struct xdp_frame *xdpf,
>  		xdp_update_skb_shared_info(skb, nr_frags,
>  					   sinfo->xdp_frags_size,
>  					   nr_frags * xdpf->frame_sz,
> -					   xdp_frame_is_frag_pfmemalloc(xdpf));
> +					   xdp_frame_get_skb_flags(xdpf));
>  
>  	/* Essential SKB info: protocol and skb->dev */
>  	skb->protocol = eth_type_trans(skb, dev);
> -- 
> 2.50.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Jesper Dangaard Brouer]

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

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

[Toke Høiland-Jørgensen]

Jakub Kicinski <kuba@kernel.org> writes:

> 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));
>
> 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 think the change is fine, but I agree with Jesper that it's a bit
weird to call them skb_flags. Maybe just xdp_buff_get_flags()?

-Toke

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

[Jakub Kicinski]

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.

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

[Jesper Dangaard Brouer]

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