LinuxLists.cc - Re: b44: regression in 2.6.22 (resend)

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

On Monday 28 May 2007 16:09:46 Maximilian Engelhardt wrote:
> On Monday 28 May 2007, Michael Buesch wrote:
> > Can you give 2.6.16 a try? The diff is not that big and we might
> > be able to find out what broke if you find out 2.6.16 works.
> > You can also try later kernels like .17, .18, .19 to further
> > reduce the patch. (You could also git-bisect, if you have the time).
> >
> I did some testing and compiled some kernels and here are the results:
>
> I was able to find out what causes the problems for me. I did build two
> 2.6.21.3 kernels, and one does work fine and the other doesn't.
>
> This is a diff of the kernel configs I used:
>
> --- /usr/src/linux-2.6.21.3-oldconfig1/.config 2007-05-28 13:41:15.000000000
> +0200
> +++ /usr/src/linux-2.6.21.3/.config 2007-05-28 14:46:08.000000000 +0200
> @@ -1,7 +1,7 @@
> #
> # Automatically generated make config: don't edit
> # Linux kernel version: 2.6.21.3
> -# Mon May 28 13:41:15 2007
> +# Mon May 28 14:46:09 2007
> #
> CONFIG_X86_32=y
> CONFIG_GENERIC_TIME=y
> @@ -32,7 +32,7 @@
> #
> # General setup
> #
> -CONFIG_LOCALVERSION="-oldconfig1"
> +CONFIG_LOCALVERSION=""
> CONFIG_LOCALVERSION_AUTO=y
> CONFIG_SWAP=y
> CONFIG_SYSVIPC=y
> @@ -108,9 +108,9 @@
> #
> # Processor type and features
> #
> -# CONFIG_TICK_ONESHOT is not set
> +CONFIG_TICK_ONESHOT=y
> # CONFIG_NO_HZ is not set
> -# CONFIG_HIGH_RES_TIMERS is not set
> +CONFIG_HIGH_RES_TIMERS=y
> # CONFIG_SMP is not set
> CONFIG_X86_PC=y
> # CONFIG_X86_ELAN is not set
>
> The -oldconfig1 is the kernel that had no problems and the other shows the b44
> problem. So if High Resolution Timer Support is disabled everything works
> fine and if I enable it the problems do appear again.
>
> I didn't test this on my 2.6.22-rc3 kernel yet, but I guess disabling High
> Resolution Timer Support will also solve the problem there.
>
> The older kernels I tried also work perfectly fine and they didn't have the
> High Resolution Timer Support yet.

So, that's interesting, indeed.
Any idea what's going on, someone? Thomas?

--
Greetings Michael.

2007-05-28 14:10:35

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

On Monday 28 May 2007, Michael Buesch wrote:
> Can you give 2.6.16 a try? The diff is not that big and we might
> be able to find out what broke if you find out 2.6.16 works.
> You can also try later kernels like .17, .18, .19 to further
> reduce the patch. (You could also git-bisect, if you have the time).
>
I did some testing and compiled some kernels and here are the results:

I was able to find out what causes the problems for me. I did build two
2.6.21.3 kernels, and one does work fine and the other doesn't.

This is a diff of the kernel configs I used:

--- /usr/src/linux-2.6.21.3-oldconfig1/.config 2007-05-28 13:41:15.000000000
+0200
+++ /usr/src/linux-2.6.21.3/.config 2007-05-28 14:46:08.000000000 +0200
@@ -1,7 +1,7 @@
#
# Automatically generated make config: don't edit
# Linux kernel version: 2.6.21.3
-# Mon May 28 13:41:15 2007
+# Mon May 28 14:46:09 2007
#
CONFIG_X86_32=y
CONFIG_GENERIC_TIME=y
@@ -32,7 +32,7 @@
#
# General setup
#
-CONFIG_LOCALVERSION="-oldconfig1"
+CONFIG_LOCALVERSION=""
CONFIG_LOCALVERSION_AUTO=y
CONFIG_SWAP=y
CONFIG_SYSVIPC=y
@@ -108,9 +108,9 @@
#
# Processor type and features
#
-# CONFIG_TICK_ONESHOT is not set
+CONFIG_TICK_ONESHOT=y
# CONFIG_NO_HZ is not set
-# CONFIG_HIGH_RES_TIMERS is not set
+CONFIG_HIGH_RES_TIMERS=y
# CONFIG_SMP is not set
CONFIG_X86_PC=y
# CONFIG_X86_ELAN is not set

The -oldconfig1 is the kernel that had no problems and the other shows the b44
problem. So if High Resolution Timer Support is disabled everything works
fine and if I enable it the problems do appear again.

I didn't test this on my 2.6.22-rc3 kernel yet, but I guess disabling High
Resolution Timer Support will also solve the problem there.

The older kernels I tried also work perfectly fine and they didn't have the
High Resolution Timer Support yet.

Maxi

Attachments:

(No filename) (1.79 kB)
signature.asc (189.00 B)
This is a digitally signed message part. Download all attachments

2007-05-29 15:21:28

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

On Mon, 2007-05-28 at 13:55 -0700, Maximilian Engelhardt wrote:
> On Monday 28 May 2007, Thomas Gleixner wrote:
> > On Mon, 2007-05-28 at 19:44 +0200, Maximilian Engelhardt wrote:
> > > > Can you please keep CONFIG_HIGH_RES_TIMERS and CONFIG_NOHZ and try the
> > > > following combinations on the kernel command line:
> > > >
> > > > 1) highres=off nohz=off (should be the same as your working config)
> > > > 2) highres=off
> > > > 3) nohz=off
> > >
> > > I tested this with my 2.6.22-rc3 kernel, here are the results:
> > >
> > > without any special boot parameters: problem does appear
> > > highres=off nohz=off: problem does not appear
> > > highres=off: problem does not appear
> > > nohz=off: problem does appear
> >
> > Is there any other strange behavior of the high res enabled kernel than
> > the b44 problem ?
>
> I didn't notice anything.
>
> >
> > > I additionally built my 2.6.22-rc2-mm1 kernel without High Resolution
> > > Timer, but the high ping problem is still there.
> >
> > Hmm, that's mysterious. Wild guess is that highres exposes the hidden
> > "feature" in a different way than rc2-mm1 does.
>
> I think the bug in 2.6.21/22-rc3 is a different one that the one in
> 2.6.22-rc2-mm1, but that's also only a wild guess :)
>
> I'll explain this a bit:
> In 2.6.21/22-rc3 is the same b44 driver that has been in the stock kernels for
> some time. With this driver and High Resolution Timer turned on I get
> problems using iperf. The problems are that the systems becomes really slow
> and unresponsive. Michael Buesch thought this could be an IRQ storm which
> sounds logical to me. This bug did never happen to me before I startet the
> iperf test.

Can you please check to see if you notice anything out of the ordinary
using netperf in place of iperf in your high res timer on/off testbed?

Thanks,
Gary

2007-05-29 22:59:28

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

On Tue, 2007-05-29 at 18:39 -0400, Jeff Garzik wrote:

> We check for 0xffffffff because that is often how a fault is indicated,
> when the memory location is read during or immediately after hotplug (or
> if the PCI bus is truly faulty). So for most hardware, you see
>
> tmp = read(irq status)
> if (!tmp)
> return irq-none /* no irq events raised */
> if (tmp == 0xffffffff)
> return irq-none /* hot unplug or h/w fault */
>
> and the method that determines no interrupt handling is needed.
>

I guess you are right, but then shouldn't the driver be checking for
faults in other parts of the code too? What if a fault/hotplug occurs
immediately after an interrupt, but before a tx?
Thanks,
Gary

2007-05-27 21:46:39

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

On Sunday 27 May 2007, Michael Buesch wrote:
> On Sunday 27 May 2007 22:36:39 Maximilian Engelhardt wrote:
> > When I ran 2.6.21.1 or 2.6.22-rc3 without any debugging tools just in
> > normal use I didn't notice any problems. It did work fine as I would
> > expect it. I think the wget and ping tests here are as they should be.
> >
> > With 2.6.22-rc2-mm1 I noticed that connections seem to be slower. The
> > ping test does confirm this, because here response times are very high.
> > As far as I can remember the wget download rate was a bit slower than
> > 2.6.21.1 or 2.6.22-rc3 till it stalled.
> > I would expect it to be someting like the other two kernels. The two
> > problems I see are the high ping times and the fact that the card stopped
> > working.
> >
> > I don't know why the iperf results are so different from my personal
> > experience. I guess the fact that I get so bad results with 2.6.21.1 and
> > 2.6.22-rc3 is that iperf does something that causes the system to be
> > extremely slow and thus degrading performance. This could be a bug
> > somewhere in the b44 driver of 2.6.21.1 and 2.6.22-RC3 that has
> > unintended been fixed by the ssb switch, but that's only a roughly guess.
>
> Ok. I guess (Yes I do :D) that there is an IRQ storm or something like
> that, because you say that your system is becoming very slow and
> unresponsive. It sounds like an IRQ is not ACKed correctly and so keeps
> triggering and stalling the system. I'll take a look at a few diffs...
> Do you see significant differences in the "hi" and/or "si" times in top?
> Do you see a significant difference in the /proc/interrupts count. For
> example that the kernel that works worse generates 10 times the IRQ count
> for the same amount of data.

ok, here are the results:

Using 2.6.22-rc3 I get lot's of hi during TX and lots of hi and si during RX.
Using 2.6.22-rc3-mm1 hi and si are significantly lower.
It's difficult to give absolute numbers, because top refreshes very slow, but
with 2.6.22-rc3 hi is about 30% during TX and RX and si is 0% during TX and
50% during RX. With Using 2.6.22-rc3-mm1 hi is 0% during TX and 0.3% during
RX and si is 10% during TX and 0% during RX.

When I do the same test on both kernels I get about 10 times (yes, it's really
about ten times like in your example) more interrupts with 2.6.22-rc3 than
with 2.6.22-rc3-mm1.

An additional thing I noticed it that it's not the BCM4401 card that stops
working but my e100 card. If I take the e100 card down and up again the
connection is working again, so the BCM4401 doesn't have a "stops working"
bug for me.

Maxi

Attachments:

(No filename) (2.55 kB)
signature.asc (189.00 B)
This is a digitally signed message part. Download all attachments

2007-05-28 19:23:52

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

On Mon, 2007-05-28 at 19:44 +0200, Maximilian Engelhardt wrote:
> > Can you please keep CONFIG_HIGH_RES_TIMERS and CONFIG_NOHZ and try the
> > following combinations on the kernel command line:
> >
> > 1) highres=off nohz=off (should be the same as your working config)
> > 2) highres=off
> > 3) nohz=off
>
> I tested this with my 2.6.22-rc3 kernel, here are the results:
>
> without any special boot parameters: problem does appear
> highres=off nohz=off: problem does not appear
> highres=off: problem does not appear
> nohz=off: problem does appear

Is there any other strange behavior of the high res enabled kernel than
the b44 problem ?

> I additionally built my 2.6.22-rc2-mm1 kernel without High Resolution Timer,
> but the high ping problem is still there.

Hmm, that's mysterious. Wild guess is that highres exposes the hidden
"feature" in a different way than rc2-mm1 does.

tglx

2007-05-28 15:32:54

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

On Mon, 2007-05-28 at 17:14 +0200, Michael Buesch wrote:
> > The -oldconfig1 is the kernel that had no problems and the other shows the b44
> > problem. So if High Resolution Timer Support is disabled everything works
> > fine and if I enable it the problems do appear again.
> >
> > I didn't test this on my 2.6.22-rc3 kernel yet, but I guess disabling High
> > Resolution Timer Support will also solve the problem there.
> >
> > The older kernels I tried also work perfectly fine and they didn't have the
> > High Resolution Timer Support yet.
>
> So, that's interesting, indeed.
> Any idea what's going on, someone? Thomas?

Not off the top of my head.

Maximilian, does the kernel work otherwise (I mean aside of the b44
driver) ?

Can you please keep CONFIG_HIGH_RES_TIMERS and CONFIG_NOHZ and try the
following combinations on the kernel command line:

1) highres=off nohz=off (should be the same as your working config)
2) highres=off
3) nohz=off

Michael, is anything in the b44 driver timer driven ?

Thanks,

tglx

2007-05-28 21:45:55

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

On Mon, 2007-05-28 at 22:55 +0200, Maximilian Engelhardt wrote:
> > > I additionally built my 2.6.22-rc2-mm1 kernel without High Resolution
> > > Timer, but the high ping problem is still there.
> >
> > Hmm, that's mysterious. Wild guess is that highres exposes the hidden
> > "feature" in a different way than rc2-mm1 does.
>
> I think the bug in 2.6.21/22-rc3 is a different one that the one in
> 2.6.22-rc2-mm1, but that's also only a wild guess :)
>
> I'll explain this a bit:
> In 2.6.21/22-rc3 is the same b44 driver that has been in the stock kernels for
> some time. With this driver and High Resolution Timer turned on I get
> problems using iperf. The problems are that the systems becomes really slow
> and unresponsive. Michael Buesch thought this could be an IRQ storm which
> sounds logical to me. This bug did never happen to me before I startet the
> iperf test.

Can you please apply

http://www.tglx.de/projects/hrtimers/2.6.22-rc3/patch-2.6.22-rc3-hrt1.patch

on top of rc3 and check, whether it has any effect on your problem.

> The other issue happens only with 2.6.22-rc2-mm1 which includes the b44 ssb
> spilt. It's independed wether High Resolution Timer is turned on or off I
> always get very varying and high ping times. The iperf-test doesn't show the
> problems from 2.6.21/22-rc3.

Neither with nor without highres ?

tglx

2007-05-28 14:56:13

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

