[RFC] changing value of NETDEV_ALIGN to cacheline size

[RFC] changing value of NETDEV_ALIGN to cacheline size

[Christian Borntraeger]

while digging through the alloc_netdev function I asked myself why there is a
fixed alignment for netdevices. Is there a special reason for choosing 32? If
yes, I suggest to add a comment to the definition. 

If not, I suspect cacheline alignment might be beneficial:
struct net_device contains several variables which are cache aligned 
(poll_list, queue_lock .....). As far as I can see, the compiler tries to 
increase the size of the structure to make that possible, but expects the
whole structure to be aligned to cache line size as well. With the current
value for NETDEV_ALIGN we dont align "struct net_device" to the cache line,
instead we align it to 32 bytes. That means that poll_list, queue_lock and 
friends are not always cache aligned, but 32 bytes aligned.

The only reason why everything worked so far is the slab allocator design, 
which gives us a page aligned struct net_device in most cases. I think we 
should not rely on the behaviour of the memory allocator and use a different 
value for NETDEV_ALIGN instead. Any comments or corrections?

cheers Christian



The patch below is compile and boot tested on s390 and x86.

Signed-off-by: Christian Borntraeger <borntrae@de.ibm.com>

include/linux/netdevice.h |    2 +-
net/core/dev.c            |    2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
----------

--- a/include/linux/netdevice.h	4 Apr 2006 07:25:47 -0000
+++ b/include/linux/netdevice.h	15 May 2006 11:06:05 -0000
@@ -504,7 +504,7 @@
 	struct class_device	class_dev;
 };
 
-#define	NETDEV_ALIGN		32
+#define	NETDEV_ALIGN		L1_CACHE_BYTES
 #define	NETDEV_ALIGN_CONST	(NETDEV_ALIGN - 1)
 
 static inline void *netdev_priv(struct net_device *dev)
--- a/net/core/dev.c	4 Apr 2006 07:25:50 -0000
+++ b/net/core/dev.c	15 May 2006 11:06:05 -0000
@@ -2986,7 +2986,7 @@
 	struct net_device *dev;
 	int alloc_size;
 
-	/* ensure 32-byte alignment of both the device and private area */
+	/* ensure cacheline alignment of both the device and private area */
 	alloc_size = (sizeof(*dev) + NETDEV_ALIGN_CONST) & ~NETDEV_ALIGN_CONST;
 	alloc_size += sizeof_priv + NETDEV_ALIGN_CONST;
 

Re: [RFC] changing value of NETDEV_ALIGN to cacheline size

[Randy.Dunlap]

On Mon, 15 May 2006 14:08:29 +0200 Christian Borntraeger wrote:

> while digging through the alloc_netdev function I asked myself why there is a
> fixed alignment for netdevices. Is there a special reason for choosing 32? If
> yes, I suggest to add a comment to the definition. 
> 
> If not, I suspect cacheline alignment might be beneficial:
> struct net_device contains several variables which are cache aligned 
> (poll_list, queue_lock .....). As far as I can see, the compiler tries to 
> increase the size of the structure to make that possible, but expects the
> whole structure to be aligned to cache line size as well. With the current
> value for NETDEV_ALIGN we dont align "struct net_device" to the cache line,
> instead we align it to 32 bytes. That means that poll_list, queue_lock and 
> friends are not always cache aligned, but 32 bytes aligned.
> 
> The only reason why everything worked so far is the slab allocator design, 
> which gives us a page aligned struct net_device in most cases. I think we 
> should not rely on the behaviour of the memory allocator and use a different 
> value for NETDEV_ALIGN instead. Any comments or corrections?
> 
> cheers Christian
> 
> 
> 
> The patch below is compile and boot tested on s390 and x86.
> 
> Signed-off-by: Christian Borntraeger <borntrae@de.ibm.com>
> 
> include/linux/netdevice.h |    2 +-
> net/core/dev.c            |    2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
> ----------
> 
> --- a/include/linux/netdevice.h	4 Apr 2006 07:25:47 -0000
> +++ b/include/linux/netdevice.h	15 May 2006 11:06:05 -0000
> @@ -504,7 +504,7 @@
>  	struct class_device	class_dev;
>  };
>  
> -#define	NETDEV_ALIGN		32
> +#define	NETDEV_ALIGN		L1_CACHE_BYTES
>  #define	NETDEV_ALIGN_CONST	(NETDEV_ALIGN - 1)

I don't know about the fixed value of 32, but if this patch is
accepted, I'd prefer NETDEV_ALIGN_MASK instead of NETDEV_ALIGN_CONST.

>  static inline void *netdev_priv(struct net_device *dev)
> --- a/net/core/dev.c	4 Apr 2006 07:25:50 -0000
> +++ b/net/core/dev.c	15 May 2006 11:06:05 -0000
> @@ -2986,7 +2986,7 @@
>  	struct net_device *dev;
>  	int alloc_size;
>  
> -	/* ensure 32-byte alignment of both the device and private area */
> +	/* ensure cacheline alignment of both the device and private area */
>  	alloc_size = (sizeof(*dev) + NETDEV_ALIGN_CONST) & ~NETDEV_ALIGN_CONST;
>  	alloc_size += sizeof_priv + NETDEV_ALIGN_CONST;

---
~Randy

Re: [RFC] changing value of NETDEV_ALIGN to cacheline size

[David S. Miller]

