Patchwork b43: Increase number of RX DMA slots

login
register
mail settings
Submitter Larry Finger
Date Feb. 18, 2013, 3:01 a.m.
Message ID <1361156480-32566-1-git-send-email-Larry.Finger@lwfinger.net>
Download mbox | patch
Permalink /patch/221137/
State Accepted
Delegated to: David Miller
Headers show

Comments

Larry Finger - Feb. 18, 2013, 3:01 a.m.
Bastian Bittorf reported that some of the silent freezes on a Linksys WRT54G
were due to overflow of the RX DMA ring buffer, which was created with 64
slots. That finding reminded me that I was seeing similar crashed on a netbook,
which also has a relatively slow processor. After increasing the number of
slots to 128, runs on the netbook that previously failed now worked; however,
I found that 109 slots had been used in one test. For that reason, the number
of slots is being increased to 256.

Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
Cc: Bastian Bittorf <bittorf@bluebottle.com>
Cc: Stable <stable@vger.kernel.org>
---
---
 drivers/net/wireless/b43/dma.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Rafał Miłecki - Feb. 18, 2013, 4:18 p.m.
2013/2/18 Larry Finger <Larry.Finger@lwfinger.net>:
> Bastian Bittorf reported that some of the silent freezes on a Linksys WRT54G
> were due to overflow of the RX DMA ring buffer, which was created with 64
> slots. That finding reminded me that I was seeing similar crashed on a netbook,
> which also has a relatively slow processor. After increasing the number of
> slots to 128, runs on the netbook that previously failed now worked; however,
> I found that 109 slots had been used in one test. For that reason, the number
> of slots is being increased to 256.

So probably ideal solution is to use 128 *and* fix the driver's
failing on overflow ;)

Did you try it on some old device? Just for sure firmware&DMA will
handle it correctly.
Bastian Bittorf - Feb. 18, 2013, 4:55 p.m.
* Rafał Miłecki <zajec5@gmail.com> [18.02.2013 17:54]:
> 2013/2/18 Larry Finger <Larry.Finger@lwfinger.net>:
> 
> So probably ideal solution is to use 128 *and* fix the driver's
> failing on overflow ;)

i will run a test tomorrow an report, keep calm.

bye, bastian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Larry Finger - Feb. 18, 2013, 5:04 p.m.
On 02/18/2013 10:55 AM, Bastian Bittorf wrote:
> * Rafał Miłecki <zajec5@gmail.com> [18.02.2013 17:54]:
>> 2013/2/18 Larry Finger <Larry.Finger@lwfinger.net>:
>>
>> So probably ideal solution is to use 128 *and* fix the driver's
>> failing on overflow ;)
>
> i will run a test tomorrow an report, keep calm.

Do you have debugging turned on for b43? If so, the slot usage line at module 
unload would be useful.

Larry


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Larry Finger - Feb. 18, 2013, 8:10 p.m.
On 02/18/2013 10:18 AM, Rafał Miłecki wrote:
> 2013/2/18 Larry Finger <Larry.Finger@lwfinger.net>:
>> Bastian Bittorf reported that some of the silent freezes on a Linksys WRT54G
>> were due to overflow of the RX DMA ring buffer, which was created with 64
>> slots. That finding reminded me that I was seeing similar crashed on a netbook,
>> which also has a relatively slow processor. After increasing the number of
>> slots to 128, runs on the netbook that previously failed now worked; however,
>> I found that 109 slots had been used in one test. For that reason, the number
>> of slots is being increased to 256.
>
> So probably ideal solution is to use 128 *and* fix the driver's
> failing on overflow ;)
>
> Did you try it on some old device? Just for sure firmware&DMA will
> handle it correctly.

I tested on BCM4318 (which is pretty old), and two different BCM4312 (14e4:4315) 
units. I think the firmware and DMA can handle it. After all, all the TX rings 
have 256 slots. There is, however, a question of the memory. TX only acquires 
the buffers when needed, but RX has to get them in advance, thus 256 slots there 
will waste a lot of memory.

I agree that there be two patches, depending on Bastian's testing. The slot size 
change can be backported to stable.

Larry

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Feb. 19, 2013, 5:52 a.m.
From: Larry Finger <Larry.Finger@lwfinger.net>
Date: Sun, 17 Feb 2013 21:01:20 -0600

> Bastian Bittorf reported that some of the silent freezes on a Linksys WRT54G
> were due to overflow of the RX DMA ring buffer, which was created with 64
> slots. That finding reminded me that I was seeing similar crashed on a netbook,
> which also has a relatively slow processor. After increasing the number of
> slots to 128, runs on the netbook that previously failed now worked; however,
> I found that 109 slots had been used in one test. For that reason, the number
> of slots is being increased to 256.
> 
> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>

Applied.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Laight - Feb. 19, 2013, 9:42 a.m.
> > Bastian Bittorf reported that some of the silent freezes on a Linksys WRT54G
> > were due to overflow of the RX DMA ring buffer, which was created with 64
> > slots. That finding reminded me that I was seeing similar crashed on a netbook,
> > which also has a relatively slow processor. After increasing the number of
> > slots to 128, runs on the netbook that previously failed now worked; however,
> > I found that 109 slots had been used in one test. For that reason, the number
> > of slots is being increased to 256.