On Monday 28 May 2007 16:12:12 Maximilian Engelhardt wrote:
> On Monday 28 May 2007, Michael Buesch wrote:
> > Can you also test the following patch?
> > I think there's a bug in b44 that is doesn't properly discard
> > shared IRQs, so it might possibly generate a NAPI storm, dunno.
> > Worth a try.
> >
> > Index: linux-2.6.22-rc3/drivers/net/b44.c
> > ===================================================================
> > --- linux-2.6.22-rc3.orig/drivers/net/b44.c 2007-05-27 23:01:44.000000000
> > +0200 +++ linux-2.6.22-rc3/drivers/net/b44.c 2007-05-28 12:48:27.000000000
> > +0200 @@ -911,6 +911,8 @@ static irqreturn_t b44_interrupt(int irq
> > spin_lock(&bp->lock);
> >
> > istat = br32(bp, B44_ISTAT);
> > + if (istat == 0xFFFFFFFF)
> > + goto out; /* Shared IRQ not for us */
> > imask = br32(bp, B44_IMASK);
> >
> > /* The interrupt mask register controls which interrupt bits
> > @@ -942,6 +944,7 @@ irq_ack:
> > bw32(bp, B44_ISTAT, istat);
> > br32(bp, B44_ISTAT);
> > }
> > +out:
> > spin_unlock(&bp->lock);
> > return IRQ_RETVAL(handled);
> > }
>
> I did try this patch on a affected kernel, but I didn't notice any big
> difference. Perhaps the kernel is a bit less slow during the test, but It's
> hard to tell.

Ok, but anyway. I think this is a bug and needs to be fixed this way. Gary?

--
Greetings Michael.

2007-05-29 21:17:50

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

I am busy bisecting the real cause. Unfortunately, oprofile doesn't work
on the laptop, and build time sucks...

This how I think the IRQ should work:

--- a/drivers/net/b44.c 2007-05-29 09:47:53.000000000 -0700
+++ b/drivers/net/b44.c 2007-05-29 09:49:50.000000000 -0700
@@ -908,9 +908,11 @@ static irqreturn_t b44_interrupt(int irq
u32 istat, imask;
int handled = 0;

- spin_lock(&bp->lock);
-
istat = br32(bp, B44_ISTAT);
+ if (istat == 0 || istat == ~0)
+ return IRQ_NONE;
+
+ spin_lock(&bp->lock);
imask = br32(bp, B44_IMASK);

/* The interrupt mask register controls which interrupt bits

2007-05-29 20:46:17

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

On Tuesday 29 May 2007 16:14:35 Gary Zambrano wrote:
> On Mon, 2007-05-28 at 16:55 +0200, Michael Buesch wrote:
> > On Monday 28 May 2007 16:12:12 Maximilian Engelhardt wrote:
> > > On Monday 28 May 2007, Michael Buesch wrote:
> > > > Can you also test the following patch?
> > > > I think there's a bug in b44 that is doesn't properly discard
> > > > shared IRQs, so it might possibly generate a NAPI storm, dunno.
> > > > Worth a try.
> > > >
> > > > Index: linux-2.6.22-rc3/drivers/net/b44.c
> > > > ===================================================================
> > > > --- linux-2.6.22-rc3.orig/drivers/net/b44.c 2007-05-27 23:01:44.000000000
> > > > +0200 +++ linux-2.6.22-rc3/drivers/net/b44.c 2007-05-28 12:48:27.000000000
> > > > +0200 @@ -911,6 +911,8 @@ static irqreturn_t b44_interrupt(int irq
> > > > spin_lock(&bp->lock);
> > > >
> > > > istat = br32(bp, B44_ISTAT);
> > > > + if (istat == 0xFFFFFFFF)
> > > > + goto out; /* Shared IRQ not for us */
> > > > imask = br32(bp, B44_IMASK);
> > > >
> > > > /* The interrupt mask register controls which interrupt bits
> > > > @@ -942,6 +944,7 @@ irq_ack:
> > > > bw32(bp, B44_ISTAT, istat);
> > > > br32(bp, B44_ISTAT);
> > > > }
> > > > +out:
> > > > spin_unlock(&bp->lock);
> > > > return IRQ_RETVAL(handled);
> > > > }
> > >
> > > I did try this patch on a affected kernel, but I didn't notice any big
> > > difference. Perhaps the kernel is a bit less slow during the test, but It's
> > > hard to tell.
> >
> > Ok, but anyway. I think this is a bug and needs to be fixed this way. Gary?
> >
>
> Thanks Michael.
> No, I don't think this is a bug and it does not need to be fixed.

Are you sure? I'm not so sure, because
1) On bcm43xx the reverse engineers told us that the card
returns 0xFFFFFFFF for no-irq-pending. Since b44 and bcm43xx
are very similiar in IRQ and DMA I just thought it would
be the case there, too. Just guessing.
2) PCMCIA cards usually return all-ones if you try to read a
register of a card that's been removed. So it's good
practice to check for this and bail out early in the IRQ
path. Do PCMCIA cards (PC-card, not neccessarily a real
16bit PCMCIA card) for b44 exist?

--
Greetings Michael.

2007-05-27 20:36:52

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

On Sunday 27 May 2007, Michael Buesch wrote:
> On Sunday 27 May 2007 21:25:17 Maximilian Engelhardt wrote:
> > 2.6.22-rc3:
> >
> > [ 5] local 192.168.1.2 port 46557 connected with 192.168.1.1 port 5001
> > [ 5] 0.0-60.4 sec 58.9 MBytes 8.18 Mbits/sec
> > [ 4] local 192.168.1.2 port 5001 connected with 192.168.1.1 port 51633
> > [ 4] 0.0-63.1 sec 7.27 MBytes 967 Kbits/sec
>
> Why do we have two different measurements here? Is one TX and one RX?
> Which one?

Yes, the first is TX (BCM4401 --> e100) and the second is RX. Both are tcp
connections. I think iperf does display the ip addresses wrong in the second
connection, but that's another issue.

>
> > koala:~# ping -c10 192.168.1.1
> > PING 192.168.1.1 (192.168.1.1) 56(84) bytes of data.
> > 64 bytes from 192.168.1.1: icmp_seq=1 ttl=64 time=0.243 ms
> > 64 bytes from 192.168.1.1: icmp_seq=2 ttl=64 time=0.234 ms
> > 64 bytes from 192.168.1.1: icmp_seq=3 ttl=64 time=0.238 ms
> > 64 bytes from 192.168.1.1: icmp_seq=4 ttl=64 time=0.235 ms
> > 64 bytes from 192.168.1.1: icmp_seq=5 ttl=64 time=0.230 ms
> > 64 bytes from 192.168.1.1: icmp_seq=6 ttl=64 time=0.317 ms
> > 64 bytes from 192.168.1.1: icmp_seq=7 ttl=64 time=0.232 ms
> > 64 bytes from 192.168.1.1: icmp_seq=8 ttl=64 time=0.232 ms
> > 64 bytes from 192.168.1.1: icmp_seq=9 ttl=64 time=0.228 ms
> > 64 bytes from 192.168.1.1: icmp_seq=10 ttl=64 time=0.238 ms
> >
> > --- 192.168.1.1 ping statistics ---
> > 10 packets transmitted, 10 received, 0% packet loss, time 8997ms
> > rtt min/avg/max/mdev = 0.228/0.242/0.317/0.031 ms
> >
> > System responsiveness was the same as with 2.6.21.1.
> >
> > wget got 11.23M/s, again same as 2.6.21.1.
> >
> >
> > 2.6.22-rc2-mm1:
> >
> > [ 5] local 192.168.1.2 port 42198 connected with 192.168.1.1 port 5001
> > [ 5] 0.0-60.1 sec 402 MBytes 56.1 Mbits/sec
> > [ 4] local 192.168.1.2 port 5001 connected with 192.168.1.1 port 48598
> > [ 4] 0.0-63.0 sec 177 MBytes 23.6 Mbits/sec
>
> So with -mm (with ssb) you actually get better performace
> then with plain 2.6.22-rc3?
>
> Can you elaborate a bit more about what you get an what you expect
> on which kernel?

When I ran 2.6.21.1 or 2.6.22-rc3 without any debugging tools just in normal
use I didn't notice any problems. It did work fine as I would expect it.
I think the wget and ping tests here are as they should be.

With 2.6.22-rc2-mm1 I noticed that connections seem to be slower. The ping
test does confirm this, because here response times are very high. As far as
I can remember the wget download rate was a bit slower than 2.6.21.1 or
2.6.22-rc3 till it stalled.
I would expect it to be someting like the other two kernels. The two problems
I see are the high ping times and the fact that the card stopped working.

I don't know why the iperf results are so different from my personal
experience. I guess the fact that I get so bad results with 2.6.21.1 and
2.6.22-rc3 is that iperf does something that causes the system to be
extremely slow and thus degrading performance. This could be a bug somewhere
in the b44 driver of 2.6.21.1 and 2.6.22-RC3 that has unintended been fixed
by the ssb switch, but that's only a roughly guess.

Maxi

Attachments:

(No filename) (3.12 kB)
signature.asc (189.00 B)
This is a digitally signed message part. Download all attachments

2007-05-28 10:51:31

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

Can you also test the following patch?
I think there's a bug in b44 that is doesn't properly discard
shared IRQs, so it might possibly generate a NAPI storm, dunno.
Worth a try.

Index: linux-2.6.22-rc3/drivers/net/b44.c
===================================================================
--- linux-2.6.22-rc3.orig/drivers/net/b44.c 2007-05-27 23:01:44.000000000 +0200
+++ linux-2.6.22-rc3/drivers/net/b44.c 2007-05-28 12:48:27.000000000 +0200
@@ -911,6 +911,8 @@ static irqreturn_t b44_interrupt(int irq
spin_lock(&bp->lock);

istat = br32(bp, B44_ISTAT);
+ if (istat == 0xFFFFFFFF)
+ goto out; /* Shared IRQ not for us */
imask = br32(bp, B44_IMASK);

/* The interrupt mask register controls which interrupt bits
@@ -942,6 +944,7 @@ irq_ack:
bw32(bp, B44_ISTAT, istat);
br32(bp, B44_ISTAT);
}
+out:
spin_unlock(&bp->lock);
return IRQ_RETVAL(handled);
}

--
Greetings Michael.

2007-05-28 20:56:44

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

On Monday 28 May 2007, Thomas Gleixner wrote:
> On Mon, 2007-05-28 at 19:44 +0200, Maximilian Engelhardt wrote:
> > > Can you please keep CONFIG_HIGH_RES_TIMERS and CONFIG_NOHZ and try the
> > > following combinations on the kernel command line:
> > >
> > > 1) highres=off nohz=off (should be the same as your working config)
> > > 2) highres=off
> > > 3) nohz=off
> >
> > I tested this with my 2.6.22-rc3 kernel, here are the results:
> >
> > without any special boot parameters: problem does appear
> > highres=off nohz=off: problem does not appear
> > highres=off: problem does not appear
> > nohz=off: problem does appear
>
> Is there any other strange behavior of the high res enabled kernel than
> the b44 problem ?

I didn't notice anything.

>
> > I additionally built my 2.6.22-rc2-mm1 kernel without High Resolution
> > Timer, but the high ping problem is still there.
>
> Hmm, that's mysterious. Wild guess is that highres exposes the hidden
> "feature" in a different way than rc2-mm1 does.

I think the bug in 2.6.21/22-rc3 is a different one that the one in
2.6.22-rc2-mm1, but that's also only a wild guess :)

I'll explain this a bit:
In 2.6.21/22-rc3 is the same b44 driver that has been in the stock kernels for
some time. With this driver and High Resolution Timer turned on I get
problems using iperf. The problems are that the systems becomes really slow
and unresponsive. Michael Buesch thought this could be an IRQ storm which
sounds logical to me. This bug did never happen to me before I startet the
iperf test.

The other issue happens only with 2.6.22-rc2-mm1 which includes the b44 ssb
spilt. It's independed wether High Resolution Timer is turned on or off I
always get very varying and high ping times. The iperf-test doesn't show the
problems from 2.6.21/22-rc3.

Maxi

Attachments:

(No filename) (1.77 kB)
signature.asc (189.00 B)
This is a digitally signed message part. Download all attachments

2007-05-27 20:47:34

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

On Sunday 27 May 2007 22:36:39 Maximilian Engelhardt wrote:
> When I ran 2.6.21.1 or 2.6.22-rc3 without any debugging tools just in normal
> use I didn't notice any problems. It did work fine as I would expect it.
> I think the wget and ping tests here are as they should be.
>
> With 2.6.22-rc2-mm1 I noticed that connections seem to be slower. The ping
> test does confirm this, because here response times are very high. As far as
> I can remember the wget download rate was a bit slower than 2.6.21.1 or
> 2.6.22-rc3 till it stalled.
> I would expect it to be someting like the other two kernels. The two problems
> I see are the high ping times and the fact that the card stopped working.
>
> I don't know why the iperf results are so different from my personal
> experience. I guess the fact that I get so bad results with 2.6.21.1 and
> 2.6.22-rc3 is that iperf does something that causes the system to be
> extremely slow and thus degrading performance. This could be a bug somewhere
> in the b44 driver of 2.6.21.1 and 2.6.22-RC3 that has unintended been fixed
> by the ssb switch, but that's only a roughly guess.

Ok. I guess (Yes I do :D) that there is an IRQ storm or something like that,
because you say that your system is becoming very slow and unresponsive.
It sounds like an IRQ is not ACKed correctly and so keeps triggering and
stalling the system. I'll take a look at a few diffs...
Do you see significant differences in the "hi" and/or "si" times in top?
Do you see a significant difference in the /proc/interrupts count. For
example that the kernel that works worse generates 10 times the IRQ count
for the same amount of data.

--
Greetings Michael.

2007-05-27 19:45:42

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

On Sunday 27 May 2007 21:25:17 Maximilian Engelhardt wrote:
> 2.6.22-rc3:
>
> [ 5] local 192.168.1.2 port 46557 connected with 192.168.1.1 port 5001
> [ 5] 0.0-60.4 sec 58.9 MBytes 8.18 Mbits/sec
> [ 4] local 192.168.1.2 port 5001 connected with 192.168.1.1 port 51633
> [ 4] 0.0-63.1 sec 7.27 MBytes 967 Kbits/sec

Why do we have two different measurements here? Is one TX and one RX?
Which one?

> koala:~# ping -c10 192.168.1.1
> PING 192.168.1.1 (192.168.1.1) 56(84) bytes of data.
> 64 bytes from 192.168.1.1: icmp_seq=1 ttl=64 time=0.243 ms
> 64 bytes from 192.168.1.1: icmp_seq=2 ttl=64 time=0.234 ms
> 64 bytes from 192.168.1.1: icmp_seq=3 ttl=64 time=0.238 ms
> 64 bytes from 192.168.1.1: icmp_seq=4 ttl=64 time=0.235 ms
> 64 bytes from 192.168.1.1: icmp_seq=5 ttl=64 time=0.230 ms
> 64 bytes from 192.168.1.1: icmp_seq=6 ttl=64 time=0.317 ms
> 64 bytes from 192.168.1.1: icmp_seq=7 ttl=64 time=0.232 ms
> 64 bytes from 192.168.1.1: icmp_seq=8 ttl=64 time=0.232 ms
> 64 bytes from 192.168.1.1: icmp_seq=9 ttl=64 time=0.228 ms
> 64 bytes from 192.168.1.1: icmp_seq=10 ttl=64 time=0.238 ms
>
> --- 192.168.1.1 ping statistics ---
> 10 packets transmitted, 10 received, 0% packet loss, time 8997ms
> rtt min/avg/max/mdev = 0.228/0.242/0.317/0.031 ms
>
> System responsiveness was the same as with 2.6.21.1.
>
> wget got 11.23M/s, again same as 2.6.21.1.
>
>
> 2.6.22-rc2-mm1:
>
> [ 5] local 192.168.1.2 port 42198 connected with 192.168.1.1 port 5001
> [ 5] 0.0-60.1 sec 402 MBytes 56.1 Mbits/sec
> [ 4] local 192.168.1.2 port 5001 connected with 192.168.1.1 port 48598
> [ 4] 0.0-63.0 sec 177 MBytes 23.6 Mbits/sec

So with -mm (with ssb) you actually get better performace
then with plain 2.6.22-rc3?

Can you elaborate a bit more about what you get an what you expect
on which kernel?

--
Greetings Michael.

2007-05-28 15:44:38

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

On Monday 28 May 2007 17:32:51 Thomas Gleixner wrote:
> On Mon, 2007-05-28 at 17:14 +0200, Michael Buesch wrote:
> > > The -oldconfig1 is the kernel that had no problems and the other shows the b44
> > > problem. So if High Resolution Timer Support is disabled everything works
> > > fine and if I enable it the problems do appear again.
> > >
> > > I didn't test this on my 2.6.22-rc3 kernel yet, but I guess disabling High
> > > Resolution Timer Support will also solve the problem there.
> > >
> > > The older kernels I tried also work perfectly fine and they didn't have the
> > > High Resolution Timer Support yet.
> >
> > So, that's interesting, indeed.
> > Any idea what's going on, someone? Thomas?
>
> Not off the top of my head.
>
> Maximilian, does the kernel work otherwise (I mean aside of the b44
> driver) ?
>
> Can you please keep CONFIG_HIGH_RES_TIMERS and CONFIG_NOHZ and try the
> following combinations on the kernel command line:
>
> 1) highres=off nohz=off (should be the same as your working config)
> 2) highres=off
> 3) nohz=off
>
> Michael, is anything in the b44 driver timer driven ?

NAPI perhaps. I don't know. Steve may know.

The only timer in b44 is to update stats every second. I doubt
that this can cause such an issue. It's not involved in transmission.

--
Greetings Michael.

2007-05-29 22:28:28

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

On Tue, 2007-05-29 at 22:45 +0200, Michael Buesch wrote:
> On Tuesday 29 May 2007 16:14:35 Gary Zambrano wrote:
> > On Mon, 2007-05-28 at 16:55 +0200, Michael Buesch wrote:
> > > On Monday 28 May 2007 16:12:12 Maximilian Engelhardt wrote:
> > > > On Monday 28 May 2007, Michael Buesch wrote:
> > > > > Can you also test the following patch?
> > > > > I think there's a bug in b44 that is doesn't properly discard
> > > > > shared IRQs, so it might possibly generate a NAPI storm, dunno.
> > > > > Worth a try.
> > > > >
> > > > > Index: linux-2.6.22-rc3/drivers/net/b44.c
> > > > > ===================================================================
> > > > > --- linux-2.6.22-rc3.orig/drivers/net/b44.c 2007-05-27 23:01:44.000000000
> > > > > +0200 +++ linux-2.6.22-rc3/drivers/net/b44.c 2007-05-28 12:48:27.000000000
> > > > > +0200 @@ -911,6 +911,8 @@ static irqreturn_t b44_interrupt(int irq
> > > > > spin_lock(&bp->lock);
> > > > >
> > > > > istat = br32(bp, B44_ISTAT);
> > > > > + if (istat == 0xFFFFFFFF)
> > > > > + goto out; /* Shared IRQ not for us */
> > > > > imask = br32(bp, B44_IMASK);
> > > > >
> > > > > /* The interrupt mask register controls which interrupt bits
> > > > > @@ -942,6 +944,7 @@ irq_ack:
> > > > > bw32(bp, B44_ISTAT, istat);
> > > > > br32(bp, B44_ISTAT);
> > > > > }
> > > > > +out:
> > > > > spin_unlock(&bp->lock);
> > > > > return IRQ_RETVAL(handled);
> > > > > }
> > > >
> > > > I did try this patch on a affected kernel, but I didn't notice any big
> > > > difference. Perhaps the kernel is a bit less slow during the test, but It's
> > > > hard to tell.
> > >
> > > Ok, but anyway. I think this is a bug and needs to be fixed this way. Gary?
> > >
> >
> > Thanks Michael.
> > No, I don't think this is a bug and it does not need to be fixed.
>
> Are you sure? I'm not so sure, because
> 1) On bcm43xx the reverse engineers told us that the card
> returns 0xFFFFFFFF for no-irq-pending. Since b44 and bcm43xx
> are very similiar in IRQ and DMA I just thought it would
> be the case there, too. Just guessing.
The b44 interrupt status reg returns a value of 0 if no interrupts are
pending. The b44 uses a mask to determine which bits (events) can
generate device interrupts on the system. If the masked interrupt status
register bits are not asserted, then the b44 will return to the system
with handled = 0.
So, I think the way the b44 interrupt code is written should be ok and
not a bug.

> 2) PCMCIA cards usually return all-ones if you try to read a
> register of a card that's been removed. So it's good
> practice to check for this and bail out early in the IRQ
> path. Do PCMCIA cards (PC-card, not neccessarily a real
> 16bit PCMCIA card) for b44 exist?

I do not know of any pccard application of the b44. As far as I know
b44s live on motherboards and in the wireless soc.

Thanks,
Gary

2007-05-28 14:12:20

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

On Monday 28 May 2007, Michael Buesch wrote:
> Can you also test the following patch?
> I think there's a bug in b44 that is doesn't properly discard
> shared IRQs, so it might possibly generate a NAPI storm, dunno.
> Worth a try.
>
> Index: linux-2.6.22-rc3/drivers/net/b44.c
> ===================================================================
> --- linux-2.6.22-rc3.orig/drivers/net/b44.c 2007-05-27 23:01:44.000000000
> +0200 +++ linux-2.6.22-rc3/drivers/net/b44.c 2007-05-28 12:48:27.000000000
> +0200 @@ -911,6 +911,8 @@ static irqreturn_t b44_interrupt(int irq
> spin_lock(&bp->lock);
>
> istat = br32(bp, B44_ISTAT);
> + if (istat == 0xFFFFFFFF)
> + goto out; /* Shared IRQ not for us */
> imask = br32(bp, B44_IMASK);
>
> /* The interrupt mask register controls which interrupt bits
> @@ -942,6 +944,7 @@ irq_ack:
> bw32(bp, B44_ISTAT, istat);
> br32(bp, B44_ISTAT);
> }
> +out:
> spin_unlock(&bp->lock);
> return IRQ_RETVAL(handled);
> }

I did try this patch on a affected kernel, but I didn't notice any big
difference. Perhaps the kernel is a bit less slow during the test, but It's
hard to tell.

Maxi

Attachments:

(No filename) (1.11 kB)
signature.asc (189.00 B)
This is a digitally signed message part. Download all attachments

2007-05-29 15:37:16

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

On Mon, 2007-05-28 at 16:55 +0200, Michael Buesch wrote:
> On Monday 28 May 2007 16:12:12 Maximilian Engelhardt wrote:
> > On Monday 28 May 2007, Michael Buesch wrote:
> > > Can you also test the following patch?
> > > I think there's a bug in b44 that is doesn't properly discard
> > > shared IRQs, so it might possibly generate a NAPI storm, dunno.
> > > Worth a try.
> > >
> > > Index: linux-2.6.22-rc3/drivers/net/b44.c
> > > ===================================================================
> > > --- linux-2.6.22-rc3.orig/drivers/net/b44.c 2007-05-27 23:01:44.000000000
> > > +0200 +++ linux-2.6.22-rc3/drivers/net/b44.c 2007-05-28 12:48:27.000000000
> > > +0200 @@ -911,6 +911,8 @@ static irqreturn_t b44_interrupt(int irq
> > > spin_lock(&bp->lock);
> > >
> > > istat = br32(bp, B44_ISTAT);
> > > + if (istat == 0xFFFFFFFF)
> > > + goto out; /* Shared IRQ not for us */
> > > imask = br32(bp, B44_IMASK);
> > >
> > > /* The interrupt mask register controls which interrupt bits
> > > @@ -942,6 +944,7 @@ irq_ack:
> > > bw32(bp, B44_ISTAT, istat);
> > > br32(bp, B44_ISTAT);
> > > }
> > > +out:
> > > spin_unlock(&bp->lock);
> > > return IRQ_RETVAL(handled);
> > > }
> >
> > I did try this patch on a affected kernel, but I didn't notice any big
> > difference. Perhaps the kernel is a bit less slow during the test, but It's
> > hard to tell.
>
> Ok, but anyway. I think this is a bug and needs to be fixed this way. Gary?
>

Thanks Michael.
No, I don't think this is a bug and it does not need to be fixed.
Thanks,
Gary

2007-05-30 10:46:25

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

On Tuesday 29 May 2007 23:36:51 Gary Zambrano wrote:
> On Tue, 2007-05-29 at 18:39 -0400, Jeff Garzik wrote:
>
> > We check for 0xffffffff because that is often how a fault is indicated,
> > when the memory location is read during or immediately after hotplug (or
> > if the PCI bus is truly faulty). So for most hardware, you see
> >
> > tmp = read(irq status)
> > if (!tmp)
> > return irq-none /* no irq events raised */
> > if (tmp == 0xffffffff)
> > return irq-none /* hot unplug or h/w fault */
> >
> > and the method that determines no interrupt handling is needed.
> >
>
> I guess you are right, but then shouldn't the driver be checking for
> faults in other parts of the code too? What if a fault/hotplug occurs
> immediately after an interrupt, but before a tx?
> Thanks,

Well, in general it's not desired (or even possible) to check every
single MMIO access. General practice is to check on entering the IRQ handler
and on values from registers or DMA which could crash the driver.
For example on DMA we get some cookie back from the device in the TX
status report that says which packet this was associated to.
That value is used to look up a table.
In bcm43xx I initialize that to 0 in the driver, which is not a valid value.
Later I check for that. So if the device is unplugged before DMA was
on that value was complete it will recognize it and it won't crash.

In general we can only do our very best to prevent bad sideeffects from
a pull-in-full-operation. We can't do much here anyway. Best we can do
is to _try_ to prevent a crash. It might not always be 100% possible
to do so, though.

--
Greetings Michael.

2007-05-27 21:14:29

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

On Sunday 27 May 2007 21:25:17 Maximilian Engelhardt wrote:
> 2.6.21.1:
> [ 5] local 192.168.1.2 port 58414 connected with 192.168.1.1 port 5001
> [ 5] 0.0-60.6 sec 1.13 MBytes 157 Kbits/sec
> [ 4] local 192.168.1.2 port 5001 connected with 192.168.1.1 port 57837
> [ 4] 0.0-63.1 sec 2.82 MBytes 375 Kbits/sec

> 2.6.22-rc3:
> [ 5] local 192.168.1.2 port 46557 connected with 192.168.1.1 port 5001
> [ 5] 0.0-60.4 sec 58.9 MBytes 8.18 Mbits/sec
> [ 4] local 192.168.1.2 port 5001 connected with 192.168.1.1 port 51633
> [ 4] 0.0-63.1 sec 7.27 MBytes 967 Kbits/sec

This is the diff between these two kernels.
I'm not sure why you see a much better TX throughput here.

Can you re-check to make sure it's not just some test-jitter?

--- linux-2.6.21.1/drivers/net/b44.c 2007-05-27 22:58:01.000000000 +0200
+++ linux-2.6.22-rc3/drivers/net/b44.c 2007-05-27 23:01:44.000000000 +0200
@@ -825,12 +825,11 @@
if (copy_skb == NULL)
goto drop_it_no_recycle;

- copy_skb->dev = bp->dev;
skb_reserve(copy_skb, 2);
skb_put(copy_skb, len);
/* DMA sync done above, copy just the actual packet */
- memcpy(copy_skb->data, skb->data+bp->rx_offset, len);
-
+ skb_copy_from_linear_data_offset(skb, bp->rx_offset,
+ copy_skb->data, len);
skb = copy_skb;
}
skb->ip_summed = CHECKSUM_NONE;
@@ -1007,7 +1006,8 @@
goto err_out;
}

