| Message ID | 20230215124305.76075-5-tirthendu.sarkar@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | i40e: support XDP multi-buffer | expand |
On 2/15/2023 4:43 AM, Tirthendu Sarkar wrote: > 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. > > 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. > > Signed-off-by: Tirthendu Sarkar <tirthendu.sarkar@intel.com> > --- > 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 s/size/truesize > + **/ > +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 > +} > + > /** > * i40e_add_rx_frag - Add contents of Rx buffer to sk_buff > * @rx_ring: rx descriptor ring to transact packets on
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? > 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? > Signed-off-by: Tirthendu Sarkar <tirthendu.sarkar@intel.com> > --- > 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. > +} > + > /** > * 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? > } else { > rx_buffer->pagecnt_bias++; > } Kind regards, Paul
> -----Original Message----- > From: Paul Menzel <pmenzel@molgen.mpg.de> > 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@intel.com> > > --- > > 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.
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 +} + /** * 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); } else { rx_buffer->pagecnt_bias++; }
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. 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. Signed-off-by: Tirthendu Sarkar <tirthendu.sarkar@intel.com> --- drivers/net/ethernet/intel/i40e/i40e_txrx.c | 54 ++++++++------------- 1 file changed, 19 insertions(+), 35 deletions(-)