From: "Randy.Dunlap" <rdunlap@xenotime.net>
Date: Mon, 15 May 2006 08:02:58 -0700

> > -#define	NETDEV_ALIGN		32
> > +#define	NETDEV_ALIGN		L1_CACHE_BYTES
> >  #define	NETDEV_ALIGN_CONST	(NETDEV_ALIGN - 1)
> 
> I don't know about the fixed value of 32, but if this patch is
> accepted, I'd prefer NETDEV_ALIGN_MASK instead of NETDEV_ALIGN_CONST.

The reason it's 32 is that old drivers depended on the
struct being at least 32-byte aligned because they would
embed structures DMA'd to/from the card in their private
area and just assumed that would be aligned enough for
the card's restrictions.

So setting it to L1_CACHE_BYTES would be wrong, because if
that happens to be less than 32 it would violate said
assumption which we are catering to.

Re: [RFC] changing value of NETDEV_ALIGN to cacheline size

[Rick Jones]

David S. Miller wrote:
> From: "Randy.Dunlap" <rdunlap@xenotime.net>
> Date: Mon, 15 May 2006 08:02:58 -0700
> 
> 
>>>-#define	NETDEV_ALIGN		32
>>>+#define	NETDEV_ALIGN		L1_CACHE_BYTES
>>> #define	NETDEV_ALIGN_CONST	(NETDEV_ALIGN - 1)
>>
>>I don't know about the fixed value of 32, but if this patch is
>>accepted, I'd prefer NETDEV_ALIGN_MASK instead of NETDEV_ALIGN_CONST.
> 
> 
> The reason it's 32 is that old drivers depended on the
> struct being at least 32-byte aligned because they would
> embed structures DMA'd to/from the card in their private
> area and just assumed that would be aligned enough for
> the card's restrictions.
> 
> So setting it to L1_CACHE_BYTES would be wrong, because if
> that happens to be less than 32 it would violate said
> assumption which we are catering to.

How about:

#define NETDEV_ALIGN_MIN 32
#if L1_CACHE_BYTES > NETDEV_ALIGN_MIN
# define NETDEV_ALIGN L1_CACHE_BYTES
#else
# define NETDEV_ALIGN NETDEV_ALIGN_MIN
#endif

rick jones

Re: [RFC] changing value of NETDEV_ALIGN to cacheline size

[Stephen Hemminger]

On Mon, 15 May 2006 14:39:23 -0700
Rick Jones <rick.jones2@hp.com> wrote:

> David S. Miller wrote:
> > From: "Randy.Dunlap" <rdunlap@xenotime.net>
> > Date: Mon, 15 May 2006 08:02:58 -0700
> > 
> > 
> >>>-#define	NETDEV_ALIGN		32
> >>>+#define	NETDEV_ALIGN		L1_CACHE_BYTES
> >>> #define	NETDEV_ALIGN_CONST	(NETDEV_ALIGN - 1)
> >>
> >>I don't know about the fixed value of 32, but if this patch is
> >>accepted, I'd prefer NETDEV_ALIGN_MASK instead of NETDEV_ALIGN_CONST.
> > 
> > 
> > The reason it's 32 is that old drivers depended on the
> > struct being at least 32-byte aligned because they would
> > embed structures DMA'd to/from the card in their private
> > area and just assumed that would be aligned enough for
> > the card's restrictions.
> > 
> > So setting it to L1_CACHE_BYTES would be wrong, because if
> > that happens to be less than 32 it would violate said
> > assumption which we are catering to.
> 
> How about:
> 
> #define NETDEV_ALIGN_MIN 32
> #if L1_CACHE_BYTES > NETDEV_ALIGN_MIN
> # define NETDEV_ALIGN L1_CACHE_BYTES
> #else
> # define NETDEV_ALIGN NETDEV_ALIGN_MIN
> #endif
> 

I can't see how adding more padding could help performance here.
Most drivers reference both parts (netdevice and netdev_priv) in
most code paths. So having the stuff on separate cache lines couldn't
help that much. 

Alternatively, there might be a performance gain if the netdevice stats
structure was rearranged so the rx and tx stats were on different cache lines.

Re: [RFC] changing value of NETDEV_ALIGN to cacheline size

[David S. Miller]

From: Rick Jones <rick.jones2@hp.com>
Date: Mon, 15 May 2006 14:39:23 -0700

> How about:

How about, just leave it alone? :-)

Re: [RFC] changing value of NETDEV_ALIGN to cacheline size

[Rick Jones]

David S. Miller wrote:
> From: Rick Jones <rick.jones2@hp.com>
> Date: Mon, 15 May 2006 14:39:23 -0700
> 
> 
>>How about:
> 
> 
> How about, just leave it alone? :-)

That would work too :) but I guess I figured based on the reason given 
just before I posted, for why setting to L1_CACHE_SIZE was wrong that 
there was an implication that tweaking it might be right.

rick

Re: [RFC] changing value of NETDEV_ALIGN to cacheline size

[Christian Borntraeger]

On Tuesday 16 May 2006 00:49, David S. Miller wrote:
> From: Rick Jones <rick.jones2@hp.com>
> Date: Mon, 15 May 2006 14:39:23 -0700
>
> > How about:
>
> How about, just leave it alone? :-)

Agreed. Currently it only makes a difference with slab debugging, which hurts 
performance no matter what we do.



-- 
Mit freundlichen Grüßen / Best Regards

Christian Borntraeger
Linux Software Engineer zSeries Linux & Virtualization