Surely the driver should work even if all the RX buffers get filled?
Increasing the number of buffers is just hiding the issue.
A burst of 300 back to back small packets probably fills the 256 slots.

I realise that dropping frames isn't ideal, and that small numbers
of buffers can make it impossible to receive long fragmented IP
messages. but increasing the number of buffers doesn't seem to
be the best fix for a 'silent freeze'.

It might be that the driver would be more robust if it only ever
put rx buffers into all but one of the ring slots.

	David



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafał Miłecki - Feb. 19, 2013, 9:57 a.m.
2013/2/19 David Laight <David.Laight@aculab.com>:
>> > Bastian Bittorf reported that some of the silent freezes on a Linksys WRT54G
>> > were due to overflow of the RX DMA ring buffer, which was created with 64
>> > slots. That finding reminded me that I was seeing similar crashed on a netbook,
>> > which also has a relatively slow processor. After increasing the number of
>> > slots to 128, runs on the netbook that previously failed now worked; however,
>> > I found that 109 slots had been used in one test. For that reason, the number
>> > of slots is being increased to 256.
>
> Surely the driver should work even if all the RX buffers get filled?
> Increasing the number of buffers is just hiding the issue.
> A burst of 300 back to back small packets probably fills the 256 slots.
>
> I realise that dropping frames isn't ideal, and that small numbers
> of buffers can make it impossible to receive long fragmented IP
> messages. but increasing the number of buffers doesn't seem to
> be the best fix for a 'silent freeze'.
>
> It might be that the driver would be more robust if it only ever
> put rx buffers into all but one of the ring slots.

That's what I said ;) I have this on my TODO, but I need to resolve my
issues with ethernet first.
Larry Finger - Feb. 19, 2013, 5:57 p.m.
On 02/19/2013 03:42 AM, David Laight wrote:
> Surely the driver should work even if all the RX buffers get filled?
> Increasing the number of buffers is just hiding the issue.
> A burst of 300 back to back small packets probably fills the 256 slots.
>
> I realise that dropping frames isn't ideal, and that small numbers
> of buffers can make it impossible to receive long fragmented IP
> messages. but increasing the number of buffers doesn't seem to
> be the best fix for a 'silent freeze'.
>
> It might be that the driver would be more robust if it only ever
> put rx buffers into all but one of the ring slots.

The real problem is that some (perhaps all) versions of the firmware, which 
manages the 'in' pointer of the FIFO ring, appears to fail to detect the ring 
full condition. That is the real cause of the freeze; however, we do not have 
access to the firmware source. We don't even have the right to redistribute it, 
which is why we have the b43-fwcutter work around.

I just reviewed about 8 months of logs on my laptop and discovered that my 2.0 
GHz dual CPU system once used 59 of 64 slots. On an netbook with an Atom running 
at 1.6 GHz, 109 slots were used. Clearly, the much slower CPU in a Linksys 
WRT54G needs more than 64, but testing to determine how many is in progress.

Current thinking is that we will change the number of slots to 128, and add code 
to the driver to detect the overflow and reset the device when it occurs. The 
increased memory usage should be manageable, most systems will never hit the 
condition, and the packet loss will be minimal for those that do.

Larry


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Feb. 19, 2013, 6:15 p.m.
From: Larry Finger <Larry.Finger@lwfinger.net>
Date: Tue, 19 Feb 2013 11:57:19 -0600

> The real problem is that some (perhaps all) versions of the firmware,
> which manages the 'in' pointer of the FIFO ring, appears to fail to
> detect the ring full condition. That is the real cause of the freeze;
> however, we do not have access to the firmware source. We don't even
> have the right to redistribute it, which is why we have the
> b43-fwcutter work around.

I understand your constraints, but this is a trivially remotely
DoS'able condition even on slow CPU atom laptops.

Send an "expansive" full sized frame followed by 300 or so 64-byte UDP
packets --> instant hang.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Larry Finger - Feb. 19, 2013, 6:28 p.m.
On 02/19/2013 12:15 PM, David Miller wrote:

> I understand your constraints, but this is a trivially remotely
> DoS'able condition even on slow CPU atom laptops.
>
> Send an "expansive" full sized frame followed by 300 or so 64-byte UDP
> packets --> instant hang.

Thanks for the suggestion for a test. I think the reset solution should survive 
with only some packet loss, but we will find out.

Larry


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bastian Bittorf - Feb. 19, 2013, 8:01 p.m.
* Larry Finger <Larry.Finger@lwfinger.net> [18.02.2013 21:17]:

> (14e4:4315) units. I think the firmware and DMA can handle it. After
> all, all the TX rings have 256 slots. There is, however, a question
> of the memory. TX only acquires the buffers when needed, but RX has
> to get them in advance, thus 256 slots there will waste a lot of
> memory.

I did some testing with 256 slots and the
"b43-workaround-rx-fifo-overflow.patch" on a very slow board,
an i'am sure, that 128 slots are not enough. kernel-log is here:

