Re: [PATCH intel-next v3 8/8] i40e: add support for XDP multi-buffer Rx

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Wed, Feb 15, 2023 at 05:37:47AM +0100, Sarkar, Tirthendu wrote:
> > -----Original Message-----
> > From: Fijalkowski, Maciej <maciej.fijalkowski@intel.com>
> 
> [...]
> > > Previously i40e_alloc_rx_buffers() was called for every 32 cleaned
> > > buffers. For multi-buffers this may not be optimal as there may be more
> > > cleaned buffers in each i40e_clean_rx_irq() call. So this is now called
> > > when at least half of the ring size has been cleaned.
> > 
> > Please align this patch with xdp_features update
> >
> 
> ACK
>  
> > >
> > > Signed-off-by: Tirthendu Sarkar <tirthendu.sarkar@intel.com>
> > > ---
> > >  drivers/net/ethernet/intel/i40e/i40e_main.c |   4 +-
> > >  drivers/net/ethernet/intel/i40e/i40e_txrx.c | 314 +++++++++++++-------
> > >  drivers/net/ethernet/intel/i40e/i40e_txrx.h |   8 -
> > >  3 files changed, 209 insertions(+), 117 deletions(-)
> > >
> > 
> > (...)
> > 
> > >  }
> > >
> > > +/**
> > > + * i40e_add_xdp_frag: Add a frag to xdp_buff
> > > + * @xdp: xdp_buff pointing to the data
> > > + * @nr_frags: return number of buffers for the packet
> > > + * @rx_buffer: rx_buffer holding data of the current frag
> > > + * @size: size of data of current frag
> > > + */
> > > +static int i40e_add_xdp_frag(struct xdp_buff *xdp, u32 *nr_frags,
> > > +			     struct i40e_rx_buffer *rx_buffer, u32 size)
> > > +{
> > > +	struct skb_shared_info *sinfo =
> > xdp_get_shared_info_from_buff(xdp);
> > > +
> > > +	if (!xdp_buff_has_frags(xdp)) {
> > > +		sinfo->nr_frags = 0;
> > > +		sinfo->xdp_frags_size = 0;
> > > +		xdp_buff_set_frags_flag(xdp);
> > > +	} else if (unlikely(sinfo->nr_frags >= MAX_SKB_FRAGS)) {
> > > +		/* Overflowing packet: All frags need to be dropped */
> > > +		return  -ENOMEM;
> > 
> > nit: double space
> > 
> 
> ACK
> 
> [...]
> > > +		xdp_res = i40e_run_xdp(rx_ring, xdp, xdp_prog);
> > > +
> > >  		if (xdp_res) {
> > > -			if (xdp_res & (I40E_XDP_TX | I40E_XDP_REDIR)) {
> > > -				xdp_xmit |= xdp_res;
> > > +			xdp_xmit |= xdp_res & (I40E_XDP_TX |
> > I40E_XDP_REDIR);
> > 
> > what was wrong with having above included in the
> > 
> > 	} else if (xdp_res & (I40E_XDP_TX | I40E_XDP_REDIR)) {
> > 
> > branch?
> > 
> 
> For multi-buffer packets, only the first 'if' branch will be executed. We need to set
> xdp_xmit for both single and multi-buffer packets.

maybe pass xdp_xmit to i40e_process_rx_buffs and use it for buf_flip
initialization? also you trimmed the code, but in there please don't
define on-stack variables smaller than u32 (u16 next)

> 
> [...]
> > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.h
> > b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
> > > index e86abc25bb5e..14ad074639ab 100644
> > > --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.h
> > > +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
> > > @@ -393,14 +393,6 @@ struct i40e_ring {
> > >
> > >  	struct rcu_head rcu;		/* to avoid race on free */
> > >  	u16 next_to_alloc;
> > > -	struct sk_buff *skb;		/* When i40e_clean_rx_ring_irq()
> > must
> > > -					 * return before it sees the EOP for
> > > -					 * the current packet, we save that
> > skb
> > > -					 * here and resume receiving this
> > > -					 * packet the next time
> > > -					 * i40e_clean_rx_ring_irq() is called
> > > -					 * for this ring.
> > > -					 */
> > 
> > this comment was valuable to me back when i was getting started with i40e,
> > so maybe we could have something equivalent around xdp_buff now?
> > 
> 
> We have a similar comment for xdp_buff in patch #7 where it was introduced.

ah i jumped straight into the #8, sorry.

> 
> > >
> > >  	struct i40e_channel *ch;
> > >  	u16 rx_offset;
> > > --
> > > 2.34.1
> > >