net drivers and cache alignment

One of the [many] nice properties of the traditional Don Becker drivers has been that often the driver-private structures are arranged such that
the structure is broken up on cacheline boundaries. The RX thread has
a cacheline, the TX thread has a cacheline, etc. Jes Sorensen also independently, in his review of 8139cp.c, suggested that the driver-private struct be update with attention to cacheline boundaries.

Early next year, I would like to start cleaning up some of the net drivers along these lines (no pun intended). To make it easier for vendors and random coders to cacheline-align struct members, I would like to make more explicit these cacheline boundaries, in a manner that is portable between 32-bit and 64-bit systems.

I attach a sample implementation, and request feedback on this approach.
The general idea is to make implementing this sort of concept "harder
to screw up."

===== drivers/net/tg3.c 1.41 vs edited =====

@@ -25,6 +25,7 @@
 #include <linux/if_vlan.h>
 #include <linux/ip.h>
 #include <linux/tcp.h>
+#include <linux/cache.h>
 
 #include <asm/system.h>
 #include <asm/io.h>

===== drivers/net/tg3.h 1.19 vs edited =====

===== include/linux/cache.h 1.5 vs edited =====

@@ -53,4 +53,14 @@
 #endif
 #endif
 
+/* helper used inside struct definitions to break up struct at
+ * cacheline boundaries.
+ * Note: do not add a semi-colon (';') after an ALIGNED_PAD use.
+ */
+struct dummy_cacheline_struct {
+	int x;
+} ____cacheline_aligned;
+#define ALIGNED_PAD(name) \
+	struct dummy_cacheline_struct name;
+
 #endif /* __LINUX_CACHE_H */

Can't the cacheline_aligned attribute be applied to individual struct members? I remember doing this for thread_struct on sparc ages ago.

I was hoping someone who knows gcc better than me knew that, and would speak up ;-)

Looks like it from the 2.4 processor.h code.

Attached is cut #2. Thanks for all the near-instant feedback so far :)
Andrew, does the attached still need padding on SMP?

===== drivers/net/tg3.c 1.41 vs edited =====

@@ -25,6 +25,7 @@
 #include <linux/if_vlan.h>
 #include <linux/ip.h>
 #include <linux/tcp.h>
+#include <linux/cache.h>
 
 #include <asm/system.h>
 #include <asm/io.h>

===== drivers/net/tg3.h 1.19 vs edited =====

It needs padding _only_ on SMP. ____cacheline_aligned_in_smp.

#define offsetof(t, m) ((int)(&((t *)0)->m))

struct foo {
        int a;
        int b __attribute__((__aligned__(1024)));
        int c;
} foo;

main()

{
        printf("%d\n", sizeof(struct foo));
        printf("%d\n", offsetof(struct foo, a));
        printf("%d\n", offsetof(struct foo, b));
        printf("%d\n", offsetof(struct foo, c));
}

./a.out
2048
0
1024
1028

So your patch will do what you want it to do. You should just tag the first member of a group with ____cacheline_aligned_in_smp, and keep an eye on things with offsetof().

Not sure why sizeof() returned 2048 though.

From: Andrew Morton <[email protected]>
Date: Sat, 07 Dec 2002 15:29:16 -0800

It needs padding _only_ on SMP. ____cacheline_aligned_in_smp.

non-smp machines lack L2 caches? That's new to me :-)

More seriously, there are real benefits on non-SMP systems.

Then I am most confused. None of these fields will be put under busmastering or anything like that, so what advantage is there in spreading them out?

Oh I see what you want - to be able to pick up all the operating fields in a single fetch.

That will increase the overall cache footprint though. I wonder if it's really a net win, over just keeping it small.

Andrew> Oh I see what you want - to be able to pick up all the Andrew> operating fields in a single fetch.

Andrew> That will increase the overall cache footprint though. I Andrew> wonder if it's really a net win, over just keeping it small.

There's another case where it matters, I guess one could look at it as similar to the SMP case, but between CPU and device. Some devices have producer indices in host memory which they update whenever it receiving a packet. By putting that seperate from TX data structures you avoid the CPU and the NIC fighting over cache lines.

Cheers,
Jes

From: Andrew Morton <[email protected]>
Date: Sat, 07 Dec 2002 15:42:00 -0800

Then I am most confused. None of these fields will be put under busmastering or anything like that, so what advantage is there in spreading them out?

When you are in the "tx path" you'll take one L2 cache miss to bring all the necessary information into the cpu's caches.

Otherwise, when data is arbitrarily scattered over multiple L2 cache lines, you'll need to service potentially more L2 cache misses.

This optimization has nothing to do with false data sharing amoungst multiple processors. It's about packing the data accesses optimally for specific code paths.

[...]

thanks.

For this case, though, I want to align on cacheline bounaries even on UP, right? That's why I picked ____cacheline_aligned. It uses L1_CACHE_BYTES when !CONFIG_SMP. Other uses of ____cacheline_aligned in the kernel seem to relate to irq matters, just like my groupings in tg3.h.

[obviously benchmarking can answer some of this, but I want to hammer out silliness first]

Jeff

What do you all think about this:

#include <stdio.h>

#define CACHE_LINE_SIZE 128
#define ____cacheline_aligned __attribute__((__aligned__(CACHE_LINE_SIZE)))

#define __cacheline_start struct { } ____cacheline_aligned;

#define offsetof(t, m) ((int)(&((t *)0)->m))

struct S {
    __cacheline_start

    int x;

    __cacheline_start

    int y;
    int z;
};

int main()

{
    struct S s;

    printf("%d\n",sizeof(struct S));
    printf("%d\n",offsetof(struct S,x));
    printf("%d\n",offsetof(struct S,y));
    printf("%d\n",offsetof(struct S,z));
}

werewolf:~> vi kk.c
werewolf:~> kk
256
0
128
132

So you don't have to modify any field, just put __cacheline_start where needed ? (and does not add any extra sizeof(int) overhead).

J.A. Magallon <[email protected]>      \                 Software is like sex:
werewolf.able.es                         \           It's better when it's free
Mandrake Linux release 9.1 (Cooker) for i586
Linux 2.4.20-jam1 (gcc 3.2 (Mandrake Linux 9.1 3.2-4mdk))

if you can mark struct members with attributes, as it appears you can, there's no need to define a struct.

Jeff

That will generate a warning on faster^Wolder versions of gcc.

mnm:/home/akpm> gcc t2.c
t2.c:11: warning: unnamed struct/union that defines no instances t2.c:15: warning: unnamed struct/union that defines no instances mnm:/home/akpm> gcc -v
Reading specs from /usr/local/lib/gcc-lib/i686-pc-linux-gnu/2.95.3/specs gcc version 2.95.3 20010315 (release)

And how 'bout this (do not have any gcc oldie available to test):

#define __cacheline_start union { int :0; } ____cacheline_aligned;

It passes gcc-3.2 -Wall...
I think it's nicer to insert __c_s than to go field by field marking them...

J.A. Magallon <[email protected]>      \                 Software is like sex:
werewolf.able.es                         \           It's better when it's free
Mandrake Linux release 9.1 (Cooker) for i586
Linux 2.4.20-jam1 (gcc 3.2 (Mandrake Linux 9.1 3.2-4mdk))

The structure contains an __aligned__(1024) item. Think about an array of 'struct foo' items. They have to be 2048 bytes or you won't align correctly.

C allows for empty space in structure padding, but not in arrays, AFAIK.

Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer