From: Alexander Lobakin <alexandr.lobakin@intel.com>
To: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Cc: Alexander Lobakin <alexandr.lobakin@intel.com>,
bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net,
netdev@vger.kernel.org, magnus.karlsson@intel.com
Subject: Re: [PATCH bpf-next v3 2/7] ice: xsk: handle SW XDP ring wrap and bump tail more often
Date: Fri, 21 Jan 2022 13:29:20 +0100 [thread overview]
Message-ID: <20220121122920.23679-1-alexandr.lobakin@intel.com> (raw)
In-Reply-To: <20220121120011.49316-3-maciej.fijalkowski@intel.com>
From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Date: Fri, 21 Jan 2022 13:00:06 +0100
> Currently, if ice_clean_rx_irq_zc() processed the whole ring and
> next_to_use != 0, then ice_alloc_rx_buf_zc() would not refill the whole
> ring even if the XSK buffer pool would have enough free entries (either
> from fill ring or the internal recycle mechanism) - it is because ring
> wrap is not handled.
>
> Improve the logic in ice_alloc_rx_buf_zc() to address the problem above.
> Do not clamp the count of buffers that is passed to
> xsk_buff_alloc_batch() in case when next_to_use + buffer count >=
> rx_ring->count, but rather split it and have two calls to the mentioned
> function - one for the part up until the wrap and one for the part after
> the wrap.
>
> Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> ---
> drivers/net/ethernet/intel/ice/ice_txrx.h | 2 +
> drivers/net/ethernet/intel/ice/ice_xsk.c | 99 ++++++++++++++++++-----
> 2 files changed, 81 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.h b/drivers/net/ethernet/intel/ice/ice_txrx.h
> index b7b3bd4816f0..94a46e0e5ed0 100644
> --- a/drivers/net/ethernet/intel/ice/ice_txrx.h
> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.h
> @@ -111,6 +111,8 @@ static inline int ice_skb_pad(void)
> (u16)((((R)->next_to_clean > (R)->next_to_use) ? 0 : (R)->count) + \
> (R)->next_to_clean - (R)->next_to_use - 1)
>
> +#define ICE_RING_QUARTER(R) ((R)->count / 4)
I would use `>> 2` here just to show off :D
> +
> #define ICE_TX_FLAGS_TSO BIT(0)
> #define ICE_TX_FLAGS_HW_VLAN BIT(1)
> #define ICE_TX_FLAGS_SW_VLAN BIT(2)
> diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
> index 2388837d6d6c..0463fc594d08 100644
> --- a/drivers/net/ethernet/intel/ice/ice_xsk.c
> +++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
> @@ -359,33 +359,28 @@ int ice_xsk_pool_setup(struct ice_vsi *vsi, struct xsk_buff_pool *pool, u16 qid)
> }
>
> /**
> - * ice_alloc_rx_bufs_zc - allocate a number of Rx buffers
> - * @rx_ring: Rx ring
> + * ice_fill_rx_descs - pick buffers from XSK buffer pool and use it
> + * @pool: XSK Buffer pool to pull the buffers from
> + * @xdp: SW ring of xdp_buff that will hold the buffers
> + * @rx_desc: Pointer to Rx descriptors that will be filled
> * @count: The number of buffers to allocate
> *
> * This function allocates a number of Rx buffers from the fill ring
> * or the internal recycle mechanism and places them on the Rx ring.
> *
> - * Returns true if all allocations were successful, false if any fail.
> + * Note that ring wrap should be handled by caller of this function.
> + *
> + * Returns the amount of allocated Rx descriptors
> */
> -bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> +static u16 ice_fill_rx_descs(struct xsk_buff_pool *pool, struct xdp_buff **xdp,
> + union ice_32b_rx_flex_desc *rx_desc, u16 count)
> {
> - union ice_32b_rx_flex_desc *rx_desc;
> - u16 ntu = rx_ring->next_to_use;
> - struct xdp_buff **xdp;
> - u32 nb_buffs, i;
> dma_addr_t dma;
> + u16 buffs;
> + int i;
>
> - rx_desc = ICE_RX_DESC(rx_ring, ntu);
> - xdp = ice_xdp_buf(rx_ring, ntu);
> -
> - nb_buffs = min_t(u16, count, rx_ring->count - ntu);
> - nb_buffs = xsk_buff_alloc_batch(rx_ring->xsk_pool, xdp, nb_buffs);
> - if (!nb_buffs)
> - return false;
> -
> - i = nb_buffs;
> - while (i--) {
> + buffs = xsk_buff_alloc_batch(pool, xdp, count);
> + for (i = 0; i < buffs; i++) {
> dma = xsk_buff_xdp_get_dma(*xdp);
> rx_desc->read.pkt_addr = cpu_to_le64(dma);
> rx_desc->wb.status_error0 = 0;
> @@ -394,13 +389,77 @@ bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> xdp++;
> }
>
> + return buffs;
> +}
> +
> +/**
> + * __ice_alloc_rx_bufs_zc - allocate a number of Rx buffers
> + * @rx_ring: Rx ring
> + * @count: The number of buffers to allocate
> + *
> + * Place the @count of descriptors onto Rx ring. Handle the ring wrap
> + * for case where space from next_to_use up to the end of ring is less
> + * than @count. Finally do a tail bump.
> + *
> + * Returns true if all allocations were successful, false if any fail.
> + */
> +static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> +{
> + union ice_32b_rx_flex_desc *rx_desc;
> + u32 nb_buffs_extra = 0, nb_buffs;
> + u16 ntu = rx_ring->next_to_use;
> + u16 total_count = count;
> + struct xdp_buff **xdp;
> +
> + rx_desc = ICE_RX_DESC(rx_ring, ntu);
> + xdp = ice_xdp_buf(rx_ring, ntu);
> +
> + if (ntu + count >= rx_ring->count) {
> + nb_buffs_extra = ice_fill_rx_descs(rx_ring->xsk_pool, xdp,
> + rx_desc,
> + rx_ring->count - ntu);
> + rx_desc = ICE_RX_DESC(rx_ring, 0);
> + xdp = ice_xdp_buf(rx_ring, 0);
> + ntu = 0;
> + count -= nb_buffs_extra;
> + ice_release_rx_desc(rx_ring, 0);
> + }
> +
> + nb_buffs = ice_fill_rx_descs(rx_ring->xsk_pool, xdp, rx_desc, count);
> +
> ntu += nb_buffs;
> if (ntu == rx_ring->count)
> ntu = 0;
>
> - ice_release_rx_desc(rx_ring, ntu);
> + if (rx_ring->next_to_use != ntu)
> + ice_release_rx_desc(rx_ring, ntu);
> +
> + return total_count == (nb_buffs_extra + nb_buffs);
> +}
> +
> +/**
> + * ice_alloc_rx_bufs_zc - allocate a number of Rx buffers
> + * @rx_ring: Rx ring
> + * @count: The number of buffers to allocate
> + *
> + * Wrapper for internal allocation routine; figure out how many tail
> + * bumps should take place based on the given threshold
> + *
> + * Returns true if all calls to internal alloc routine succeeded
> + */
> +bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> +{
> + u16 rx_thresh = ICE_RING_QUARTER(rx_ring);
> + u16 batched, leftover, i, tail_bumps;
> +
> + batched = count & ~(rx_thresh - 1);
The ring size can be a non power-of-two unfortunately, it is rather
aligned to just 32: [0]. So it can be e.g. 96 and the mask will
break then.
You could use roundup_pow_of_two(ICE_RING_QUARTER(rx_ring)), but
might can be a little slower due to fls_long() (bitsearch) inside.
(I would generally prohibit non-pow-2 ring sizes at all from inside
the Ethtool callbacks since it makes no sense to me :p)
Also, it's not recommended to open-code align-down since we got
the ALIGN_DOWN(value, pow_of_two_alignment) macro. The macro hell
inside expands to the same op you do in here.
> + tail_bumps = batched / rx_thresh;
> + leftover = count & (rx_thresh - 1);
>
> - return count == nb_buffs;
> + for (i = 0; i < tail_bumps; i++)
> + if (!__ice_alloc_rx_bufs_zc(rx_ring, rx_thresh))
> + return false;
> + return __ice_alloc_rx_bufs_zc(rx_ring, leftover);
> }
>
> /**
> --
> 2.33.1
[0] https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/intel/ice/ice_ethtool.c#L2729
Thanks,
Al
next prev parent reply other threads:[~2022-01-21 12:31 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-21 12:00 [PATCH v3 bpf-next 0/7] xsk: Intel driver improvements Maciej Fijalkowski
2022-01-21 12:00 ` [PATCH bpf-next v3 1/7] ice: remove likely for napi_complete_done Maciej Fijalkowski
2022-01-21 12:00 ` [PATCH bpf-next v3 2/7] ice: xsk: handle SW XDP ring wrap and bump tail more often Maciej Fijalkowski
2022-01-21 12:29 ` Alexander Lobakin [this message]
2022-01-21 14:31 ` Maciej Fijalkowski
2022-01-24 16:44 ` Maciej Fijalkowski
2022-01-21 12:00 ` [PATCH bpf-next v3 3/7] ice: make Tx threshold dependent on ring length Maciej Fijalkowski
2022-01-21 12:40 ` Rx: " Alexander Lobakin
2022-01-21 14:34 ` Maciej Fijalkowski
2022-01-21 12:00 ` [PATCH bpf-next v3 4/7] i40e: xsk: move tmp desc array from driver to pool Maciej Fijalkowski
2022-01-21 12:00 ` [PATCH bpf-next v3 5/7] ice: xsk: avoid potential dead AF_XDP Tx processing Maciej Fijalkowski
2022-01-21 12:00 ` [PATCH bpf-next v3 6/7] ice: xsk: improve AF_XDP ZC Tx and use batching API Maciej Fijalkowski
2022-01-21 12:54 ` Alexander Lobakin
2022-01-21 13:17 ` Alexander Lobakin
2022-01-21 14:41 ` Maciej Fijalkowski
2022-01-21 12:00 ` [PATCH bpf-next v3 7/7] ice: xsk: borrow xdp_tx_active logic from i40e Maciej Fijalkowski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220121122920.23679-1-alexandr.lobakin@intel.com \
--to=alexandr.lobakin@intel.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=maciej.fijalkowski@intel.com \
--cc=magnus.karlsson@intel.com \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).