netdev - Re: [PATCH bpf-next v3 2/7] ice: xsk: handle SW XDP ring wrap and bump tail more often

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Ye7XhNFwC/5RggCL@boxer>
Date:   Mon, 24 Jan 2022 17:44:52 +0100
From:   Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To:     Alexander Lobakin <alexandr.lobakin@...el.com>
Cc:     bpf@...r.kernel.org, ast@...nel.org, daniel@...earbox.net,
        netdev@...r.kernel.org, magnus.karlsson@...el.com,
        jesse.brandeburg@...el.com
Subject: Re: [PATCH bpf-next v3 2/7] ice: xsk: handle SW XDP ring wrap and
 bump tail more often

On Fri, Jan 21, 2022 at 03:31:31PM +0100, Maciej Fijalkowski wrote:
> On Fri, Jan 21, 2022 at 01:29:20PM +0100, Alexander Lobakin wrote:
> > From: Maciej Fijalkowski <maciej.fijalkowski@...el.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@...el.com>
> > > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@...el.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
> 
> :)
> 
> 
> (...)
> 
> > 
> > > +
> > > +/**
> > > + * 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?

I decided to go with an approach that forbids xsk socket attachment when
ring length (either tx or rx) is not pow(2). Sending v4 with a separate
patch for that.

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