Tigon3 5701 PCI-X recv performance problem

I am seeing a problem with PCI-X recv performance on the Broadcom 5701 cards. This is due to a known PCI-X errata with the DMA engine when buffers are non zero offset aligned. As well as performance problems there are also a lot of kernel unaligned access messages in the system log (hundereds of them) :-

kernel unaligned access to 0xe0000030174f382e, ip=0xe0000000047d1f80

This affects both 2.4x and 2.6x tg3.o drivers

Measure of performance WITHOUT fix :-
mig125:~ # nttcp -r -T -l262144 -w1024 -n1000 10.50.1.130 -l262144 -w1024
Bytes Real s CPU s Real-MBit/s CPU-MBit/s Calls Real-C/s CPU-C/s

l2621440000  118.36   43.24    177.1846    484.9782 1818304  15362.52   42049.3
12621440000  118.34   12.70    177.2167   1651.3857   10000     84.50     787.4

I then applied a fix to the driver :-

line 2260 of linux-2.6.0-test6/drivers/net/tg3.c

- if (len > RX_COPY_THRESHOLD) {
+ if (len > RX_COPY_THRESHOLD && tp->rx_offset == 2) {

Then I loaded a tg3.o module which has the fix, and ran the test again

Measure of performance WITH fix :-
mig125:~ # nttcp -r -T -l262144 -w1024 -n1000 10.50.1.130 -l262144 -w1024

     Bytes  Real s   CPU s Real-MBit/s  CPU-MBit/s   Calls  Real-C/s   CPU-C/s
l262144000    2.23    1.64    940.6910   1277.5033   35082  15736.26   21370.6

1262144000 2.22 1.43 944.0677 1470.4690 1000 450.17 701.2

This ONLY affects Broadcom 5701 based Gigabit Ethernet cards 5704 cards do not have the same errata. I confirmed that the fix did not affect the performance or functionality of 5704 cards, actually the fix ensures that the 5701 cards don't go through the same code path as 5704 (becuase tp->rx_offset is 0 for 5701 so we always align the buffers for the 5701 card).

John

John Partridge

Silicon Graphics Inc
Tel:  651-683-3428
Vnet: 233-3428
E-Mail: [email protected]	

Oh yeah? What are your numbers like if you just disable the ia64 kernel unaligned access printk()?

Not too much different

mig125:~ # nttcp -r -T -l262144 -w1024 -n1000 10.50.1.130 -l262144 -w1024

     Bytes  Real s   CPU s Real-MBit/s  CPU-MBit/s   Calls  Real-C/s   CPU-C/s
l318341120   10.00    9.96    254.6131    255.6215   22500   2249.47    2258.4
1318341120   10.00    0.41    254.6825   6156.0247   19430   1943.07   46966.7

John Partridge

Silicon Graphics Inc
Tel:  651-683-3428
Vnet: 233-3428
E-Mail: [email protected]

The problem is that your change is arch-dependant yet you make it run on all platforms.

On x86 we don't want to do what your change is doing, the unaligned accesses are cheap enough.

We need to abstract this, probably in the same way it is done in the Tulip and other drivers which have similar issues.

OK, fair enough, you mean like :-

#if defined(__ia64__)
	if (len > RX_COPY_THRESHOLD && tp->rx_offset == 2) {
#else
	if (len > RX_COPY_THRESHOLD) {

#endif

John Partridge

Silicon Graphics Inc
Tel:  651-683-3428
Vnet: 233-3428
E-Mail: [email protected]

I think it'd be better to create some macro like this:

#ifdef FORCE_SKB_ALIGNMENT

# define SKB_COPY_CHECK(len) len > RX_COPY_THRESHOLD && tp->rx_offset == 2
#else
# define SKB_COPY_CHECK(len) len > RX_COPY_THRESHOLD
#endif

Then replace the code in the if with the new macro. We can define FORCE_SKB_ALIGNMENT in our build environment as can others if necessary. otherwise, everyone does what they did before.

Steve

Steve Modica
work: 651-683-3224
MTS-Technical Lead
"Give a man a fish, and he will eat for a day, hit him with a fish and
he leaves you alone" - me

Please, do this right. Make a "CONFIG_UNALIGNED_VERY_SLOW" or something that platforms can define, and then drivers can ifdef upon.

Hi David,

it's definitely not the printk. They have that throttled so it only prints once for a large number of occurances.

The problem is that on the Altix platform they have to deal with unaligned accesses via an exception handler. This causes them to run through hundreds of instructions. There's evidntally another mechanism in the cpu that's more efficient in daling with unaligned accesses, but we can't use that for some reason (atomicity I think, but I don't recall).

This extra memcpy when the buffer is not unaligned currently only impacts 5701 (since it's the only chip for which the 2 byte pad is omitted) although the alignment issue would bite us no matter what the chip if the 2 byte pad is omitted.

Steve

Steve Modica
work: 651-683-3224
MTS-Technical Lead
"Give a man a fish, and he will eat for a day, hit him with a fish and
he leaves you alone" - me

You mean, ia64.

Has anyone optimized the unaligned trap handler on ia64 (perhaps coding it in raw assembler) to see what the situation looks like then?

Nobody wants to ever comment on this...

But it affects all platforms, not just ones that ia64 which have the unaligned trap cost. For example, we _DEFINITELY_ don't want to do what your patch causes to happen on x86.

Atomicity should not be needed to access a private skb.

Maybe you didn't want to change the core stack to use the unaligned access mechanism?

In that case it may be better to fix the stack with some macro that expands to the unaligned access on IA64 and a normal load on other architectures.

-Andi

I have a very strong feeling that we'd really need both options to arrive at an optimal implementation on all platforms.

Something that makes drivers copy packets, and something else that traps packet header accesses.

But I don't like any of these solutions (and I know Linus will never accept a set of changes that puts netdev_get_unaligned() macro usage all over the entire networking layer). Instead, we should do what you proposed long ago. Seperating the protocol headers from the packet data. Then we need only align the protocol headers.

In fact, I can suggest a very efficient implementation:

1) Driver allocates paged SKBs. There is ~128 bytes of skb->data buffer area, and pages are chopped up into MTU'ish sized chunks and hung onto SKBs in the frag list.

2) The device is given the page buffers to receive packets into.

3) On RX, align skb->data (ie. skb_reserve(skb, 2) for ethernet drivers) and copy the first N bytes from the head of the paged buffer to skb->data, point the fraglist entry for the page at offset "N" into the page chunk buffer.

Problem solved.

I agree that it would be the best solution, but isn't it a bit late in 2.6 now for that? Sounds more like a great 2.7.0 project.

It's not that it's a new problem - we had this since the Alpha port and it hasn't gotten more urgent suddenly.

For 2.6 short term probably some bandaid like the CONFIG_UNALIGNMENT_COSTLY and doing driver copies is better.

Hmm - you mean it allocates a full page and does suballocation by itself?

The suballocation would need to be per CPU to be SMP efficient I guess, which would complicate it.

Another drawback that the page allocators per CPU allocator is not as strong as slabs, so it may perform not as good.

And it would require the skb driver accessible destructor that was resisted against for so long to manage the suballocation lists ;-)

