On Fri, Jan 21, 2022 at 01:29:20PM +0100, Alexander Lobakin wrote:
> From: Maciej Fijalkowski <maciej.fijalkowski@xxxxxxxxx>
> 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@xxxxxxxxx>
> > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@xxxxxxxxx>
> > ---
> > 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
:)
(...)
>
> > +
> > +/**
> > + * 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.
Ugh nice catch!
> 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)
Although user would some of the freedom it makes a lot of sense to me.
Jesse, what's your view?
>
> 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.
ack I'll try to use existing macros.
>
> > + 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