http://intercity-vpn.de/files/openwrt/b43test.dmesg.txt

this was a normal download (100mb @ 1 megabyte/s), but if you
do some udp-magic (netperf) you run soon into trouble.

thanks for your work!

bye, bastian / wireless.subsignal.org / weimarnetz e.V.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Julian Calaby - Feb. 20, 2013, 12:47 a.m.
Hi Larry,

On Wed, Feb 20, 2013 at 5:28 AM, Larry Finger <Larry.Finger@lwfinger.net> wrote:
> On 02/19/2013 12:15 PM, David Miller wrote:
>
>> I understand your constraints, but this is a trivially remotely
>> DoS'able condition even on slow CPU atom laptops.
>>
>> Send an "expansive" full sized frame followed by 300 or so 64-byte UDP
>> packets --> instant hang.
>
>
> Thanks for the suggestion for a test. I think the reset solution should
> survive with only some packet loss, but we will find out.

Is it be possible to increase the number of slots at runtime? Maybe an
even better solution would be to keep the existing number of slots,
and if they run out, reset and increase incrementally to some sensible
maximum value.

Thanks,
Larry Finger - Feb. 20, 2013, 2:42 a.m.
On 02/19/2013 06:47 PM, Julian Calaby wrote:
>
> Is it be possible to increase the number of slots at runtime? Maybe an
> even better solution would be to keep the existing number of slots,
> and if they run out, reset and increase incrementally to some sensible
> maximum value.

The number could be increased a bit, but on systems with 32-bit DMA such as the 
BCM4318, the maximum size of the ring buffer is 4KB. Even more importantly, each 
slot is allocated an skb of 2390 bytes. Even at 256 slots, the memory allocation 
is pretty large.

Larry

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
=?ISO-8859-1?Q?Stefanik_G=E1bor?= - Feb. 20, 2013, 6:26 a.m.
On Wed, Feb 20, 2013 at 3:42 AM, Larry Finger <Larry.Finger@lwfinger.net> wrote:
> On 02/19/2013 06:47 PM, Julian Calaby wrote:
>>
>>
>> Is it be possible to increase the number of slots at runtime? Maybe an
>> even better solution would be to keep the existing number of slots,
>> and if they run out, reset and increase incrementally to some sensible
>> maximum value.
>
>
> The number could be increased a bit, but on systems with 32-bit DMA such as
> the BCM4318, the maximum size of the ring buffer is 4KB. Even more
> importantly, each slot is allocated an skb of 2390 bytes. Even at 256 slots,
> the memory allocation is pretty large.
>
> Larry
>

Is this an issue that vendor drivers are also vulnerable to? If it is
a firmware issue, I would certainly think so.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Larry Finger - Feb. 20, 2013, 7:15 a.m.
On 02/20/2013 12:26 AM, Gábor Stefanik wrote:
>
> Is this an issue that vendor drivers are also vulnerable to? If it is
> a firmware issue, I would certainly think so.

I also think so, at least if they are using the firmware version that Bastian is 
using. His logs don't have that info in them, but I certainly saw the problem on 
my BCM4312 using firmware 508.154 from 2009.

Larry



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bastian Bittorf - Feb. 20, 2013, 8:15 a.m.
* Larry Finger <Larry.Finger@lwfinger.net> [20.02.2013 08:32]:
> On 02/20/2013 12:26 AM, Gábor Stefanik wrote:
> >
> >Is this an issue that vendor drivers are also vulnerable to? If it is
> >a firmware issue, I would certainly think so.
> 
> I also think so, at least if they are using the firmware version that
> Bastian is using. His logs don't have that info in them, but I
> certainly saw the problem on my BCM4312 using firmware 508.154 from
> 2009.
 
Another test this morning with heavy downloading (but tcp only)
show slot usage auf max 204/256. we are using firmware

"version 666.2 (2011-02-23 01:15:07)" which is OpenWrt's default
for b43. see here the full logs, including minstrel output and dmesg:

http://intercity-vpn.de/files/openwrt/b43test2.dmesg.txt

if a slot needs ~2500 bytes, so 256 slot are only 640kb which seems
ok to me. ofcourse it raises the memory consumption by 500kb, but now
the router is useful 8-)

thank you! bye, bastian

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

Patch

diff --git a/drivers/net/wireless/b43/dma.h b/drivers/net/wireless/b43/dma.h
index 315b96e..9fdd198 100644
--- a/drivers/net/wireless/b43/dma.h
+++ b/drivers/net/wireless/b43/dma.h
@@ -169,7 +169,7 @@  struct b43_dmadesc_generic {
 
 /* DMA engine tuning knobs */
 #define B43_TXRING_SLOTS		256
-#define B43_RXRING_SLOTS		64
+#define B43_RXRING_SLOTS		256
 #define B43_DMA0_RX_FW598_BUFSIZE	(B43_DMA0_RX_FW598_FO + IEEE80211_MAX_FRAME_LEN)
 #define B43_DMA0_RX_FW351_BUFSIZE	(B43_DMA0_RX_FW351_FO + IEEE80211_MAX_FRAME_LEN)