But with such a destructor you would actually not need to do own allocation, because you could just convert the memory areas returned by slab to (struct page *, offset)

-Andi

Frankly, I just want to shut all the ia64 users up because they keep barking due to the kernel unaligned trap message that port spits out.

We could write some helper routines.

Andi, stop right there, we're talking about mitigating the horrible performance some $6.00 USD Taiwaneese network cards get on expensive ia64 systems. How much effort do you think we should drain into optimizing this? :-)

Well, this thread was about the tigon3 and I don't see that as an el cheapo card. If SGI uses it on the Altix I guess they want it to perform well with many CPUs.

Anyways, it was just a brain dump regarding your idea. I personally think it's better to just use slab to implement it, allocating pages doesn't seem to have any advantages to me.

-Andi

It's one of the oldest variants of the tg3 chip and it's full of hardware bugs when used in PCI-X.

The page chunk allocator is meant to make it easier to put the non-header parts in the frag list of the SKB, see? It means we don't need to do anything special in the networking, all the receive paths handle frag'd RX packets properly.

We can't take pages SLAB is using and attach them to the fraglist of the SKB, kfree_skb() is going to try and put_page() on them.

If we used slab, we'd need to do something like:

1) skb->h.raw or whatever have special meaning and point to different buffers.

2) There's a skb->data2 or something like that.

