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