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

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CY4PR1101MB2360B76C18FDEECAFE3169EE90A39@CY4PR1101MB2360.namprd11.prod.outlook.com>
Date:   Wed, 15 Feb 2023 04:37:47 +0000
From:   "Sarkar, Tirthendu" <tirthendu.sarkar@...el.com>
To:     "Fijalkowski, Maciej" <maciej.fijalkowski@...el.com>
CC:     "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
        "Brandeburg, Jesse" <jesse.brandeburg@...el.com>,
        "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>
Subject: RE: [PATCH intel-next v3 8/8] i40e: add support for XDP multi-buffer
 Rx

> -----Original Message-----
> From: Fijalkowski, Maciej <maciej.fijalkowski@...el.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@...el.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.

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