Both are a lot of work compared to my suggestion.

Ok. Why do we care about it then? Copying should be fine for that.

Sure, but to handle the sub allocation you need a destructor per fragment. (otherwise how do you want to share a page between different packets)

And when you have a destructor you can as well allocate from slab and convert the virtual pointer to (struct page *, offset)

If you don't want to share pages between different packets: I don't like this because it would increase memory use of networking with 1.5k MTU by factor 2. I don't think that would be a good path to go down, Linux networking is already too bloated.

BTW I think this all should be also ifdefed with CONFIG_SLOW_UNALIGNMENT. I certainly don't want any of this on x86-64 where unalignment cost one cycle only.

Another BTW Actually this technique would allow socket buffers in highmem, but it's not really needed yet and all 32bit architecture which have high mem usually have fast unalignment handling.

-Andi

Aha, no you don't, this is the beauty of it.

Let's say we've packed 4 packets into a page (or 10 in 2 pages, whatever the optimal packing is), as you attach each chunk to a SKB you up the page count (if the buffer straddles 2 or more pages you use one frag entry for each of those pages and bump the count as approprise). As far as the networking is concerned, it's some page cache page or whatever, it doesn't care.

Then kfree_skb(skb) just does the right thing by putting all the pages, when the page count goes to zero it's free'd up.

I agree, I don't even want this rediculious crap on sparc64 where I can make the unaligned trap handler 30 or 40 cycles or even less.

BTW, your highmem example is interesting, but even more interesting are the cards that do the magic multiple-TCP-packet coalescing so that the data parts are all page aligned. They want infrastructure like this.

Question.

Is there a way for us to create a CONFIG entry like this, but also allow for a modules.conf argument? (perhaps "options tg3 unalignment_costly")

Customers running Suse on Altix have to edit modules.conf anyhow so they use the tg3 module (instead of bcm). So when they do this, we could easily have them add an argument to the modules.conf line and we avoid a kernel build. This let's us run with a standard ia64 kernel.

Steve

Steve Modica
work: 651-683-3224
MTS-Technical Lead
"Give a man a fish, and he will eat for a day, hit him with a fish and
he leaves you alone" - me

Doesn't it already have a rx_copy_break argument? If yes just set it to zero and it will always copy. If not add it.

-Andi

The problem with this solution is that it also impacts 5704 cards, which don't have the alignment issue. We are figuring all this out. We may just switch over completely and replace the 5701 cards.

Then just don't set the argument to zero in modules.conf on the 5704cards. That could be even handled somewhere in user space. Switching over sounds best of course, because the performance with the copying won't be great

-Andi

There is no "argument" we don't have a variable in the tg3 driver that determines the "copybreak", it's a hard-coded macro define.

For them it's really easy to know when the 5701 hw bug case is present, so they can do something like:

