netdev - Re: r8169 : always copying the rx buffer to new skb

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: <4DAFA778.9000705@hotmail.com>
Date:	Wed, 20 Apr 2011 23:41:44 -0400
From:	John Lumby <johnlumby@...mail.com>
To:	Francois Romieu <romieu@...zoreil.com>
CC:	netdev@...r.kernel.org, Ben Hutchings <bhutchings@...arflare.com>,
	nic_swsd@...ltek.com
Subject: Re: r8169 :  always copying the rx buffer to new skb

On 04/20/11 15:13, Francois Romieu wrote:
> John Lumby<johnlumby@...mail.com>  :
> [...]
>> I've  verified that [...] and everything works just
>> fine,
> Did your testing account for some memory pressure ?

No,  something I need to do.     In my tests paging rate was light.     
I will try a squeeze test to see what happens.

>> So do we really need to be that concerned about occasional
>> allocation failure?
> See $search_engine +r8169 high order memory allocation failure.

The bug reports I see are all related to the problem that occurred at 
open in the days when the init_ring() requested GFP_ATOMIC *and* 
insisted that *all* 256 buffers must be obtained or else failed the open.
I take your point,  but I think it is different in the interrupt case  
-   the rx_interrupt does not consider a single alloc failure to trigger 
any visible failure.   In the old code (and my patched),  it just tries 
again next time.   In the current code,  it can't do that since it has 
no skb to pass up,  so it drops the packet.

Have I missed some other bug reports on alloc failures?

But I also realize that (I guess) few sysadmins may have ever set the 
rx_copybreak down low in the days when the parameter was there,   so 
maybe we just don't really know how many problems would arise with 
it.     So I would propose leaving it to default to 16383 as before.

> Why don't you send the patch through the mailing list ?
>
> (hint, hint)

In my next post.   Still on 2.6.39-rc2 and too late for me to try merge 
to latest right now,  hope still useful.

>>
>> I'm just not sure I
>> see why that has to imply the always_copy.
>>
>>
>> Because of high-order memory allocation failure under memory pressure and
>> memory wastage.

I think memory allocation failure can occur regardless of code design  
-  old,  current and my patched code all do the same amount of dynamic 
alloc'ing of skbs.   It's just the initial set in the ring which is 
different.   In the current code,  alloc failure => we drop packets.    
Maybe dropping many packets might cause more trouble owing to 
re-transmits  than not replenishing the rx buffers temporarily.    
Although I guess the NIC stays up with the current code.   Not really sure.

>>
>>   Btw several 816x have limited jumbo frames abilities.

Is that point that there is some special significance for memory 
allocation?   Not sure about that  -  I mean the total data rate per sec 
is limited to 1Gbit nominal in each direction regardless of size of data 
buffers  (I think).     So naiively jumbo will not put more memory 
pressure than 1500 will it?    Or are you thinking about testing?

John Lumby
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html