netdev - Re: [PATCH net 1/3] i40e: avoid premature Rx buffer reuse

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: <256ab09e-1cea-c8ab-9589-b0c5809bdea7@intel.com>
Date:   Tue, 25 Aug 2020 13:25:16 +0200
From:   Björn Töpel <bjorn.topel@...el.com>
To:     Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        Björn Töpel <bjorn.topel@...il.com>
Cc:     jeffrey.t.kirsher@...el.com, intel-wired-lan@...ts.osuosl.org,
        magnus.karlsson@...el.com, magnus.karlsson@...il.com,
        netdev@...r.kernel.org, piotr.raczynski@...el.com,
        maciej.machnikowski@...el.com, lirongqing@...du.com
Subject: Re: [PATCH net 1/3] i40e: avoid premature Rx buffer reuse

On 2020-08-25 13:13, Maciej Fijalkowski wrote:
> On Tue, Aug 25, 2020 at 11:16:27AM +0200, Björn Töpel wrote:
[...]
>>   	struct i40e_rx_buffer *rx_buffer;
>>   
>>   	rx_buffer = i40e_rx_bi(rx_ring, rx_ring->next_to_clean);
>> +	*rx_buffer_pgcnt = i40e_rx_buffer_page_count(rx_buffer);
> 
> What i previously meant was:
> 
> #if (PAGE_SIZE < 8192)
> 	*rx_buffer_pgcnt = page_count(rx_buffer->page);
> #endif
> 
> and see below
> 

Right...

>>   	prefetchw(rx_buffer->page);
>>   
>>   	/* we are reusing so sync this buffer for CPU use */
>> @@ -2112,9 +2124,10 @@ static struct sk_buff *i40e_build_skb(struct i40e_ring *rx_ring,
>>    * either recycle the buffer or unmap it and free the associated resources.
>>    */
>>   static void i40e_put_rx_buffer(struct i40e_ring *rx_ring,
>> -			       struct i40e_rx_buffer *rx_buffer)
>> +			       struct i40e_rx_buffer *rx_buffer,
>> +			       int rx_buffer_pgcnt)
>>   {
>> -	if (i40e_can_reuse_rx_page(rx_buffer)) {
>> +	if (i40e_can_reuse_rx_page(rx_buffer, rx_buffer_pgcnt)) {
>>   		/* hand second half of page back to the ring */
>>   		i40e_reuse_rx_page(rx_ring, rx_buffer);
>>   	} else {
>> @@ -2319,6 +2332,7 @@ static int i40e_clean_rx_irq(struct i40e_ring *rx_ring, int budget)
>>   	unsigned int xdp_xmit = 0;
>>   	bool failure = false;
>>   	struct xdp_buff xdp;
>> +	int rx_buffer_pgcnt;
> 
> you could move scope this variable only for the
> 
> while (likely(total_rx_packets < (unsigned int)budget))
> 
> loop and init this to 0. then you could drop the helper function you've
> added. and BTW the page_count is not being used for big pages but i agree
> that it's better to have it set to 0.
>

...but isn't it a bit nasty with an output parameter that relies on the 
that the input was set to zero. I guess it's a matter of taste, but I 
find that more error prone.

Let me know if you have strong feelings about this, and I'll respin (but 
I rather not!).


Björn


>>   
>>   #if (PAGE_SIZE < 8192)
>>   	xdp.frame_sz = i40e_rx_frame_truesize(rx_ring, 0);
>> @@ -2370,7 +2384,7 @@ static int i40e_clean_rx_irq(struct i40e_ring *rx_ring, int budget)
>>   			break;
>>   
>>   		i40e_trace(clean_rx_irq, rx_ring, rx_desc, skb);
>> -		rx_buffer = i40e_get_rx_buffer(rx_ring, size);
>> +		rx_buffer = i40e_get_rx_buffer(rx_ring, size, &rx_buffer_pgcnt);
>>   
>>   		/* retrieve a buffer from the ring */
>>   		if (!skb) {
>> @@ -2413,7 +2427,7 @@ static int i40e_clean_rx_irq(struct i40e_ring *rx_ring, int budget)
>>   			break;
>>   		}
>>   
>> -		i40e_put_rx_buffer(rx_ring, rx_buffer);
>> +		i40e_put_rx_buffer(rx_ring, rx_buffer, rx_buffer_pgcnt);
>>   		cleaned_count++;
>>   
>>   		if (i40e_is_non_eop(rx_ring, rx_desc, skb))
>> -- 
>> 2.25.1
>>