netdev - Re: [Intel-wired-lan] [PATCH intel-next v4 4/8] i40e: Change size to truesize when using i40e_rx_buffer

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: <10c0dcb4-f353-41a8-dfff-e99d2dca7fb2@molgen.mpg.de>
Date:   Thu, 16 Feb 2023 08:49:34 +0100
From:   Paul Menzel <pmenzel@...gen.mpg.de>
To:     Tirthendu Sarkar <tirthendu.sarkar@...el.com>
Cc:     intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
        jesse.brandeburg@...el.com, anthony.l.nguyen@...el.com,
        bpf@...r.kernel.org, magnus.karlsson@...el.com
Subject: Re: [Intel-wired-lan] [PATCH intel-next v4 4/8] i40e: Change size to
 truesize when using i40e_rx_buffer_flip()

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