On Wed, Feb 15, 2023 at 05:37:47AM +0100, Sarkar, Tirthendu wrote:
> > -----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.
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
> > >