if (len > RX_COPY_THRESHOLD

#ifdef CONFIG_UNALIGNED_EXPENSIVE
	    || tp->rx_offset == 2
#endif
	    ) {
        ...

And then only their platform and only when the buggy chips are present, will get the desired change in behavior.

Could we create a module argument like "do_5701_align" and just give it a -1 default which does NOT do the copy but if we give the arg 1, 0, whatever it DOES to the copy to align the buffs ?

Then inside the driver we define a macro to handle that ?

That way all you need is to include "tg3 do_5701_align=1" in modules.conf

That way all the default comiles work and no CONFIG option for people who compile all defaults and don't want to re compile the kernel for this issue.

Is that acceptable ?

John

John Partridge

Silicon Graphics Inc
Tel:  651-683-3428
Vnet: 233-3428
E-Mail: [email protected]

Absolutely not, we're not going to put super-hacky module parameters into the driver just because you guys don't want to create and use the CONFIG_UNALIGNED_EXPENSIVE config option needed to fix this properly.

No it's not.

When you add this new config option, and set it on ia64, the only thing that will need to be rebuilt is the tg3 driver, no other thing in the entire kernel tree should trigger a dependency on this new config option.

I don't understand why at all you guys are trying to avoid fixing this the proper way, you will meet only resistence from me if you continue doing that :)

The key point for me is, as David said yesterday, SGI users will only have to recompile the tg3 driver with the CONFIG defined to get the desired behavior, not hte whole kernel. They just use the recompiled driver with whatever kernel they are using. Users who use SGI propack will already have the (static) driver with the CONFIG defined by SGI, SuSE users who use a tg3.o use it as a module anyway, so it's not a big deal just to recompile tg3. The only problem I can see is if users use RedHat Advanced Server, I believe the tg3 driver is static in the kernel so they won't be able to just recompile the tg3 driver.

I do propose that we make the CONFIG a little more descriptive something like

CONFIG_5701PCIX_UNALIGNED_EXPENSIVE

just to make it easier for users to see what it does.

John

John Partridge

Silicon Graphics Inc
Tel:  651-683-3428
Vnet: 233-3428
E-Mail: [email protected]

That's grosser than gross.

We're trying to create a generic mechanism to fix this kind of problem in _ALL_ drivers. I will heavily thwart any attempt to make this some kind of "tg3 thing".

I'm working on a patch for the tg3 driver for 2.6

Please can you look at this. I'm not too sure about the Kconfig entry as I have not done one before :-

@@ -2257,7 +2257,11 @@
 
                len = ((desc->idx_len & RXD_LEN_MASK) >> RXD_LEN_SHIFT) - 4; /* omit crc */
 
-               if (len > RX_COPY_THRESHOLD) {
+               if (len > RX_COPY_THRESHOLD
+#if defined(CONFIG_UNALIGNED_EXPENSIVE)
+                        && tp->rx_offset == 2
+#endif
+               ) {
                        int skb_size;
 
                        skb_size = tg3_alloc_rx_skb(tp, opaque_key,

@@ -2017,6 +2017,9 @@
          To compile this driver as a module, choose M here: the module
          will be called tg3.  This is recommended.
 
+config CONFIG_UNALIGNED_EXPENSIVE
+       bool "Use Aligned SKB's for 5701 cards (for Itanium2 based systems)"
+       depends on TIGON3 && IA64
 endmenu
 
 #

Thks
John

John Partridge

Silicon Graphics Inc
Tel:  651-683-3428
Vnet: 233-3428
E-Mail: [email protected]

It belongs in arch/${ARCH}/Kconfig not drivers/net/Kconfig

It's a static property of the architecture, not something the user chooses one way or the other.

Is this OK ? I was not sure about the order should depends be beofre default or the other way round ? or does it matter ? I suspect it does.

Thks
John

@@ -2257,7 +2257,11 @@
 
                len = ((desc->idx_len & RXD_LEN_MASK) >> RXD_LEN_SHIFT) - 4; /*                                                        omit crc */
 
-               if (len > RX_COPY_THRESHOLD) {
+               if (len > RX_COPY_THRESHOLD
+#if defined(CONFIG_UNALIGNED_EXPENSIVE)
+                        && tp->rx_offset == 2
+#endif
+               ) {
                        int skb_size;
 
                        skb_size = tg3_alloc_rx_skb(tp, opaque_key,

@@ -468,6 +468,11 @@
          agent" (/sbin/hotplug) to load modules and set up software needed
          to use devices as you hotplug them.
 
+config CONFIG_UNALIGNED_EXPENSIVE
+       bool "Use Aligned SKB's for Tigon3 5701 network adapters"
+       depends on MCKINLEY
+       default y
+
 source "drivers/pci/hotplug/Kconfig"
 
 source "drivers/pcmcia/Kconfig"

John Partridge

Silicon Graphics Inc
Tel:  651-683-3428
Vnet: 233-3428
E-Mail: [email protected]

Why are you depending upon MCKINLEY? Don't all ia64 cpus give traps for unaligned memory accesses?

That is what this CONFIG option tells the whole kernel.

I have no evidence of a problem with Itanium1, only on Itanium2 (MCKINLEY) I have seen it on HP and SGI MCKINLEY

John Partridge

Silicon Graphics Inc
Tel:  651-683-3428
Vnet: 233-3428
E-Mail: [email protected]

Ask the other David M about eepro100.c patches he wanted to send to me...

Jeff

Jeff,

I have not seen the change for this go into 2.6 is there something else I should be doing ?

Thanks
John

John Partridge

Silicon Graphics Inc
Tel:  651-683-3428
Vnet: 233-3428
E-Mail: [email protected]

OK, I made it ALL IA64, but I only know for sure about Itanium2's

@@ -2257,7 +2257,11 @@
 
                len = ((desc->idx_len & RXD_LEN_MASK) >> RXD_LEN_SHIFT) - 4; /* omit crc */
 
-               if (len > RX_COPY_THRESHOLD) {
+               if (len > RX_COPY_THRESHOLD
+#if defined(CONFIG_UNALIGNED_EXPENSIVE)
+                        && tp->rx_offset == 2
+#endif
+               ) {
                        int skb_size;
 
                        skb_size = tg3_alloc_rx_skb(tp, opaque_key,

@@ -468,6 +468,11 @@
          agent" (/sbin/hotplug) to load modules and set up software needed
          to use devices as you hotplug them.
 
+config CONFIG_UNALIGNED_EXPENSIVE
+       bool "Use kernel aligned buffers"
+       depends on IA64
+       default y
+
 source "drivers/pci/hotplug/Kconfig"
 
 source "drivers/pcmcia/Kconfig"

John Partridge

Silicon Graphics Inc
Tel:  651-683-3428
Vnet: 233-3428
E-Mail: [email protected]

This patch looks a lot better.

Jeff, perhaps you'll combine this with the thing David Mosberger wanted and then merge?

Thanks Andi,

@@ -2257,7 +2257,11 @@
 
                len = ((desc->idx_len & RXD_LEN_MASK) >> RXD_LEN_SHIFT) - 4; /* omit crc */
 
-               if (len > RX_COPY_THRESHOLD) {
+               if (len > RX_COPY_THRESHOLD
+#if defined(CONFIG_UNALIGNED_EXPENSIVE)
+                        && tp->rx_offset == 2
+#endif
+               ) {
                        int skb_size;
 
                        skb_size = tg3_alloc_rx_skb(tp, opaque_key,

@@ -468,6 +468,10 @@
          agent" (/sbin/hotplug) to load modules and set up software needed
          to use devices as you hotplug them.
 
+config CONFIG_UNALIGNED_EXPENSIVE
+       depends on IA64
+       default y
+
 source "drivers/pci/hotplug/Kconfig"
 
 source "drivers/pcmcia/Kconfig"

John Partridge

Silicon Graphics Inc
Tel:  651-683-3428
Vnet: 233-3428
E-Mail: [email protected]

indeed.

For my part, I will apply the patch to drivers/net/tg3.c, which is fine, and will let the arch/ia64/Kconfig patch go through normal channels (since ia64 seems to be flowing these days).

Jeff

Yes, that looks right to me. I'm still learning the whole 2.6 Kconfig thing

Is this OK ?

@@ -2257,7 +2257,11 @@
 
                len = ((desc->idx_len & RXD_LEN_MASK) >> RXD_LEN_SHIFT) - 4; /* omit crc */
 
-               if (len > RX_COPY_THRESHOLD) {
+               if (len > RX_COPY_THRESHOLD
+#if defined(CONFIG_UNALIGNED_EXPENSIVE)
+                        && tp->rx_offset == 2
+#endif
+               ) {
                        int skb_size;
 
                        skb_size = tg3_alloc_rx_skb(tp, opaque_key,

@@ -468,6 +468,10 @@
          agent" (/sbin/hotplug) to load modules and set up software needed
          to use devices as you hotplug them.
 
+config UNALIGNED_EXPENSIVE
+       depends on IA64
+       default y
+
 source "drivers/pci/hotplug/Kconfig"
 
 source "drivers/pcmcia/Kconfig"

John Partridge

Silicon Graphics Inc
Tel:  651-683-3428
Vnet: 233-3428
E-Mail: [email protected]

We should CC David Mosberger on this, too. He's been wanting something like this (for, as you say, all network drivers) for a while now.

Jeff

The problem with this is if you mix 5704 and 5701 cards in the same system you are not seting it for the 5701's that need it.

John

John Partridge

Silicon Graphics Inc
Tel:  651-683-3428
Vnet: 233-3428
E-Mail: [email protected]