- memcpy(skb_put(bounce_skb, len), skb->data, skb->len);
+ skb_copy_from_linear_data(skb, skb_put(bounce_skb, len),
+ skb->len);
dev_kfree_skb_any(skb);
skb = bounce_skb;
}

--
Greetings Michael.

2007-05-28 00:25:21

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

Ok, another question: On which CPU architecture are you?

--
Greetings Michael.

2007-05-27 21:50:54

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

On Sunday 27 May 2007, Michael Buesch wrote:
> On Sunday 27 May 2007 23:13:32 Michael Buesch wrote:
> > On Sunday 27 May 2007 21:25:17 Maximilian Engelhardt wrote:
> > > 2.6.21.1:
> > > [ 5] local 192.168.1.2 port 58414 connected with 192.168.1.1 port 5001
> > > [ 5] 0.0-60.6 sec 1.13 MBytes 157 Kbits/sec
> > > [ 4] local 192.168.1.2 port 5001 connected with 192.168.1.1 port 57837
> > > [ 4] 0.0-63.1 sec 2.82 MBytes 375 Kbits/sec
> > >
> > > 2.6.22-rc3:
> > > [ 5] local 192.168.1.2 port 46557 connected with 192.168.1.1 port 5001
> > > [ 5] 0.0-60.4 sec 58.9 MBytes 8.18 Mbits/sec
> > > [ 4] local 192.168.1.2 port 5001 connected with 192.168.1.1 port 51633
> > > [ 4] 0.0-63.1 sec 7.27 MBytes 967 Kbits/sec
> >
> > This is the diff between these two kernels.
> > I'm not sure why you see a much better TX throughput here.
> >
> > Can you re-check to make sure it's not just some test-jitter?
>
> Oh, eh, and what I forgot to ask:
> Do you know an old kernel that works perfectly well for you,
> so I can look at a diff between this one and anything >=2.6.21.1.

I don't know any, most older kernels did work fine for me, but I never user
iperf there so I guess if the bug is there also I simply didn't trigger it.
If you think it's usefull I could go back and try different kernels, but that
would take some time.
Except the iperf bug 2.6.21.1 and 2.6.22-rc3 work fine.

Maxi

Attachments:

(No filename) (1.38 kB)
signature.asc (189.00 B)
This is a digitally signed message part. Download all attachments

2007-05-29 22:39:39

by Jeff Garzik

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

Gary Zambrano wrote:
> The b44 interrupt status reg returns a value of 0 if no interrupts are
> pending. The b44 uses a mask to determine which bits (events) can
> generate device interrupts on the system. If the masked interrupt status
> register bits are not asserted, then the b44 will return to the system
> with handled = 0.
> So, I think the way the b44 interrupt code is written should be ok and
> not a bug.

This is normal.

We check for 0xffffffff because that is often how a fault is indicated,
when the memory location is read during or immediately after hotplug (or
if the PCI bus is truly faulty). So for most hardware, you see

tmp = read(irq status)
if (!tmp)
return irq-none /* no irq events raised */
if (tmp == 0xffffffff)
return irq-none /* hot unplug or h/w fault */

and the method that determines no interrupt handling is needed.

Regards,

Jeff

2007-05-29 17:24:01

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

On Tuesday 29 May 2007, Gary Zambrano wrote:
> On Mon, 2007-05-28 at 13:55 -0700, Maximilian Engelhardt wrote:
> > On Monday 28 May 2007, Thomas Gleixner wrote:
> > > On Mon, 2007-05-28 at 19:44 +0200, Maximilian Engelhardt wrote:
> > > > > Can you please keep CONFIG_HIGH_RES_TIMERS and CONFIG_NOHZ and try
> > > > > the following combinations on the kernel command line:
> > > > >
> > > > > 1) highres=off nohz=off (should be the same as your working config)
> > > > > 2) highres=off
> > > > > 3) nohz=off
> > > >
> > > > I tested this with my 2.6.22-rc3 kernel, here are the results:
> > > >
> > > > without any special boot parameters: problem does appear
> > > > highres=off nohz=off: problem does not appear
> > > > highres=off: problem does not appear
> > > > nohz=off: problem does appear
> > >
> > > Is there any other strange behavior of the high res enabled kernel than
> > > the b44 problem ?
> >
> > I didn't notice anything.
> >
> > > > I additionally built my 2.6.22-rc2-mm1 kernel without High Resolution
> > > > Timer, but the high ping problem is still there.
> > >
> > > Hmm, that's mysterious. Wild guess is that highres exposes the hidden
> > > "feature" in a different way than rc2-mm1 does.
> >
> > I think the bug in 2.6.21/22-rc3 is a different one that the one in
> > 2.6.22-rc2-mm1, but that's also only a wild guess :)
> >
> > I'll explain this a bit:
> > In 2.6.21/22-rc3 is the same b44 driver that has been in the stock
> > kernels for some time. With this driver and High Resolution Timer turned
> > on I get problems using iperf. The problems are that the systems becomes
> > really slow and unresponsive. Michael Buesch thought this could be an
> > IRQ storm which sounds logical to me. This bug did never happen to me
> > before I startet the iperf test.
>
> Can you please check to see if you notice anything out of the ordinary
> using netperf in place of iperf in your high res timer on/off testbed?

ok, here are the results, I also had a look at the cpu kernel usage.
'good' means that the kernel responsiveness during the test was as I would
expect it and I didn't notice any problems.

highres enabled:

netperf: 80%sy 15%si (good)
iperf: not really messureable (bad, problem described above)

highres disabled:

netperf: 80%sy 15%si (good)
iperf: 5%sy 30%hi 15%si (good)

for test tests I did run the following commands:
netperf -l 60 192.168.1.1
iperf -c 192.168.1.1 -r -t 60

I also tried to run iperf without any additional arguments (iperf -c
192.168.1.1) on the problematic kernel but the result is the same as the
command I wrote above.

Maxi

Attachments:

(No filename) (2.54 kB)
signature.asc (189.00 B)
This is a digitally signed message part. Download all attachments

2007-05-28 10:18:41

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

Can you give 2.6.16 a try? The diff is not that big and we might
be able to find out what broke if you find out 2.6.16 works.
You can also try later kernels like .17, .18, .19 to further
reduce the patch. (You could also git-bisect, if you have the time).

git-diff v2.6.16..v2.6.22-rc3 drivers/net/b44.c

diff --git a/drivers/net/b44.c b/drivers/net/b44.c
index c3267e4..879a2ff 100644
--- a/drivers/net/b44.c
+++ b/drivers/net/b44.c
@@ -2,6 +2,7 @@
*
* Copyright (C) 2002 David S. Miller ([email protected])
* Fixed by Pekka Pietikainen ([email protected])
+ * Copyright (C) 2006 Broadcom Corporation.
*
* Distribute under GPL.
*/
@@ -28,8 +29,8 @@ #include "b44.h"

#define DRV_MODULE_NAME "b44"
#define PFX DRV_MODULE_NAME ": "
-#define DRV_MODULE_VERSION "0.97"
-#define DRV_MODULE_RELDATE "Nov 30, 2005"
+#define DRV_MODULE_VERSION "1.01"
+#define DRV_MODULE_RELDATE "Jun 16, 2006"

#define B44_DEF_MSG_ENABLE \
(NETIF_MSG_DRV | \
@@ -58,7 +59,6 @@ #define B44_TX_RING_SIZE 512
#define B44_DEF_TX_RING_PENDING (B44_TX_RING_SIZE - 1)
#define B44_TX_RING_BYTES (sizeof(struct dma_desc) * \
B44_TX_RING_SIZE)
-#define B44_DMA_MASK 0x3fffffff

#define TX_RING_GAP(BP) \
(B44_TX_RING_SIZE - (BP)->tx_pending)
@@ -74,6 +74,15 @@ #define TX_PKT_BUF_SZ (B44_MAX_MTU + ET
/* minimum number of free TX descriptors required to wake up TX process */
#define B44_TX_WAKEUP_THRESH (B44_TX_RING_SIZE / 4)

+/* b44 internal pattern match filter info */
+#define B44_PATTERN_BASE 0x400
+#define B44_PATTERN_SIZE 0x80
+#define B44_PMASK_BASE 0x600
+#define B44_PMASK_SIZE 0x10
+#define B44_MAX_PATTERNS 16
+#define B44_ETHIPV6UDP_HLEN 62
+#define B44_ETHIPV4UDP_HLEN 42
+
static char version[] __devinitdata =
DRV_MODULE_NAME ".c:v" DRV_MODULE_VERSION " (" DRV_MODULE_RELDATE ")\n";

@@ -100,7 +109,12 @@ MODULE_DEVICE_TABLE(pci, b44_pci_tbl);

static void b44_halt(struct b44 *);
static void b44_init_rings(struct b44 *);
-static void b44_init_hw(struct b44 *);
+
+#define B44_FULL_RESET 1
+#define B44_FULL_RESET_SKIP_PHY 2
+#define B44_PARTIAL_RESET 3
+
+static void b44_init_hw(struct b44 *, int);

static int dma_desc_align_mask;
static int dma_desc_sync_size;
@@ -136,7 +150,7 @@ static inline unsigned long br32(const s
return readl(bp->regs + reg);
}

-static inline void bw32(const struct b44 *bp,
+static inline void bw32(const struct b44 *bp,
unsigned long reg, unsigned long val)
{
writel(val, bp->regs + reg);
@@ -286,13 +300,13 @@ static void __b44_cam_write(struct b44 *
val |= ((u32) data[4]) << 8;
val |= ((u32) data[5]) << 0;
bw32(bp, B44_CAM_DATA_LO, val);
- val = (CAM_DATA_HI_VALID |
+ val = (CAM_DATA_HI_VALID |
(((u32) data[0]) << 8) |
(((u32) data[1]) << 0));
bw32(bp, B44_CAM_DATA_HI, val);
bw32(bp, B44_CAM_CTRL, (CAM_CTRL_WRITE |
(index << CAM_CTRL_INDEX_SHIFT)));
- b44_wait_bit(bp, B44_CAM_CTRL, CAM_CTRL_BUSY, 100, 1);
+ b44_wait_bit(bp, B44_CAM_CTRL, CAM_CTRL_BUSY, 100, 1);
}

static inline void __b44_disable_ints(struct b44 *bp)
@@ -410,25 +424,18 @@ static void __b44_set_flow_ctrl(struct b

static void b44_set_flow_ctrl(struct b44 *bp, u32 local, u32 remote)
{
- u32 pause_enab = bp->flags & (B44_FLAG_TX_PAUSE |
- B44_FLAG_RX_PAUSE);
+ u32 pause_enab = 0;

- if (local & ADVERTISE_PAUSE_CAP) {
- if (local & ADVERTISE_PAUSE_ASYM) {
- if (remote & LPA_PAUSE_CAP)
- pause_enab |= (B44_FLAG_TX_PAUSE |
- B44_FLAG_RX_PAUSE);
- else if (remote & LPA_PAUSE_ASYM)
- pause_enab |= B44_FLAG_RX_PAUSE;
- } else {
- if (remote & LPA_PAUSE_CAP)
- pause_enab |= (B44_FLAG_TX_PAUSE |
- B44_FLAG_RX_PAUSE);
- }
- } else if (local & ADVERTISE_PAUSE_ASYM) {
- if ((remote & LPA_PAUSE_CAP) &&
- (remote & LPA_PAUSE_ASYM))
- pause_enab |= B44_FLAG_TX_PAUSE;
+ /* The driver supports only rx pause by default because
+ the b44 mac tx pause mechanism generates excessive
+ pause frames.
+ Use ethtool to turn on b44 tx pause if necessary.
+ */
+ if ((local & ADVERTISE_PAUSE_CAP) &&
+ (local & ADVERTISE_PAUSE_ASYM)){
+ if ((remote & LPA_PAUSE_ASYM) &&
+ !(remote & LPA_PAUSE_CAP))
+ pause_enab |= B44_FLAG_RX_PAUSE;
}

__b44_set_flow_ctrl(bp, pause_enab);
@@ -608,8 +615,7 @@ static void b44_tx(struct b44 *bp)
struct ring_info *rp = &bp->tx_buffers[cons];
struct sk_buff *skb = rp->skb;

- if (unlikely(skb == NULL))
- BUG();
+ BUG_ON(skb == NULL);

pci_unmap_single(bp->pdev,
pci_unmap_addr(rp, mapping),
@@ -657,9 +663,11 @@ static int b44_alloc_rx_skb(struct b44 *

/* Hardware bug work-around, the chip is unable to do PCI DMA
to/from anything above 1GB :-( */
- if (mapping + RX_PKT_BUF_SZ > B44_DMA_MASK) {
+ if (dma_mapping_error(mapping) ||
+ mapping + RX_PKT_BUF_SZ > DMA_30BIT_MASK) {
/* Sigh... */
- pci_unmap_single(bp->pdev, mapping, RX_PKT_BUF_SZ,PCI_DMA_FROMDEVICE);
+ if (!dma_mapping_error(mapping))
+ pci_unmap_single(bp->pdev, mapping, RX_PKT_BUF_SZ,PCI_DMA_FROMDEVICE);
dev_kfree_skb_any(skb);
skb = __dev_alloc_skb(RX_PKT_BUF_SZ,GFP_DMA);
if (skb == NULL)
@@ -667,8 +675,10 @@ static int b44_alloc_rx_skb(struct b44 *
mapping = pci_map_single(bp->pdev, skb->data,
RX_PKT_BUF_SZ,
PCI_DMA_FROMDEVICE);
- if (mapping + RX_PKT_BUF_SZ > B44_DMA_MASK) {
- pci_unmap_single(bp->pdev, mapping, RX_PKT_BUF_SZ,PCI_DMA_FROMDEVICE);
+ if (dma_mapping_error(mapping) ||
+ mapping + RX_PKT_BUF_SZ > DMA_30BIT_MASK) {
+ if (!dma_mapping_error(mapping))
+ pci_unmap_single(bp->pdev, mapping, RX_PKT_BUF_SZ,PCI_DMA_FROMDEVICE);
dev_kfree_skb_any(skb);
return -ENOMEM;
}
@@ -710,7 +720,7 @@ static void b44_recycle_rx(struct b44 *b
struct ring_info *src_map, *dest_map;
struct rx_header *rh;
int dest_idx;
- u32 ctrl;
+ __le32 ctrl;

dest_idx = dest_idx_unmasked & (B44_RX_RING_SIZE - 1);
dest_desc = &bp->rx_ring[dest_idx];
@@ -746,7 +756,7 @@ static void b44_recycle_rx(struct b44 *b
dest_idx * sizeof(dest_desc),
DMA_BIDIRECTIONAL);

- pci_dma_sync_single_for_device(bp->pdev, src_desc->addr,
+ pci_dma_sync_single_for_device(bp->pdev, le32_to_cpu(src_desc->addr),
RX_PKT_BUF_SZ,
PCI_DMA_FROMDEVICE);
}
@@ -772,7 +782,7 @@ static int b44_rx(struct b44 *bp, int bu
RX_PKT_BUF_SZ,
PCI_DMA_FROMDEVICE);
rh = (struct rx_header *) skb->data;
- len = cpu_to_le16(rh->len);
+ len = le16_to_cpu(rh->len);
if ((len > (RX_PKT_BUF_SZ - bp->rx_offset)) ||
(rh->flags & cpu_to_le16(RX_FLAG_ERRORS))) {
drop_it:
@@ -788,7 +798,7 @@ static int b44_rx(struct b44 *bp, int bu
do {
udelay(2);
barrier();
- len = cpu_to_le16(rh->len);
+ len = le16_to_cpu(rh->len);
} while (len == 0 && i++ < 5);
if (len == 0)
goto drop_it;
@@ -815,12 +825,11 @@ static int b44_rx(struct b44 *bp, int bu
if (copy_skb == NULL)
goto drop_it_no_recycle;

- copy_skb->dev = bp->dev;
skb_reserve(copy_skb, 2);
skb_put(copy_skb, len);
/* DMA sync done above, copy just the actual packet */
- memcpy(copy_skb->data, skb->data+bp->rx_offset, len);
-
+ skb_copy_from_linear_data_offset(skb, bp->rx_offset,
+ copy_skb->data, len);
skb = copy_skb;
}
skb->ip_summed = CHECKSUM_NONE;
@@ -873,12 +882,14 @@ static int b44_poll(struct net_device *n
}

if (bp->istat & ISTAT_ERRORS) {
- spin_lock_irq(&bp->lock);
+ unsigned long flags;
+
+ spin_lock_irqsave(&bp->lock, flags);
b44_halt(bp);
b44_init_rings(bp);
- b44_init_hw(bp);
+ b44_init_hw(bp, B44_FULL_RESET_SKIP_PHY);
netif_wake_queue(bp->dev);
- spin_unlock_irq(&bp->lock);
+ spin_unlock_irqrestore(&bp->lock, flags);
done = 1;
}

@@ -890,7 +901,7 @@ static int b44_poll(struct net_device *n
return (done ? 0 : 1);
}

-static irqreturn_t b44_interrupt(int irq, void *dev_id, struct pt_regs *regs)
+static irqreturn_t b44_interrupt(int irq, void *dev_id)
{
struct net_device *dev = dev_id;
struct b44 *bp = netdev_priv(dev);
@@ -902,8 +913,9 @@ static irqreturn_t b44_interrupt(int irq
istat = br32(bp, B44_ISTAT);
imask = br32(bp, B44_IMASK);

- /* ??? What the fuck is the purpose of the interrupt mask
- * ??? register if we have to mask it out by hand anyways?
+ /* The interrupt mask register controls which interrupt bits
+ * will actually raise an interrupt to the CPU when set by hw/firmware,
+ * but doesn't mask off the bits.
*/
istat &= imask;
if (istat) {
@@ -945,7 +957,7 @@ static void b44_tx_timeout(struct net_de

b44_halt(bp);
b44_init_rings(bp);
- b44_init_hw(bp);
+ b44_init_hw(bp, B44_FULL_RESET);

spin_unlock_irq(&bp->lock);

@@ -974,9 +986,10 @@ static int b44_start_xmit(struct sk_buff
}

mapping = pci_map_single(bp->pdev, skb->data, len, PCI_DMA_TODEVICE);
- if (mapping + len > B44_DMA_MASK) {
+ if (dma_mapping_error(mapping) || mapping + len > DMA_30BIT_MASK) {
/* Chip can't handle DMA to/from >1GB, use bounce buffer */
- pci_unmap_single(bp->pdev, mapping, len, PCI_DMA_TODEVICE);
+ if (!dma_mapping_error(mapping))
+ pci_unmap_single(bp->pdev, mapping, len, PCI_DMA_TODEVICE);

bounce_skb = __dev_alloc_skb(TX_PKT_BUF_SZ,
GFP_ATOMIC|GFP_DMA);
@@ -985,14 +998,16 @@ static int b44_start_xmit(struct sk_buff

mapping = pci_map_single(bp->pdev, bounce_skb->data,
len, PCI_DMA_TODEVICE);
- if (mapping + len > B44_DMA_MASK) {
- pci_unmap_single(bp->pdev, mapping,
+ if (dma_mapping_error(mapping) || mapping + len > DMA_30BIT_MASK) {
+ if (!dma_mapping_error(mapping))
+ pci_unmap_single(bp->pdev, mapping,
len, PCI_DMA_TODEVICE);
dev_kfree_skb_any(bounce_skb);
goto err_out;
}

- memcpy(skb_put(bounce_skb, len), skb->data, skb->len);
+ skb_copy_from_linear_data(skb, skb_put(bounce_skb, len),
+ skb->len);
dev_kfree_skb_any(skb);
skb = bounce_skb;
}
@@ -1060,11 +1075,11 @@ static int b44_change_mtu(struct net_dev
b44_halt(bp);
dev->mtu = new_mtu;
b44_init_rings(bp);
- b44_init_hw(bp);
+ b44_init_hw(bp, B44_FULL_RESET);
spin_unlock_irq(&bp->lock);

b44_enable_ints(bp);
-
+
return 0;
}

@@ -1210,7 +1225,8 @@ static int b44_alloc_consistent(struct b
DMA_TABLE_BYTES,
DMA_BIDIRECTIONAL);

- if (rx_ring_dma + size > B44_DMA_MASK) {
+ if (dma_mapping_error(rx_ring_dma) ||
+ rx_ring_dma + size > DMA_30BIT_MASK) {
kfree(rx_ring);
goto out_err;
}
@@ -1236,7 +1252,8 @@ static int b44_alloc_consistent(struct b
DMA_TABLE_BYTES,
DMA_TO_DEVICE);

- if (tx_ring_dma + size > B44_DMA_MASK) {
+ if (dma_mapping_error(tx_ring_dma) ||
+ tx_ring_dma + size > DMA_30BIT_MASK) {
kfree(tx_ring);
goto out_err;
}
@@ -1271,7 +1288,7 @@ static void b44_chip_reset(struct b44 *b
if (ssb_is_core_up(bp)) {
bw32(bp, B44_RCV_LAZY, 0);
bw32(bp, B44_ENET_CTRL, ENET_CTRL_DISABLE);
- b44_wait_bit(bp, B44_ENET_CTRL, ENET_CTRL_DISABLE, 100, 1);
+ b44_wait_bit(bp, B44_ENET_CTRL, ENET_CTRL_DISABLE, 200, 1);
bw32(bp, B44_DMATX_CTRL, 0);
bp->tx_prod = bp->tx_cons = 0;
if (br32(bp, B44_DMARX_STAT) & DMARX_STAT_EMASK) {
@@ -1339,6 +1356,9 @@ static int b44_set_mac_addr(struct net_d
if (netif_running(dev))
return -EBUSY;

+ if (!is_valid_ether_addr(addr->sa_data))
+ return -EINVAL;
+
memcpy(dev->dev_addr, addr->sa_data, dev->addr_len);

spin_lock_irq(&bp->lock);
@@ -1352,13 +1372,15 @@ static int b44_set_mac_addr(struct net_d
* packet processing. Invoked with bp->lock held.
*/
static void __b44_set_rx_mode(struct net_device *);
-static void b44_init_hw(struct b44 *bp)
+static void b44_init_hw(struct b44 *bp, int reset_kind)
{
u32 val;

b44_chip_reset(bp);
- b44_phy_reset(bp);
- b44_setup_phy(bp);
+ if (reset_kind == B44_FULL_RESET) {
+ b44_phy_reset(bp);
+ b44_setup_phy(bp);
+ }

/* Enable CRC32, set proper LED modes and power on PHY */
bw32(bp, B44_MAC_CTRL, MAC_CTRL_CRC32_ENAB | MAC_CTRL_PHY_LEDCTRL);
@@ -1372,16 +1394,21 @@ static void b44_init_hw(struct b44 *bp)
bw32(bp, B44_TXMAXLEN, bp->dev->mtu + ETH_HLEN + 8 + RX_HEADER_LEN);

bw32(bp, B44_TX_WMARK, 56); /* XXX magic */
- bw32(bp, B44_DMATX_CTRL, DMATX_CTRL_ENABLE);
- bw32(bp, B44_DMATX_ADDR, bp->tx_ring_dma + bp->dma_offset);
- bw32(bp, B44_DMARX_CTRL, (DMARX_CTRL_ENABLE |
- (bp->rx_offset << DMARX_CTRL_ROSHIFT)));
- bw32(bp, B44_DMARX_ADDR, bp->rx_ring_dma + bp->dma_offset);
+ if (reset_kind == B44_PARTIAL_RESET) {
+ bw32(bp, B44_DMARX_CTRL, (DMARX_CTRL_ENABLE |
+ (bp->rx_offset << DMARX_CTRL_ROSHIFT)));
+ } else {
+ bw32(bp, B44_DMATX_CTRL, DMATX_CTRL_ENABLE);
+ bw32(bp, B44_DMATX_ADDR, bp->tx_ring_dma + bp->dma_offset);
+ bw32(bp, B44_DMARX_CTRL, (DMARX_CTRL_ENABLE |
+ (bp->rx_offset << DMARX_CTRL_ROSHIFT)));
+ bw32(bp, B44_DMARX_ADDR, bp->rx_ring_dma + bp->dma_offset);

- bw32(bp, B44_DMARX_PTR, bp->rx_pending);
- bp->rx_prod = bp->rx_pending;
+ bw32(bp, B44_DMARX_PTR, bp->rx_pending);
+ bp->rx_prod = bp->rx_pending;

- bw32(bp, B44_MIB_CTRL, MIB_CTRL_CLR_ON_READ);
+ bw32(bp, B44_MIB_CTRL, MIB_CTRL_CLR_ON_READ);
+ }

val = br32(bp, B44_ENET_CTRL);
bw32(bp, B44_ENET_CTRL, (val | ENET_CTRL_ENABLE));
@@ -1397,11 +1424,11 @@ static int b44_open(struct net_device *d
goto out;

b44_init_rings(bp);
- b44_init_hw(bp);
+ b44_init_hw(bp, B44_FULL_RESET);

b44_check_phy(bp);

- err = request_irq(dev->irq, b44_interrupt, SA_SHIRQ, dev->name, dev);
+ err = request_irq(dev->irq, b44_interrupt, IRQF_SHARED, dev->name, dev);
if (unlikely(err < 0)) {
b44_chip_reset(bp);
b44_free_rings(bp);
@@ -1441,11 +1468,145 @@ #ifdef CONFIG_NET_POLL_CONTROLLER
static void b44_poll_controller(struct net_device *dev)
{
disable_irq(dev->irq);
- b44_interrupt(dev->irq, dev, NULL);
+ b44_interrupt(dev->irq, dev);
enable_irq(dev->irq);
}
#endif

+static void bwfilter_table(struct b44 *bp, u8 *pp, u32 bytes, u32 table_offset)
+{
+ u32 i;
+ u32 *pattern = (u32 *) pp;
+
+ for (i = 0; i < bytes; i += sizeof(u32)) {
+ bw32(bp, B44_FILT_ADDR, table_offset + i);
+ bw32(bp, B44_FILT_DATA, pattern[i / sizeof(u32)]);
+ }
+}
+
+static int b44_magic_pattern(u8 *macaddr, u8 *ppattern, u8 *pmask, int offset)
+{
+ int magicsync = 6;
+ int k, j, len = offset;
+ int ethaddr_bytes = ETH_ALEN;
+
+ memset(ppattern + offset, 0xff, magicsync);
+ for (j = 0; j < magicsync; j++)
+ set_bit(len++, (unsigned long *) pmask);
+
+ for (j = 0; j < B44_MAX_PATTERNS; j++) {
+ if ((B44_PATTERN_SIZE - len) >= ETH_ALEN)
+ ethaddr_bytes = ETH_ALEN;
+ else
+ ethaddr_bytes = B44_PATTERN_SIZE - len;
+ if (ethaddr_bytes <=0)
+ break;
+ for (k = 0; k< ethaddr_bytes; k++) {
+ ppattern[offset + magicsync +
+ (j * ETH_ALEN) + k] = macaddr[k];
+ len++;
+ set_bit(len, (unsigned long *) pmask);
+ }
+ }
+ return len - 1;
+}
+
+/* Setup magic packet patterns in the b44 WOL
+ * pattern matching filter.
+ */
+static void b44_setup_pseudo_magicp(struct b44 *bp)
+{
+
+ u32 val;
+ int plen0, plen1, plen2;
+ u8 *pwol_pattern;
+ u8 pwol_mask[B44_PMASK_SIZE];
+
+ pwol_pattern = kmalloc(B44_PATTERN_SIZE, GFP_KERNEL);
+ if (!pwol_pattern) {
+ printk(KERN_ERR PFX "Memory not available for WOL\n");
+ return;
+ }
+
+ /* Ipv4 magic packet pattern - pattern 0.*/
+ memset(pwol_pattern, 0, B44_PATTERN_SIZE);
+ memset(pwol_mask, 0, B44_PMASK_SIZE);
+ plen0 = b44_magic_pattern(bp->dev->dev_addr, pwol_pattern, pwol_mask,
+ B44_ETHIPV4UDP_HLEN);
+
+ bwfilter_table(bp, pwol_pattern, B44_PATTERN_SIZE, B44_PATTERN_BASE);
+ bwfilter_table(bp, pwol_mask, B44_PMASK_SIZE, B44_PMASK_BASE);
+
+ /* Raw ethernet II magic packet pattern - pattern 1 */
+ memset(pwol_pattern, 0, B44_PATTERN_SIZE);
+ memset(pwol_mask, 0, B44_PMASK_SIZE);
+ plen1 = b44_magic_pattern(bp->dev->dev_addr, pwol_pattern, pwol_mask,
+ ETH_HLEN);
+
+ bwfilter_table(bp, pwol_pattern, B44_PATTERN_SIZE,
+ B44_PATTERN_BASE + B44_PATTERN_SIZE);
+ bwfilter_table(bp, pwol_mask, B44_PMASK_SIZE,
+ B44_PMASK_BASE + B44_PMASK_SIZE);
+
+ /* Ipv6 magic packet pattern - pattern 2 */
+ memset(pwol_pattern, 0, B44_PATTERN_SIZE);
+ memset(pwol_mask, 0, B44_PMASK_SIZE);
+ plen2 = b44_magic_pattern(bp->dev->dev_addr, pwol_pattern, pwol_mask,
+ B44_ETHIPV6UDP_HLEN);
+
+ bwfilter_table(bp, pwol_pattern, B44_PATTERN_SIZE,
+ B44_PATTERN_BASE + B44_PATTERN_SIZE + B44_PATTERN_SIZE);
+ bwfilter_table(bp, pwol_mask, B44_PMASK_SIZE,
+ B44_PMASK_BASE + B44_PMASK_SIZE + B44_PMASK_SIZE);
+
+ kfree(pwol_pattern);
+
+ /* set these pattern's lengths: one less than each real length */
+ val = plen0 | (plen1 << 8) | (plen2 << 16) | WKUP_LEN_ENABLE_THREE;
+ bw32(bp, B44_WKUP_LEN, val);
+
+ /* enable wakeup pattern matching */
+ val = br32(bp, B44_DEVCTRL);
+ bw32(bp, B44_DEVCTRL, val | DEVCTRL_PFE);
+
+}
+
+static void b44_setup_wol(struct b44 *bp)
+{
+ u32 val;
+ u16 pmval;
+
+ bw32(bp, B44_RXCONFIG, RXCONFIG_ALLMULTI);
+
+ if (bp->flags & B44_FLAG_B0_ANDLATER) {
+
+ bw32(bp, B44_WKUP_LEN, WKUP_LEN_DISABLE);
+
+ val = bp->dev->dev_addr[2] << 24 |
+ bp->dev->dev_addr[3] << 16 |
+ bp->dev->dev_addr[4] << 8 |
+ bp->dev->dev_addr[5];
+ bw32(bp, B44_ADDR_LO, val);
+
+ val = bp->dev->dev_addr[0] << 8 |
+ bp->dev->dev_addr[1];
+ bw32(bp, B44_ADDR_HI, val);
+
+ val = br32(bp, B44_DEVCTRL);
+ bw32(bp, B44_DEVCTRL, val | DEVCTRL_MPM | DEVCTRL_PFE);
+
+ } else {
+ b44_setup_pseudo_magicp(bp);
+ }
+
+ val = br32(bp, B44_SBTMSLOW);
+ bw32(bp, B44_SBTMSLOW, val | SBTMSLOW_PE);
+
+ pci_read_config_word(bp->pdev, SSB_PMCSR, &pmval);
+ pci_write_config_word(bp->pdev, SSB_PMCSR, pmval | SSB_PE);
+
+}
+
static int b44_close(struct net_device *dev)
{
struct b44 *bp = netdev_priv(dev);
@@ -1471,6 +1632,11 @@ #endif

netif_poll_enable(dev);

+ if (bp->flags & B44_FLAG_WOL_ENABLE) {
+ b44_init_hw(bp, B44_PARTIAL_RESET);
+ b44_setup_wol(bp);
+ }
+
b44_free_consistent(bp);

return 0;
@@ -1543,18 +1709,19 @@ static void __b44_set_rx_mode(struct net
bw32(bp, B44_RXCONFIG, val);
} else {
unsigned char zero[6] = {0, 0, 0, 0, 0, 0};
- int i = 0;
+ int i = 1;

__b44_set_mac_addr(bp);

- if (dev->flags & IFF_ALLMULTI)
+ if ((dev->flags & IFF_ALLMULTI) ||
+ (dev->mc_count > B44_MCAST_TABLE_SIZE))
val |= RXCONFIG_ALLMULTI;
else
i = __b44_load_mcast(bp, dev);
-
- for (; i < 64; i++) {
- __b44_cam_write(bp, zero, i);
- }
+
+ for (; i < 64; i++)
+ __b44_cam_write(bp, zero, i);
+
bw32(bp, B44_RXCONFIG, val);
val = br32(bp, B44_CAM_CTRL);
bw32(bp, B44_CAM_CTRL, val | CAM_CTRL_ENABLE);
@@ -1616,8 +1783,6 @@ static int b44_get_settings(struct net_d
{
struct b44 *bp = netdev_priv(dev);

- if (!netif_running(dev))
- return -EAGAIN;
cmd->supported = (SUPPORTED_Autoneg);
cmd->supported |= (SUPPORTED_100baseT_Half |
SUPPORTED_100baseT_Full |
@@ -1645,6 +1810,12 @@ static int b44_get_settings(struct net_d
XCVR_INTERNAL : XCVR_EXTERNAL;
cmd->autoneg = (bp->flags & B44_FLAG_FORCE_LINK) ?
AUTONEG_DISABLE : AUTONEG_ENABLE;
+ if (cmd->autoneg == AUTONEG_ENABLE)
+ cmd->advertising |= ADVERTISED_Autoneg;
+ if (!netif_running(dev)){
+ cmd->speed = 0;
+ cmd->duplex = 0xff;
+ }
cmd->maxtxpkt = 0;
cmd->maxrxpkt = 0;
return 0;
@@ -1654,9 +1825,6 @@ static int b44_set_settings(struct net_d
{
struct b44 *bp = netdev_priv(dev);

- if (!netif_running(dev))
- return -EAGAIN;
-
/* We do not support gigabit. */
if (cmd->autoneg == AUTONEG_ENABLE) {
if (cmd->advertising &
@@ -1673,28 +1841,39 @@ static int b44_set_settings(struct net_d
spin_lock_irq(&bp->lock);

if (cmd->autoneg == AUTONEG_ENABLE) {
- bp->flags &= ~B44_FLAG_FORCE_LINK;
- bp->flags &= ~(B44_FLAG_ADV_10HALF |
+ bp->flags &= ~(B44_FLAG_FORCE_LINK |
+ B44_FLAG_100_BASE_T |
+ B44_FLAG_FULL_DUPLEX |
+ B44_FLAG_ADV_10HALF |
B44_FLAG_ADV_10FULL |
B44_FLAG_ADV_100HALF |
B44_FLAG_ADV_100FULL);
- if (cmd->advertising & ADVERTISE_10HALF)
- bp->flags |= B44_FLAG_ADV_10HALF;
- if (cmd->advertising & ADVERTISE_10FULL)
- bp->flags |= B44_FLAG_ADV_10FULL;
- if (cmd->advertising & ADVERTISE_100HALF)
- bp->flags |= B44_FLAG_ADV_100HALF;
- if (cmd->advertising & ADVERTISE_100FULL)
- bp->flags |= B44_FLAG_ADV_100FULL;
+ if (cmd->advertising == 0) {
+ bp->flags |= (B44_FLAG_ADV_10HALF |
+ B44_FLAG_ADV_10FULL |
+ B44_FLAG_ADV_100HALF |
+ B44_FLAG_ADV_100FULL);
+ } else {
+ if (cmd->advertising & ADVERTISED_10baseT_Half)
+ bp->flags |= B44_FLAG_ADV_10HALF;
+ if (cmd->advertising & ADVERTISED_10baseT_Full)
+ bp->flags |= B44_FLAG_ADV_10FULL;
+ if (cmd->advertising & ADVERTISED_100baseT_Half)
+ bp->flags |= B44_FLAG_ADV_100HALF;
+ if (cmd->advertising & ADVERTISED_100baseT_Full)
+ bp->flags |= B44_FLAG_ADV_100FULL;
+ }
} else {
bp->flags |= B44_FLAG_FORCE_LINK;
+ bp->flags &= ~(B44_FLAG_100_BASE_T | B44_FLAG_FULL_DUPLEX);
if (cmd->speed == SPEED_100)
bp->flags |= B44_FLAG_100_BASE_T;
if (cmd->duplex == DUPLEX_FULL)
bp->flags |= B44_FLAG_FULL_DUPLEX;
}

- b44_setup_phy(bp);
+ if (netif_running(dev))
+ b44_setup_phy(bp);

spin_unlock_irq(&bp->lock);

@@ -1730,12 +1909,12 @@ static int b44_set_ringparam(struct net_

b44_halt(bp);
b44_init_rings(bp);
- b44_init_hw(bp);
+ b44_init_hw(bp, B44_FULL_RESET);
netif_wake_queue(bp->dev);
spin_unlock_irq(&bp->lock);

b44_enable_ints(bp);
-
+
return 0;
}

@@ -1773,14 +1952,14 @@ static int b44_set_pauseparam(struct net
if (bp->flags & B44_FLAG_PAUSE_AUTO) {
b44_halt(bp);
b44_init_rings(bp);
- b44_init_hw(bp);
+ b44_init_hw(bp, B44_FULL_RESET);
} else {
__b44_set_flow_ctrl(bp, bp->flags);
}
spin_unlock_irq(&bp->lock);

b44_enable_ints(bp);
-
+
return 0;
}

@@ -1815,12 +1994,40 @@ static void b44_get_ethtool_stats(struct
spin_unlock_irq(&bp->lock);
}

-static struct ethtool_ops b44_ethtool_ops = {
+static void b44_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
+{
+ struct b44 *bp = netdev_priv(dev);
+
+ wol->supported = WAKE_MAGIC;
+ if (bp->flags & B44_FLAG_WOL_ENABLE)
+ wol->wolopts = WAKE_MAGIC;
+ else
+ wol->wolopts = 0;
+ memset(&wol->sopass, 0, sizeof(wol->sopass));
+}
+
+static int b44_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
+{
+ struct b44 *bp = netdev_priv(dev);
+
+ spin_lock_irq(&bp->lock);
+ if (wol->wolopts & WAKE_MAGIC)
+ bp->flags |= B44_FLAG_WOL_ENABLE;
+ else
+ bp->flags &= ~B44_FLAG_WOL_ENABLE;
+ spin_unlock_irq(&bp->lock);
+
+ return 0;
+}
+
+static const struct ethtool_ops b44_ethtool_ops = {
.get_drvinfo = b44_get_drvinfo,
.get_settings = b44_get_settings,
.set_settings = b44_set_settings,
.nway_reset = b44_nway_reset,
.get_link = ethtool_op_get_link,
+ .get_wol = b44_get_wol,
+ .set_wol = b44_set_wol,
.get_ringparam = b44_get_ringparam,
.set_ringparam = b44_set_ringparam,
.get_pauseparam = b44_get_pauseparam,
@@ -1853,10 +2060,10 @@ out:
static int b44_read_eeprom(struct b44 *bp, u8 *data)
{
long i;
- u16 *ptr = (u16 *) data;
+ __le16 *ptr = (__le16 *) data;

for (i = 0; i < 128; i += 2)
- ptr[i / 2] = readw(bp->regs + 4096 + i);
+ ptr[i / 2] = cpu_to_le16(readw(bp->regs + 4096 + i));

return 0;
}
@@ -1876,6 +2083,12 @@ static int __devinit b44_get_invariants(
bp->dev->dev_addr[3] = eeprom[80];
bp->dev->dev_addr[4] = eeprom[83];
bp->dev->dev_addr[5] = eeprom[82];
+
+ if (!is_valid_ether_addr(&bp->dev->dev_addr[0])){
+ printk(KERN_ERR PFX "Invalid MAC address found in EEPROM\n");
+ return -EINVAL;
+ }
+
memcpy(bp->dev->perm_addr, bp->dev->dev_addr, bp->dev->addr_len);

bp->phy_addr = eeprom[90] & 0x1f;
@@ -1890,9 +2103,13 @@ static int __devinit b44_get_invariants(
bp->core_unit = ssb_core_unit(bp);
bp->dma_offset = SB_PCI_DMA;

- /* XXX - really required?
+ /* XXX - really required?
bp->flags |= B44_FLAG_BUGGY_TXPTR;
*/
+
+ if (ssb_get_core_rev(bp) >= 7)
+ bp->flags |= B44_FLAG_B0_ANDLATER;
+
out:
return err;
}
@@ -1911,13 +2128,14 @@ static int __devinit b44_init_one(struct

err = pci_enable_device(pdev);
if (err) {
- printk(KERN_ERR PFX "Cannot enable PCI device, "
+ dev_err(&pdev->dev, "Cannot enable PCI device, "
"aborting.\n");
return err;
}

if (!(pci_resource_flags(pdev, 0) & IORESOURCE_MEM)) {
- printk(KERN_ERR PFX "Cannot find proper PCI device "
+ dev_err(&pdev->dev,
+ "Cannot find proper PCI device "
"base address, aborting.\n");
err = -ENODEV;
goto err_out_disable_pdev;
@@ -1925,24 +2143,22 @@ static int __devinit b44_init_one(struct

err = pci_request_regions(pdev, DRV_MODULE_NAME);
if (err) {
- printk(KERN_ERR PFX "Cannot obtain PCI resources, "
- "aborting.\n");
+ dev_err(&pdev->dev,
+ "Cannot obtain PCI resources, aborting.\n");
goto err_out_disable_pdev;
}

pci_set_master(pdev);

- err = pci_set_dma_mask(pdev, (u64) B44_DMA_MASK);
+ err = pci_set_dma_mask(pdev, (u64) DMA_30BIT_MASK);
if (err) {
- printk(KERN_ERR PFX "No usable DMA configuration, "
- "aborting.\n");
+ dev_err(&pdev->dev, "No usable DMA configuration, aborting.\n");
goto err_out_free_res;
}
-
- err = pci_set_consistent_dma_mask(pdev, (u64) B44_DMA_MASK);
+
+ err = pci_set_consistent_dma_mask(pdev, (u64) DMA_30BIT_MASK);
if (err) {
- printk(KERN_ERR PFX "No usable DMA configuration, "
- "aborting.\n");
+ dev_err(&pdev->dev, "No usable DMA configuration, aborting.\n");
goto err_out_free_res;
}

@@ -1951,7 +2167,7 @@ static int __devinit b44_init_one(struct

dev = alloc_etherdev(sizeof(*bp));
if (!dev) {
- printk(KERN_ERR PFX "Etherdev alloc failed, aborting.\n");
+ dev_err(&pdev->dev, "Etherdev alloc failed, aborting.\n");
err = -ENOMEM;
goto err_out_free_res;
}
@@ -1972,8 +2188,7 @@ static int __devinit b44_init_one(struct

bp->regs = ioremap(b44reg_base, b44reg_len);
if (bp->regs == 0UL) {
- printk(KERN_ERR PFX "Cannot map device registers, "
- "aborting.\n");
+ dev_err(&pdev->dev, "Cannot map device registers, aborting.\n");
err = -ENOMEM;
goto err_out_free_dev;
}
@@ -2003,8 +2218,8 @@ #endif

err = b44_get_invariants(bp);
if (err) {
- printk(KERN_ERR PFX "Problem fetching invariants of chip, "
- "aborting.\n");
+ dev_err(&pdev->dev,
+ "Problem fetching invariants of chip, aborting.\n");
goto err_out_iounmap;
}

@@ -2024,8 +2239,7 @@ #endif

err = register_netdev(dev);
if (err) {
- printk(KERN_ERR PFX "Cannot register net device, "
- "aborting.\n");
+ dev_err(&pdev->dev, "Cannot register net device, aborting.\n");
goto err_out_iounmap;
}

@@ -2033,6 +2247,11 @@ #endif

pci_save_state(bp->pdev);

+ /* Chip reset provides power to the b44 MAC & PCI cores, which
+ * is necessary for MAC register access.
+ */
+ b44_chip_reset(bp);
+
printk(KERN_INFO "%s: Broadcom 4400 10/100BaseT Ethernet ", dev->name);
for (i = 0; i < 6; i++)
printk("%2.2x%c", dev->dev_addr[i],
@@ -2078,16 +2297,20 @@ static int b44_suspend(struct pci_dev *p

del_timer_sync(&bp->timer);

- spin_lock_irq(&bp->lock);
+ spin_lock_irq(&bp->lock);

b44_halt(bp);
- netif_carrier_off(bp->dev);
+ netif_carrier_off(bp->dev);
netif_device_detach(bp->dev);
b44_free_rings(bp);

spin_unlock_irq(&bp->lock);

free_irq(dev->irq, dev);
+ if (bp->flags & B44_FLAG_WOL_ENABLE) {
+ b44_init_hw(bp, B44_PARTIAL_RESET);
+ b44_setup_wol(bp);
+ }
pci_disable_device(pdev);
return 0;
}
@@ -2096,21 +2319,32 @@ static int b44_resume(struct pci_dev *pd
{
struct net_device *dev = pci_get_drvdata(pdev);
struct b44 *bp = netdev_priv(dev);
+ int rc = 0;

pci_restore_state(pdev);
- pci_enable_device(pdev);
+ rc = pci_enable_device(pdev);
+ if (rc) {
+ printk(KERN_ERR PFX "%s: pci_enable_device failed\n",
+ dev->name);
+ return rc;
+ }
+
pci_set_master(pdev);

if (!netif_running(dev))
return 0;

- if (request_irq(dev->irq, b44_interrupt, SA_SHIRQ, dev->name, dev))
+ rc = request_irq(dev->irq, b44_interrupt, IRQF_SHARED, dev->name, dev);
+ if (rc) {
printk(KERN_ERR PFX "%s: request_irq failed\n", dev->name);
+ pci_disable_device(pdev);
+ return rc;
+ }

spin_lock_irq(&bp->lock);

b44_init_rings(bp);
- b44_init_hw(bp);
+ b44_init_hw(bp, B44_FULL_RESET);
netif_device_attach(bp->dev);
spin_unlock_irq(&bp->lock);

@@ -2139,7 +2373,7 @@ static int __init b44_init(void)
dma_desc_align_mask = ~(dma_desc_align_size - 1);
dma_desc_sync_size = max_t(unsigned int, dma_desc_align_size, sizeof(struct dma_desc));

- return pci_module_init(&b44_driver);
+ return pci_register_driver(&b44_driver);
}

static void __exit b44_cleanup(void)

--
Greetings Michael.

2007-05-27 22:16:24

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

On Sunday 27 May 2007, Michael Buesch wrote:
> On Sunday 27 May 2007 21:25:17 Maximilian Engelhardt wrote:
> > 2.6.21.1:
> > [ 5] local 192.168.1.2 port 58414 connected with 192.168.1.1 port 5001
> > [ 5] 0.0-60.6 sec 1.13 MBytes 157 Kbits/sec
> > [ 4] local 192.168.1.2 port 5001 connected with 192.168.1.1 port 57837
> > [ 4] 0.0-63.1 sec 2.82 MBytes 375 Kbits/sec
> >
> > 2.6.22-rc3:
> > [ 5] local 192.168.1.2 port 46557 connected with 192.168.1.1 port 5001
> > [ 5] 0.0-60.4 sec 58.9 MBytes 8.18 Mbits/sec
> > [ 4] local 192.168.1.2 port 5001 connected with 192.168.1.1 port 51633
> > [ 4] 0.0-63.1 sec 7.27 MBytes 967 Kbits/sec
>
> This is the diff between these two kernels.
> I'm not sure why you see a much better TX throughput here.
>
> Can you re-check to make sure it's not just some test-jitter?
>
2.6.21.1:

[ 5] local 192.168.1.2 port 54423 connected with 192.168.1.1 port 5001
[ 5] 0.0-60.3 sec 3.06 MBytes 426 Kbits/sec
[ 4] local 192.168.1.2 port 5001 connected with 192.168.1.1 port 41053
[ 4] 0.0-163.0 sec 130 MBytes 6.67 Mbits/sec

2.6.22-rc3:

[ 5] local 192.168.1.2 port 46002 connected with 192.168.1.1 port 5001
[ 5] 0.0-61.5 sec 84.0 MBytes 11.5 Mbits/sec
[ 4] local 192.168.1.2 port 5001 connected with 192.168.1.1 port 44379
[ 4] 0.0-93.8 sec 30.6 MBytes 2.74 Mbits/sec

For TX the iperf server reports the same values as the client (all values are
from the client) but for RX they are differen:

2.6.21.1: (iperf server log):

[ 5] local 192.168.1.1 port 5001 connected with 192.168.1.2 port 54423
[ 5] 0.0-60.5 sec 3.06 MBytes 425 Kbits/sec
[ 5] local 192.168.1.1 port 41053 connected with 192.168.1.2 port 5001
[ 5] 0.0-63.1 sec 130 MBytes 17.2 Mbits/sec

2.6.22-rc3 (iperf server log):

[ 4] local 192.168.1.1 port 5001 connected with 192.168.1.2 port 46002
[ 4] 0.0-61.6 sec 84.0 MBytes 11.5 Mbits/sec
[ 4] local 192.168.1.1 port 44379 connected with 192.168.1.2 port 5001
[ 4] 0.0-63.3 sec 30.6 MBytes 4.06 Mbits/sec

I have no idea how iperf internally works and what can cause such different
results here.

>
> --- linux-2.6.21.1/drivers/net/b44.c 2007-05-27 22:58:01.000000000 +0200
> +++ linux-2.6.22-rc3/drivers/net/b44.c 2007-05-27 23:01:44.000000000 +0200
> @@ -825,12 +825,11 @@
> if (copy_skb == NULL)
> goto drop_it_no_recycle;
>
> - copy_skb->dev = bp->dev;
> skb_reserve(copy_skb, 2);
> skb_put(copy_skb, len);
> /* DMA sync done above, copy just the actual packet
> */ - memcpy(copy_skb->data, skb->data+bp->rx_offset,
> len); -
> + skb_copy_from_linear_data_offset(skb,
> bp->rx_offset, +
> copy_skb->data, len); skb = copy_skb;
> }
> skb->ip_summed = CHECKSUM_NONE;
> @@ -1007,7 +1006,8 @@
> goto err_out;
> }
>
> - memcpy(skb_put(bounce_skb, len), skb->data, skb->len);
> + skb_copy_from_linear_data(skb, skb_put(bounce_skb, len),
> + skb->len);
> dev_kfree_skb_any(skb);
> skb = bounce_skb;
> }

Attachments:

(No filename) (3.28 kB)
signature.asc (189.00 B)
This is a digitally signed message part. Download all attachments

2007-05-29 18:29:13

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

On Monday 28 May 2007, Thomas Gleixner wrote:
> On Mon, 2007-05-28 at 22:55 +0200, Maximilian Engelhardt wrote:
> > > > I additionally built my 2.6.22-rc2-mm1 kernel without High Resolution
> > > > Timer, but the high ping problem is still there.
> > >
> > > Hmm, that's mysterious. Wild guess is that highres exposes the hidden
> > > "feature" in a different way than rc2-mm1 does.
> >
> > I think the bug in 2.6.21/22-rc3 is a different one that the one in
> > 2.6.22-rc2-mm1, but that's also only a wild guess :)
> >
> > I'll explain this a bit:
> > In 2.6.21/22-rc3 is the same b44 driver that has been in the stock
> > kernels for some time. With this driver and High Resolution Timer turned
> > on I get problems using iperf. The problems are that the systems becomes
> > really slow and unresponsive. Michael Buesch thought this could be an
> > IRQ storm which sounds logical to me. This bug did never happen to me
> > before I startet the iperf test.
>
> Can you please apply
>
> http://www.tglx.de/projects/hrtimers/2.6.22-rc3/patch-2.6.22-rc3-hrt1.patch
>
> on top of rc3 and check, whether it has any effect on your problem.
>
The patch didn't change anything.

> > The other issue happens only with 2.6.22-rc2-mm1 which includes the b44
> > ssb spilt. It's independed wether High Resolution Timer is turned on or
> > off I always get very varying and high ping times. The iperf-test doesn't
> > show the problems from 2.6.21/22-rc3.
>
> Neither with nor without highres ?

Yes, it doesn't matter if highres is turned on or off. iperf never showed the
problem from 2.6.21/22-rc3.

Maxi

Attachments:

(No filename) (1.56 kB)
signature.asc (189.00 B)
This is a digitally signed message part. Download all attachments

2007-05-28 17:44:49

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

On Monday 28 May 2007, Thomas Gleixner wrote:
> On Mon, 2007-05-28 at 17:14 +0200, Michael Buesch wrote:
> > > The -oldconfig1 is the kernel that had no problems and the other shows
> > > the b44 problem. So if High Resolution Timer Support is disabled
> > > everything works fine and if I enable it the problems do appear again.
> > >
> > > I didn't test this on my 2.6.22-rc3 kernel yet, but I guess disabling
> > > High Resolution Timer Support will also solve the problem there.
> > >
> > > The older kernels I tried also work perfectly fine and they didn't have
> > > the High Resolution Timer Support yet.
> >
> > So, that's interesting, indeed.
> > Any idea what's going on, someone? Thomas?
>
> Not off the top of my head.
>
> Maximilian, does the kernel work otherwise (I mean aside of the b44
> driver) ?
>
> Can you please keep CONFIG_HIGH_RES_TIMERS and CONFIG_NOHZ and try the
> following combinations on the kernel command line:
>
> 1) highres=off nohz=off (should be the same as your working config)
> 2) highres=off
> 3) nohz=off

I tested this with my 2.6.22-rc3 kernel, here are the results:

without any special boot parameters: problem does appear
highres=off nohz=off: problem does not appear
highres=off: problem does not appear
nohz=off: problem does appear

I additionally built my 2.6.22-rc2-mm1 kernel without High Resolution Timer,
but the high ping problem is still there.

Maxi

Attachments:

(No filename) (1.38 kB)
signature.asc (189.00 B)
This is a digitally signed message part. Download all attachments

2007-05-27 21:16:50

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

On Sunday 27 May 2007 23:13:32 Michael Buesch wrote:
> On Sunday 27 May 2007 21:25:17 Maximilian Engelhardt wrote:
> > 2.6.21.1:
> > [ 5] local 192.168.1.2 port 58414 connected with 192.168.1.1 port 5001
> > [ 5] 0.0-60.6 sec 1.13 MBytes 157 Kbits/sec
> > [ 4] local 192.168.1.2 port 5001 connected with 192.168.1.1 port 57837
> > [ 4] 0.0-63.1 sec 2.82 MBytes 375 Kbits/sec
>
> > 2.6.22-rc3:
> > [ 5] local 192.168.1.2 port 46557 connected with 192.168.1.1 port 5001
> > [ 5] 0.0-60.4 sec 58.9 MBytes 8.18 Mbits/sec
> > [ 4] local 192.168.1.2 port 5001 connected with 192.168.1.1 port 51633
> > [ 4] 0.0-63.1 sec 7.27 MBytes 967 Kbits/sec
>
> This is the diff between these two kernels.
> I'm not sure why you see a much better TX throughput here.
>
> Can you re-check to make sure it's not just some test-jitter?

Oh, eh, and what I forgot to ask:
Do you know an old kernel that works perfectly well for you,
so I can look at a diff between this one and anything >=2.6.21.1.

--
Greetings Michael.

2007-05-28 00:40:33

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

On Monday 28 May 2007, Michael Buesch wrote:
> Ok, another question: On which CPU architecture are you?

maxi@koala:~$ uname -m
i686

Maxi

Attachments:

(No filename) (139.00 B)
signature.asc (189.00 B)
This is a digitally signed message part. Download all attachments

2007-06-04 06:39:51

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

On Sun, 2007-06-03 at 18:26 +0200, Maximilian Engelhardt wrote:
> > Is there any other strange behavior of the high res enabled kernel than
> > the b44 problem ?
>
> I didn't notice anything in the past (as I wrote). But today I did some tests
> for an updated version of the p54 mac80211 wlan driver and I noticed exactly
> the same problem:
>
> when booting with highres=off everything is fine.
> But when I boot an highres enabled kernel and I do the iperf-test with the p54
> driver, my systems becomes unresponsive during the test. It seems to be
> exactly the same problem I have with the b44 driver.
> So this might not be a bug in the b44 code but a bug somewhere in the linux
> networking code.
>
> I did the test with an 2.6.22-rc3-git4 kernel and the p54 driver built
> external as module.

Can you look at iperf to figure out, whether it does some weird timer
stuff (high frequency interval timer or such) ? Either check the code or
strace it.

tglx

2007-06-04 19:38:55

by Ingo Molnar

[permalink] [raw]

Subject: Re: iperf: performance regression (was b44 driver problem?)

* Stephen Hemminger <[email protected]> wrote:

> Yes, the following patch makes iperf work better than ever. But are
> other broken applications going to have same problem. Sounds like the
> old "who runs first" fork() problems.

this is the first such app and really, and even for this app: i've been
frequently running iperf on -rt kernels for _years_ and never noticed
how buggy its 'locking' code was, and that it would under some
circumstances use up the whole CPU on high-res timers.

If this were a widespread problem then the right solution would be to
preserve the old behavior. The child-runs-first thing was an unspecified
detail and many apps grew to depend on how the kernel did it - and when
we changed it all hell broke lose. Even today, when i switch off
child-runs-first, Gnome would not start up because some of its startup
threads are racy and hang :-/

I googled for usleep(0) quickly (on google.com/codesearch and on
google.com) and it didnt seem to suggest that the problem is widespread.
(3 hits on the code-search site, all were false positives.)

so ... if this situation were even just a little bit more serious i'd
argue for working this around and implementing some API quirk. But right
now i'm leaning towards just saying that the iperf fix exists and fixes
the problem, and that we already have kernels out with the new behavior.
Maybe we should add a once-per-boot printk to flesh out any other apps?
I'd be surprised if there was more than iperf.

Ingo

2007-06-04 16:22:35

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

On Mon, 04 Jun 2007 08:39:48 +0200
Thomas Gleixner <[email protected]> wrote:

> On Sun, 2007-06-03 at 18:26 +0200, Maximilian Engelhardt wrote:
> > > Is there any other strange behavior of the high res enabled kernel than
> > > the b44 problem ?
> >
> > I didn't notice anything in the past (as I wrote). But today I did some tests
> > for an updated version of the p54 mac80211 wlan driver and I noticed exactly
> > the same problem:
> >
> > when booting with highres=off everything is fine.
> > But when I boot an highres enabled kernel and I do the iperf-test with the p54
> > driver, my systems becomes unresponsive during the test. It seems to be
> > exactly the same problem I have with the b44 driver.
> > So this might not be a bug in the b44 code but a bug somewhere in the linux
> > networking code.
> >
> > I did the test with an 2.6.22-rc3-git4 kernel and the p54 driver built
> > external as module.
>
> Can you look at iperf to figure out, whether it does some weird timer
> stuff (high frequency interval timer or such) ? Either check the code or
> strace it.
>
> tglx
>
>

It is the receiver doing a tight loop doing gettimeofday/recv calls.

sendto(-1227715616, 0xc, 3085438964, 0, {...}, 3067249832) = 0
gettimeofday({1180973726, 981615}, NULL) = 0
gettimeofday({1180973726, 981751}, NULL) = 0
futex(0x8055c64, 0x5 /* FUTEX_??? */, 1) = 1
futex(0x8055c90, FUTEX_WAKE, 1) = 0
recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 8192, 0) = 8192
gettimeofday({1180973726, 982754}, NULL) = 0
recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 8192, 0) = 8192
gettimeofday({1180973726, 983790}, NULL) = 0
recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 8192, 0) = 8192
gettimeofday({1180973726, 984355}, NULL) = 0
recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 8192, 0) = 8192
gettimeofday({1180973726, 984706}, NULL) = 0
recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 8192, 0) = 8192
gettimeofday({1180973726, 985111}, NULL) = 0
recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 8192, 0) = 8192
gettimeofday({1180973726, 985499}, NULL) = 0
recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 8192, 0) = 8192
gettimeofday({1180973726, 986088}, NULL) = 0
recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 8192, 0) = 8192
gettimeofday({1180973726, 986436}, NULL) = 0
recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 8192, 0) = 8192
gettimeofday({1180973726, 986916}, NULL) = 0
recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 8192, 0) = 8192
gettimeofday({1180973726, 987397}, NULL) = 0
recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 8192, 0) = 8192
gettimeofday({1180973726, 987872}, NULL) = 0
recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 8192, 0) = 8192
gettimeofday({1180973726, 988440}, NULL) = 0
recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 8192, 0) = 8192
gettimeofday({1180973726, 988823}, NULL) = 0
recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 8192, 0) = 8192
gettimeofday({1180973726, 989314}, NULL) = 0
recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 8192, 0) = 8192
gettimeofday({1180973726, 990029}, NULL) = 0
recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 8192, 0) = 8192
gettimeofday({1180973726, 990890}, NULL) = 0
recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 8192, 0) = 8192
gettimeofday({1180973726, 991803}, NULL) = 0
recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 8192, 0) = 8192
gettimeofday({1180973726, 992616}, NULL) = 0
recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 8192, 0) = 8192
gettimeofday({1180973726, 993105}, NULL) = 0
recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 8192, 0) = 8192
gettimeofday({1180973726, 993585}, NULL) = 0
recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 8192, 0) = 8192
gettimeofday({1180973726, 994014}, NULL) = 0
...

recv(4, "45678901234567890123456789012345"..., 8192, 0) = 1448
gettimeofday({1180973757, 172437}, NULL) = 0
recv(4, "23456789012345678901234567890123"..., 8192, 0) = 1448
gettimeofday({1180973757, 172576}, NULL) = 0
recv(4, "01234567890123456789012345678901"..., 8192, 0) = 1632
gettimeofday({1180973757, 172752}, NULL) = 0
recv(4, "", 8192, 0) = 0
gettimeofday({1180973757, 172797}, NULL) = 0
gettimeofday({1180973757, 172817}, NULL) = 0
nanosleep({0, 0}, NULL) = 0
nanosleep({0, 0}, NULL) = 0
close(4) = 0
futex(0x8055d04, 0x5 /* FUTEX_??? */, 1) = 1
futex(0x8055d30, FUTEX_WAKE, 1) = 0
_exit(0)

--
Stephen Hemminger <[email protected]>

2007-06-04 19:26:24

[permalink] [raw]

Subject: Re: iperf: performance regression (was b44 driver problem?)

On Mon, 2007-06-04 at 21:00 +0200, Thomas Gleixner wrote:
> >
> > Yes, the following patch makes iperf work better than ever.
> > But are other broken applications going to have same problem.
> > Sounds like the old "who runs first" fork() problems.
>
> Not really. The fork() "who runs first" problem is nowhere specified.
>
> usleep(0) is well defined:
>
> .... If the value of useconds is 0, then the call has no effect.
>
> So the call into the kernel has been wrong for quite a time.
>

Just for clarification: I'm not saying that we should break the (broken)
user space ABI. I'm going to work out a patch which prints out a warning
(limited number per boot) and emulating the old behavior by a call to
yield() along with an entry into (mis)feature-removal.txt.

tglx

2007-06-03 16:27:16

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

On Monday 28 May 2007, Thomas Gleixner wrote:
> On Mon, 2007-05-28 at 19:44 +0200, Maximilian Engelhardt wrote:
> > > Can you please keep CONFIG_HIGH_RES_TIMERS and CONFIG_NOHZ and try the
> > > following combinations on the kernel command line:
> > >
> > > 1) highres=off nohz=off (should be the same as your working config)
> > > 2) highres=off
> > > 3) nohz=off
> >
> > I tested this with my 2.6.22-rc3 kernel, here are the results:
> >
> > without any special boot parameters: problem does appear
> > highres=off nohz=off: problem does not appear
> > highres=off: problem does not appear
> > nohz=off: problem does appear
>
> Is there any other strange behavior of the high res enabled kernel than
> the b44 problem ?

I didn't notice anything in the past (as I wrote). But today I did some tests
for an updated version of the p54 mac80211 wlan driver and I noticed exactly
the same problem:

when booting with highres=off everything is fine.
But when I boot an highres enabled kernel and I do the iperf-test with the p54
driver, my systems becomes unresponsive during the test. It seems to be
exactly the same problem I have with the b44 driver.
So this might not be a bug in the b44 code but a bug somewhere in the linux
networking code.

I did the test with an 2.6.22-rc3-git4 kernel and the p54 driver built
external as module.

Maxi

Attachments:

(No filename) (1.32 kB)
signature.asc (189.00 B)
This is a digitally signed message part. Download all attachments

2007-06-04 17:13:52

[permalink] [raw]

Subject: iperf: performance regression (was b44 driver problem?)

On Mon, 04 Jun 2007 18:35:58 +0200
Thomas Gleixner <[email protected]> wrote:

> On Mon, 2007-06-04 at 09:09 -0700, Stephen Hemminger wrote:
> > > > I did the test with an 2.6.22-rc3-git4 kernel and the p54 driver built
> > > > external as module.
> > >
> > > Can you look at iperf to figure out, whether it does some weird timer
> > > stuff (high frequency interval timer or such) ? Either check the code or
> > > strace it.
> >
> > It is the receiver doing a tight loop doing gettimeofday/recv calls.
> >
> >
> > sendto(-1227715616, 0xc, 3085438964, 0, {...}, 3067249832) = 0
> > gettimeofday({1180973726, 981615}, NULL) = 0
> > gettimeofday({1180973726, 981751}, NULL) = 0
> > futex(0x8055c64, 0x5 /* FUTEX_??? */, 1) = 1
> > futex(0x8055c90, FUTEX_WAKE, 1) = 0
> > recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 8192, 0) = 8192
> > gettimeofday({1180973726, 982754}, NULL) = 0
> > recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 8192, 0) = 8192
> > gettimeofday({1180973726, 983790}, NULL) = 0
>
> Well, gettimeofday() is not affected by the highres code, but
>
> > nanosleep({0, 0}, NULL) = 0
> > nanosleep({0, 0}, NULL) = 0
>
> is. The nanosleep call with a relative timeout of 0 returns immediately
> with highres enabled, while it sleeps at least until the next tick
> arrives when highres is off. Are there more of those stupid sleeps in
> the code ?

GLIBC pthread_mutex does it, YES it is a problem!
Looks like the old behavior is required for ABI compatibility.

iperf server has several threads. One thread is using pthread_mutex_lock
to wait for the other thread. It looks like pthread_mutex_lock is using
nanosleep as yield().

Multi-thread strace shows how this could kill performance.
These are with old 2.6.20 kernel that doesn't have the problem:

sendto(-1210930208, 0xc, 3085438964, 0, {...}, 3084035240) = 0
socket(PF_INET, SOCK_STREAM, IPPROTO_IP) = 3
setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
bind(3, {sa_family=AF_INET, sin_port=htons(5001), sin_addr=inet_addr("0.0.0.0")}, 16) = 0
listen(3, 5) = 0
futex(0x8055c64, 0x5 /* FUTEX_??? */, 1) = 1
accept(3, {sa_family=AF_INET, sin_port=htons(49973), sin_addr=inet_addr("192.168.0.14")}, [16]) = 4
getsockname(4, {sa_family=AF_INET, sin_port=htons(5001), sin_addr=inet_addr("192.168.0.12")}, [16]) = 0
recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 24, 0) = 24
mmap2(NULL, 8392704, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0xb6528000
mprotect(0xb6528000, 4096, PROT_NONE) = 0
clone(child_stack=0xb6d284a4, flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID|CLONE_DETACHED, parent_tidptr=0xb6d28bd8, {entry_number:6, base_addr:0xb6d28b90, limit:1048575, seg_32bit:1, contents:0, read_exec_only:0, limit_in_pages:1, seg_not_present:0, useable:1}, child_tidptr=0xb6d28bd8) = 5622
accept(3, 0x8058b98, [128]) = ? ERESTARTSYS (To be restarted)

==============

sendto(-1219322912, 0xc, 3085438964, 0, {...}, 3075642536) = 0
futex(0x8055d04, 0x5 /* FUTEX_??? */, 1) = 1
futex(0x8055d30, FUTEX_WAKE, 1) = 1
futex(0x8055c64, FUTEX_WAIT, 1, NULL) = 0
futex(0x8055d04, 0x5 /* FUTEX_??? */, 1) = 1
futex(0x8055d30, FUTEX_WAKE, 1) = 1
futex(0x8055c90, FUTEX_WAKE, 1) = 0
getsockopt(3, SOL_SOCKET, SO_RCVBUF, [87380], [4]) = 0
fstat64(1, {st_mode=S_IFCHR|0620, st_rdev=makedev(136, 0), ...}) = 0
mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0xb7fb0000
write(1, "--------------------------------"..., 61) = 61
write(1, "Server listening on TCP port 500"..., 34) = 34
write(1, "TCP window size: 85.3 KByte (def"..., 38) = 38
write(1, "--------------------------------"..., 61) = 61
nanosleep({0, 0}, NULL) = 0
futex(0x8055d04, 0x5 /* FUTEX_??? */, 1) = 1
futex(0x8055d30, FUTEX_WAKE, 1) = 1
futex(0x8055c64, FUTEX_WAIT, 3, NULL) = 0
futex(0x8055c90, FUTEX_WAIT, 2, NULL) = -1 EAGAIN (Resource temporarily unavailable)
futex(0x8055d04, 0x5 /* FUTEX_??? */, 1) = 1
futex(0x8055d30, FUTEX_WAKE, 1) = 0
futex(0x8055c90, FUTEX_WAKE, 1) = 0
write(1, "[ 4] local 192.168.0.12 port 50"..., 74) = 74
nanosleep({0, 0}, NULL) = 0
nanosleep({0, 0}, NULL) = 0
nanosleep({0, 0}, NULL) = 0
nanosleep({0, 0}, NULL) = 0
nanosleep({0, 0}, NULL) = 0
nanosleep({0, 0}, NULL) = 0
nanosleep({0, 0}, NULL) = 0
nanosleep({0, 0}, NULL) = 0

...

nanosleep({0, 0}, NULL) = 0
nanosleep({0, 0}, NULL) = 0
nanosleep({0, 0}, NULL) = 0
nanosleep({0, 0}, NULL) = 0
nanosleep({0, 0}, NULL) = 0
nanosleep({0, 0}, NULL) = 0
write(1, "[ 4] 0.0-30.2 sec 336 MByte"..., 50) = 50
nanosleep({0, 0}, NULL) = 0
nanosleep({0, 0}, NULL) = 0
futex(0x8055d04, 0x5 /* FUTEX_??? */, 1) = 1
futex(0x8055d30, FUTEX_WAKE, 1) = 0
futex(0x8055c64, FUTEX_WAIT, 5, NULL) = -1 EINTR (Interrupted system call)

===============

sendto(-1227715616, 0xc, 3085438964, 0, {...}, 3067249832) = 0
gettimeofday({1180973726, 981615}, NULL) = 0
gettimeofday({1180973726, 981751}, NULL) = 0
futex(0x8055c64, 0x5 /* FUTEX_??? */, 1) = 1
futex(0x8055c90, FUTEX_WAKE, 1) = 0
recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 8192, 0) = 8192
gettimeofday({1180973726, 982754}, NULL) = 0
recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 8192, 0) = 8192
gettimeofday({1180973726, 983790}, NULL) = 0
recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 8192, 0) = 8192
gettimeofday({1180973726, 984355}, NULL) = 0
recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 8192, 0) = 8192

...

gettimeofday({1180973757, 172338}, NULL) = 0
recv(4, "45678901234567890123456789012345"..., 8192, 0) = 1448
gettimeofday({1180973757, 172437}, NULL) = 0
recv(4, "23456789012345678901234567890123"..., 8192, 0) = 1448
gettimeofday({1180973757, 172576}, NULL) = 0
recv(4, "01234567890123456789012345678901"..., 8192, 0) = 1632
gettimeofday({1180973757, 172752}, NULL) = 0
recv(4, "", 8192, 0) = 0
gettimeofday({1180973757, 172797}, NULL) = 0
gettimeofday({1180973757, 172817}, NULL) = 0
nanosleep({0, 0}, NULL) = 0
nanosleep({0, 0}, NULL) = 0
close(4) = 0
futex(0x8055d04, 0x5 /* FUTEX_??? */, 1) = 1
futex(0x8055d30, FUTEX_WAKE, 1) = 0
_exit(0) = ?

2007-06-04 19:48:52

[permalink] [raw]

Subject: Re: iperf: performance regression (was b44 driver problem?)

On Monday 04 June 2007, Ingo Molnar wrote:
> * Stephen Hemminger <[email protected]> wrote:
> > Yes, the following patch makes iperf work better than ever. But are
> > other broken applications going to have same problem. Sounds like the
> > old "who runs first" fork() problems.
>
> this is the first such app and really, and even for this app: i've been
> frequently running iperf on -rt kernels for _years_ and never noticed
> how buggy its 'locking' code was, and that it would under some
> circumstances use up the whole CPU on high-res timers.

I must admit I don't know much about that topic, but there is one thing I
don't understand. Why is iperf (even if it's buggy) able to use up the whole
cpu? I didn't run it as root but as my normal user so it should have limited
rights. Shouldn't the linux scheduler distribute cpu time among all running
processes?

Maxi

Attachments:

(No filename) (890.00 B)
signature.asc (189.00 B)
This is a digitally signed message part. Download all attachments

2007-06-04 18:06:20

[permalink] [raw]

Subject: Re: iperf: performance regression (was b44 driver problem?)

On Mon, 04 Jun 2007 19:32:48 +0200
Thomas Gleixner <[email protected]> wrote:

> On Mon, 2007-06-04 at 09:59 -0700, Stephen Hemminger wrote:
> > > > gettimeofday({1180973726, 982754}, NULL) = 0
> > > > recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 8192, 0) = 8192
> > > > gettimeofday({1180973726, 983790}, NULL) = 0
> > >
> > > Well, gettimeofday() is not affected by the highres code, but
> > >
> > > > nanosleep({0, 0}, NULL) = 0
> > > > nanosleep({0, 0}, NULL) = 0
> > >
> > > is. The nanosleep call with a relative timeout of 0 returns immediately
> > > with highres enabled, while it sleeps at least until the next tick
> > > arrives when highres is off. Are there more of those stupid sleeps in
> > > the code ?
> >
> > GLIBC pthread_mutex does it, YES it is a problem!
> > Looks like the old behavior is required for ABI compatibility.
> >
> > iperf server has several threads. One thread is using pthread_mutex_lock
> > to wait for the other thread. It looks like pthread_mutex_lock is using
> > nanosleep as yield().
>
> I doubt that. This is in the iperf code itself.
>
> void thread_rest ( void ) {
> #if defined( HAVE_THREAD )
> #if defined( HAVE_POSIX_THREAD )
> // TODO add checks for sched_yield or pthread_yield and call that
> // if available
> usleep( 0 );
>
> ----------^^^^
>
> It results in a nanosleep({0,0}, NULL)
>
> tglx
>

Yes, the following patch makes iperf work better than ever.
But are other broken applications going to have same problem.
Sounds like the old "who runs first" fork() problems.

--- iperf-2.0.2/compat/Thread.c.orig 2005-05-03 08:15:51.000000000 -0700
+++ iperf-2.0.2/compat/Thread.c 2007-06-04 10:54:21.000000000 -0700
@@ -405,9 +405,13 @@
void thread_rest ( void ) {
#if defined( HAVE_THREAD )
#if defined( HAVE_POSIX_THREAD )
- // TODO add checks for sched_yield or pthread_yield and call that
- // if available
+
+#if defined( _POSIX_PRIORITY_SCHEDULING )
+ sched_yield();
+#else
usleep( 0 );
+#endif
+
#else // Win32
SwitchToThread( );
#endif

--
Stephen Hemminger <[email protected]>

2007-06-04 20:53:30

[permalink] [raw]

Subject: Re: iperf: performance regression (was b44 driver problem?)

On Monday 04 June 2007, Stephen Hemminger wrote:
> On Mon, 4 Jun 2007 21:47:59 +0200
>
> Maximilian Engelhardt <[email protected]> wrote:
> > On Monday 04 June 2007, Ingo Molnar wrote:
> > > * Stephen Hemminger <[email protected]> wrote:
> > > > Yes, the following patch makes iperf work better than ever. But are
> > > > other broken applications going to have same problem. Sounds like the
> > > > old "who runs first" fork() problems.
> > >
> > > this is the first such app and really, and even for this app: i've been
> > > frequently running iperf on -rt kernels for _years_ and never noticed
> > > how buggy its 'locking' code was, and that it would under some
> > > circumstances use up the whole CPU on high-res timers.
> >
> > I must admit I don't know much about that topic, but there is one thing I
> > don't understand. Why is iperf (even if it's buggy) able to use up the
> > whole cpu? I didn't run it as root but as my normal user so it should
> > have limited rights. Shouldn't the linux scheduler distribute cpu time
> > among all running processes?
>
> In this case, there are two threads. One is receiving data and the other
> is spinning checking on progress. If the spinning thread doesn't yield,
> it will end up using it's whole quantum (10ms at 100hz), before the
> scheduler lets the receiver run again. If the receiving thread doesn't
> get to run then on a UP the performance stinks.
>
Ok, let's see if I got this right:
If there are other processes that want cpu time they will get it after the
quantum for the iperf thread is used up. So cpu time will be distributed
among other processes, but it takes some time until they get it and this
increases latency.

> The problem only showed up laptop because most of my other systems are
> SMP (or fake SMP/HT), and usually set HZ to 1000 not 100.

Hm, on my laptop (Pentium M) I have configured CONFIG_HZ_300 and CONFIG_NO_HZ.
On my desktop PC (Athlon 2000+, also UP) I also have CONFIG_HZ_300 and
CONFIG_NO_HZ but didn't notice the problem.

Maxi

Attachments:

(No filename) (1.99 kB)
signature.asc (189.00 B)
This is a digitally signed message part. Download all attachments

2007-06-04 17:32:52

[permalink] [raw]

Subject: Re: iperf: performance regression (was b44 driver problem?)

On Mon, 2007-06-04 at 09:59 -0700, Stephen Hemminger wrote:
> > > gettimeofday({1180973726, 982754}, NULL) = 0
> > > recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 8192, 0) = 8192
> > > gettimeofday({1180973726, 983790}, NULL) = 0
> >
> > Well, gettimeofday() is not affected by the highres code, but
> >
> > > nanosleep({0, 0}, NULL) = 0
> > > nanosleep({0, 0}, NULL) = 0
> >
> > is. The nanosleep call with a relative timeout of 0 returns immediately
> > with highres enabled, while it sleeps at least until the next tick
> > arrives when highres is off. Are there more of those stupid sleeps in
> > the code ?
>
> GLIBC pthread_mutex does it, YES it is a problem!
> Looks like the old behavior is required for ABI compatibility.
>
> iperf server has several threads. One thread is using pthread_mutex_lock
> to wait for the other thread. It looks like pthread_mutex_lock is using
> nanosleep as yield().

I doubt that. This is in the iperf code itself.

void thread_rest ( void ) {
#if defined( HAVE_THREAD )
#if defined( HAVE_POSIX_THREAD )
// TODO add checks for sched_yield or pthread_yield and call that
// if available
usleep( 0 );

----------^^^^

It results in a nanosleep({0,0}, NULL)

tglx

2007-06-04 20:15:55

[permalink] [raw]

Subject: Re: iperf: performance regression (was b44 driver problem?)

On Mon, 4 Jun 2007 21:47:59 +0200
Maximilian Engelhardt <[email protected]> wrote:

> On Monday 04 June 2007, Ingo Molnar wrote:
> > * Stephen Hemminger <[email protected]> wrote:
> > > Yes, the following patch makes iperf work better than ever. But are
> > > other broken applications going to have same problem. Sounds like the
> > > old "who runs first" fork() problems.
> >
> > this is the first such app and really, and even for this app: i've been
> > frequently running iperf on -rt kernels for _years_ and never noticed
> > how buggy its 'locking' code was, and that it would under some
> > circumstances use up the whole CPU on high-res timers.
>
> I must admit I don't know much about that topic, but there is one thing I
> don't understand. Why is iperf (even if it's buggy) able to use up the whole
> cpu? I didn't run it as root but as my normal user so it should have limited
> rights. Shouldn't the linux scheduler distribute cpu time among all running
> processes?

In this case, there are two threads. One is receiving data and the other
is spinning checking on progress. If the spinning thread doesn't yield,
it will end up using it's whole quantum (10ms at 100hz), before the
scheduler lets the receiver run again. If the receiving thread doesn't
get to run then on a UP the performance stinks.

The problem only showed up laptop because most of my other systems are
SMP (or fake SMP/HT), and usually set HZ to 1000 not 100.

It kind of reminds me of the family road trip with the kids in the back
seat going "Are we there yet?"

--
Stephen Hemminger <[email protected]>

2007-06-04 16:36:04

[permalink] [raw]

Subject: Re: b44: regression in 2.6.22 (resend)

On Mon, 2007-06-04 at 09:09 -0700, Stephen Hemminger wrote:
> > > I did the test with an 2.6.22-rc3-git4 kernel and the p54 driver built
> > > external as module.
> >
> > Can you look at iperf to figure out, whether it does some weird timer
> > stuff (high frequency interval timer or such) ? Either check the code or
> > strace it.
>
> It is the receiver doing a tight loop doing gettimeofday/recv calls.
>
>
> sendto(-1227715616, 0xc, 3085438964, 0, {...}, 3067249832) = 0
> gettimeofday({1180973726, 981615}, NULL) = 0
> gettimeofday({1180973726, 981751}, NULL) = 0
> futex(0x8055c64, 0x5 /* FUTEX_??? */, 1) = 1
> futex(0x8055c90, FUTEX_WAKE, 1) = 0
> recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 8192, 0) = 8192
> gettimeofday({1180973726, 982754}, NULL) = 0
> recv(4, "\0\0\0\0\0\0\0\1\0\0\23\211\0\0\0\0\0\0\0\0\377\377\364"..., 8192, 0) = 8192
> gettimeofday({1180973726, 983790}, NULL) = 0

Well, gettimeofday() is not affected by the highres code, but

> nanosleep({0, 0}, NULL) = 0
> nanosleep({0, 0}, NULL) = 0

is. The nanosleep call with a relative timeout of 0 returns immediately
with highres enabled, while it sleeps at least until the next tick
arrives when highres is off. Are there more of those stupid sleeps in
the code ?

tglx

2007-06-04 19:01:05