Re: [RFC] changing value of NETDEV_ALIGN to cacheline size
On Tuesday 16 May 2006 00:49, David S. Miller wrote: From: Rick Jones [EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC] changing value of NETDEV_ALIGN to cacheline size
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 [EMAIL PROTECTED] 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 - +++ b/include/linux/netdevice.h 15 May 2006 11:06:05 - @@ -504,7 +504,7 @@ struct class_device class_dev; }; -#defineNETDEV_ALIGN32 +#defineNETDEV_ALIGNL1_CACHE_BYTES #defineNETDEV_ALIGN_CONST (NETDEV_ALIGN - 1) static inline void *netdev_priv(struct net_device *dev) --- a/net/core/dev.c4 Apr 2006 07:25:50 - +++ b/net/core/dev.c15 May 2006 11:06:05 - @@ -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; - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] changing value of NETDEV_ALIGN to cacheline size
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 [EMAIL PROTECTED] 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 - +++ b/include/linux/netdevice.h 15 May 2006 11:06:05 - @@ -504,7 +504,7 @@ struct class_device class_dev; }; -#define NETDEV_ALIGN32 +#define NETDEV_ALIGNL1_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 - +++ b/net/core/dev.c 15 May 2006 11:06:05 - @@ -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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] changing value of NETDEV_ALIGN to cacheline size
David S. Miller wrote: From: Randy.Dunlap [EMAIL PROTECTED] Date: Mon, 15 May 2006 08:02:58 -0700 -#defineNETDEV_ALIGN32 +#defineNETDEV_ALIGNL1_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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] changing value of NETDEV_ALIGN to cacheline size
David S. Miller wrote: From: Rick Jones [EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
