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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----Original Message-----
> From: Fijalkowski, Maciej <maciej.fijalkowski@xxxxxxxxx>

[...]
> > 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@xxxxxxxxx>
> > ---
> >  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.

[...]
> > 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.

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




[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux