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@xxxxxxxxxx>
> ---
> 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@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(-)
>
> 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:
signature.asc
Description: PGP signature
