RE: [Intel-wired-lan] [PATCH intel-next v4 4/8] i40e: Change size to truesize when using i40e_rx_buffer_flip()

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

 



> -----Original Message-----
> From: Paul Menzel <pmenzel@xxxxxxxxxxxxx>
> Subject: Re: [Intel-wired-lan] [PATCH intel-next v4 4/8] i40e: Change size to
> truesize when using i40e_rx_buffer_flip()
> Importance: Low
> 
> Dear Tirthendu,
> 
> 
> Thank you for your patch.
> 
> Am 15.02.23 um 13:43 schrieb Tirthendu Sarkar:
> > Truesize is now passed directly to i40e_rx_buffer_flip() instead of size
> > so that it does not need to recalculate truesize from size using
> > i40e_rx_frame_truesize() before adjusting page offset.
> 
> Did the compiler not optimize that well enough?
> 

For flipping the buffer, data's 'truesize' is needed. So i40e_rx_buffer_flip() needed  to call 
i40e_rx_frame_truesize() to get the 'truesize' since only 'size' was passed to it. With this 
patch the caller provides 'truesize' itself to i40e_rx_buffer_flip().

> > With these change the function can now be used during skb building and
> > adding frags. In later patches it will also be easier for adjusting
> > page offsets for multi-buffers.
> 
> Why couldn’t the function be used before?
> 

i40e_rx_frame_trusize() does not cover all cases for truesize calculation , for e.g., for frags
in non-legacy-rx mode. Since i40e_rx_buffer_flip() was depending on i40e_rx_frame_trusize() 
for truesize, it could not be used for adding frags.

> > Signed-off-by: Tirthendu Sarkar <tirthendu.sarkar@xxxxxxxxx>
> > ---
> >   drivers/net/ethernet/intel/i40e/i40e_txrx.c | 54 ++++++++-------------
> >   1 file changed, 19 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> > index a7fba294a8f4..019abd7273a2 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> > @@ -2018,6 +2018,21 @@ static bool i40e_can_reuse_rx_page(struct
> i40e_rx_buffer *rx_buffer,
> >   	return true;
> >   }
> >
> > +/**
> > + * i40e_rx_buffer_flip - adjusted rx_buffer to point to an unused region
> > + * @rx_buffer: Rx buffer to adjust
> > + * @size: Size of adjustment
> > + **/
> > +static void i40e_rx_buffer_flip(struct i40e_rx_buffer *rx_buffer,
> > +				unsigned int truesize)
> > +{
> > +#if (PAGE_SIZE < 8192)
> > +	rx_buffer->page_offset ^= truesize;
> > +#else
> > +	rx_buffer->page_offset += truesize;
> > +#endif
> 
> It’d be great if you sent a patch on top, doing the check not in the
> preprocessor but in native C code.
> 

We don’t want to introduce branches. Also, note this part of the code is not
newly introduced in this patchset.

> > +}
> > +
> >   /**
> >    * i40e_add_rx_frag - Add contents of Rx buffer to sk_buff
> >    * @rx_ring: rx descriptor ring to transact packets on
> > @@ -2045,11 +2060,7 @@ static void i40e_add_rx_frag(struct i40e_ring
> *rx_ring,
> >   			rx_buffer->page_offset, size, truesize);
> >
> >   	/* page is being used so we must update the page offset */
> > -#if (PAGE_SIZE < 8192)
> > -	rx_buffer->page_offset ^= truesize;
> > -#else
> > -	rx_buffer->page_offset += truesize;
> > -#endif
> > +	i40e_rx_buffer_flip(rx_buffer, truesize);
> >   }
> >
> >   /**
> > @@ -2154,11 +2165,7 @@ static struct sk_buff *i40e_construct_skb(struct
> i40e_ring *rx_ring,
> >   				size, truesize);
> >
> >   		/* buffer is used by skb, update page_offset */
> > -#if (PAGE_SIZE < 8192)
> > -		rx_buffer->page_offset ^= truesize;
> > -#else
> > -		rx_buffer->page_offset += truesize;
> > -#endif
> > +		i40e_rx_buffer_flip(rx_buffer, truesize);
> >   	} else {
> >   		/* buffer is unused, reset bias back to rx_buffer */
> >   		rx_buffer->pagecnt_bias++;
> > @@ -2209,11 +2216,7 @@ static struct sk_buff *i40e_build_skb(struct
> i40e_ring *rx_ring,
> >   		skb_metadata_set(skb, metasize);
> >
> >   	/* buffer is used by skb, update page_offset */
> > -#if (PAGE_SIZE < 8192)
> > -	rx_buffer->page_offset ^= truesize;
> > -#else
> > -	rx_buffer->page_offset += truesize;
> > -#endif
> > +	i40e_rx_buffer_flip(rx_buffer, truesize);
> >
> >   	return skb;
> >   }
> > @@ -2326,25 +2329,6 @@ static int i40e_run_xdp(struct i40e_ring *rx_ring,
> struct xdp_buff *xdp, struct
> >   	return result;
> >   }
> >
> > -/**
> > - * i40e_rx_buffer_flip - adjusted rx_buffer to point to an unused region
> > - * @rx_ring: Rx ring
> > - * @rx_buffer: Rx buffer to adjust
> > - * @size: Size of adjustment
> > - **/
> > -static void i40e_rx_buffer_flip(struct i40e_ring *rx_ring,
> > -				struct i40e_rx_buffer *rx_buffer,
> > -				unsigned int size)
> > -{
> > -	unsigned int truesize = i40e_rx_frame_truesize(rx_ring, size);
> > -
> > -#if (PAGE_SIZE < 8192)
> > -	rx_buffer->page_offset ^= truesize;
> > -#else
> > -	rx_buffer->page_offset += truesize;
> > -#endif
> > -}
> > -
> >   /**
> >    * i40e_xdp_ring_update_tail - Updates the XDP Tx ring tail register
> >    * @xdp_ring: XDP Tx ring
> > @@ -2513,7 +2497,7 @@ static int i40e_clean_rx_irq(struct i40e_ring
> *rx_ring, int budget,
> >   		if (xdp_res) {
> >   			if (xdp_res & (I40E_XDP_TX | I40E_XDP_REDIR)) {
> >   				xdp_xmit |= xdp_res;
> > -				i40e_rx_buffer_flip(rx_ring, rx_buffer, size);
> > +				i40e_rx_buffer_flip(rx_buffer,
> xdp.frame_sz);
> 
> Why is `xdp.frame_sz` the correct size now?
> 

As explained before, earlier i40e_rx_buffer_flip() was calculating 'truesize'
internally but now depends on the caller for it to be provided. 'xdp.frame_sz'
actually is the truesize of the buffer.

> >   			} else {
> >   				rx_buffer->pagecnt_bias++;
> >   			}
> 
> 
> Kind regards,
> 
> Paul

Thanks.




[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