[RFC] restore netdev_priv optimization

[RFC] restore netdev_priv optimization

[Stephen Hemminger]

Compile tested only!!!

Fix optimization of netdev_priv() lost by the  addition of multiqueue.
Move the variable size subqueues to after the constant size priv area.

When putting back the old netdev_priv() code, I tried to make it
clearer by using roundup() and ALIGN() macros.

--- a/include/linux/netdevice.h	2007-08-17 12:08:51.000000000 -0400
+++ b/include/linux/netdevice.h	2007-08-17 12:48:03.000000000 -0400
@@ -575,16 +575,15 @@ struct net_device
 
 	/* The TX queue control structures */
 	unsigned int			egress_subqueue_count;
-	struct net_device_subqueue	egress_subqueue[1];
+	struct net_device_subqueue	*egress_subqueue;
 };
 #define to_net_dev(d) container_of(d, struct net_device, dev)
 
 #define	NETDEV_ALIGN		32
-#define	NETDEV_ALIGN_CONST	(NETDEV_ALIGN - 1)
 
 static inline void *netdev_priv(const struct net_device *dev)
 {
-	return dev->priv;
+	return (void *)dev + ALIGN(sizeof(struct net_device), NETDEV_ALIGN);
 }
 
 #define SET_MODULE_OWNER(dev) do { } while (0)
--- a/net/core/dev.c	2007-08-17 09:02:57.000000000 -0400
+++ b/net/core/dev.c	2007-08-17 12:48:30.000000000 -0400
@@ -3706,7 +3706,22 @@ EXPORT_SYMBOL(dev_get_stats);
  *	@queue_count:	the number of subqueues to allocate
  *
  *	Allocates a struct net_device with private data area for driver use
- *	and performs basic initialization.  Also allocates subquue structs
+ *	and performs basic initialization.
+ *
+ *      Layout:
+ *              allocation->+------------+
+ *                          | (pad)      | dev->padded
+ *                   dev -->+------------+ -- (32 byte boundary)
+ *                          | net_device |
+ *      netdev_priv(dev) -->+------------+ -- (32 byte boundary)
+ *                          | device     |
+ *                          | private    |
+ *			    |            |
+ * dev->egress_subqueue  -->+------------+ -- (32 byte boundary)
+ *                          | Tx queue(s)|
+ *                          +------------+
+ *
+ * Also allocates subqueue structs
  *	for each queue on the device at the end of the netdevice.
  */
 struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
@@ -3719,10 +3734,9 @@ struct net_device *alloc_netdev_mq(int s
 	BUG_ON(strlen(name) >= sizeof(dev->name));
 
 	/* ensure 32-byte alignment of both the device and private area */
-	alloc_size = (sizeof(*dev) + NETDEV_ALIGN_CONST +
-		     (sizeof(struct net_device_subqueue) * (queue_count - 1))) &
-		     ~NETDEV_ALIGN_CONST;
-	alloc_size += sizeof_priv + NETDEV_ALIGN_CONST;
+	alloc_size = roundup(sizeof(*dev), NETDEV_ALIGN);
+	alloc_size += roundup(sizeof_priv, NETDEV_ALIGN);
+	alloc_size += sizeof(struct net_device_subqueue) * queue_count;
 
 	p = kzalloc(alloc_size, GFP_KERNEL);
 	if (!p) {
@@ -3730,19 +3744,20 @@ struct net_device *alloc_netdev_mq(int s
 		return NULL;
 	}
 
-	dev = (struct net_device *)
-		(((long)p + NETDEV_ALIGN_CONST) & ~NETDEV_ALIGN_CONST);
+	/* Pad so that network device is on cache line boundary	 */
+	dev = (struct net_device *) ALIGN((unsigned long) p, NETDEV_ALIGN);
 	dev->padded = (char *)dev - (char *)p;
 
-	if (sizeof_priv) {
-		dev->priv = ((char *)dev +
-			     ((sizeof(struct net_device) +
-			       (sizeof(struct net_device_subqueue) *
-				(queue_count - 1)) + NETDEV_ALIGN_CONST)
-			      & ~NETDEV_ALIGN_CONST));
-	}
+	if (sizeof_priv)
+		dev->priv = netdev_priv(dev);
 
+	/* Put subqueue(s) which are variable size after fix sized priv area */
 	dev->egress_subqueue_count = queue_count;
+	dev->egress_subqueue = ALIGN((unsigned long)(netdev_priv(dev) + sizeof_priv),
+				     NETDEV_ALIGN);
+
+	pr_debug("alloc_netdev_mq: p=%p dev=%p priv=%p q=%p\n",
+	       p, dev, dev->priv, dev->egress_subqueue);
 
 	setup(dev);
 	strcpy(dev->name, name);

Re: [RFC] restore netdev_priv optimization

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Fri, 17 Aug 2007 15:40:22 -0700

> Compile tested only!!!

Obviously.  The first loopback transmit is guarenteed to crash.

> Fix optimization of netdev_priv() lost by the  addition of multiqueue.
> Move the variable size subqueues to after the constant size priv area.
> 
> When putting back the old netdev_priv() code, I tried to make it
> clearer by using roundup() and ALIGN() macros.
> 
> --- a/include/linux/netdevice.h	2007-08-17 12:08:51.000000000 -0400
> +++ b/include/linux/netdevice.h	2007-08-17 12:48:03.000000000 -0400
> @@ -575,16 +575,15 @@ struct net_device
>  
>  	/* The TX queue control structures */
>  	unsigned int			egress_subqueue_count;
> -	struct net_device_subqueue	egress_subqueue[1];
> +	struct net_device_subqueue	*egress_subqueue;
>  };

This just trades off the dev->priv dereference for a
dev->egress_subqueue dereference.  I bet they occur about equally in
the data paths, at least on transmit.

And this also breaks loopback again, which uses a static struct netdev
in the kernel image, it doesn't use alloc_netdev(), so egress_subqueue
of loopback will be NULL.

Re: [RFC] restore netdev_priv optimization

[Stephen Hemminger]

On Fri, 17 Aug 2007 16:04:09 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <shemminger@linux-foundation.org>
> Date: Fri, 17 Aug 2007 15:40:22 -0700
> 
> > Compile tested only!!!
> 
> Obviously.  The first loopback transmit is guarenteed to crash.

That is fixable.

> > Fix optimization of netdev_priv() lost by the  addition of multiqueue.
> > Move the variable size subqueues to after the constant size priv area.
> > 
> > When putting back the old netdev_priv() code, I tried to make it
> > clearer by using roundup() and ALIGN() macros.
> > 
> > --- a/include/linux/netdevice.h	2007-08-17 12:08:51.000000000 -0400
> > +++ b/include/linux/netdevice.h	2007-08-17 12:48:03.000000000 -0400
> > @@ -575,16 +575,15 @@ struct net_device
> >  
> >  	/* The TX queue control structures */
> >  	unsigned int			egress_subqueue_count;
> > -	struct net_device_subqueue	egress_subqueue[1];
> > +	struct net_device_subqueue	*egress_subqueue;
> >  };
> 
> This just trades off the dev->priv dereference for a
> dev->egress_subqueue dereference.  I bet they occur about equally in
> the data paths, at least on transmit.

The subqueue is only referenced in start/stop queue and that only happens
once per packet on normal tx, and only if multiqueue is used.

The existing multiqueue penalizes all devices, not just multiqueue devices.

> And this also breaks loopback again, which uses a static struct netdev
> in the kernel image, it doesn't use alloc_netdev(), so egress_subqueue
> of loopback will be NULL.

That can be overcome.

-- 
Stephen Hemminger <shemminger@linux-foundation.org>

Re: [RFC] restore netdev_priv optimization

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Fri, 17 Aug 2007 16:19:28 -0700

> The subqueue is only referenced in start/stop queue and that only happens
> once per packet on normal tx, and only if multiqueue is used.

If it only happens when multiqueue, then why does loopback need
at least one queue entry there even though it's not multiqueue? :-)

This thing is deref'd regardless of multi-queue.

Once per TX packet is a lot, and it's the same amount of derefs
as dev->priv will have on the TX path.

[RFC] restore netdev_priv optimization (planb)

[Stephen Hemminger]

Fix optimization of netdev_priv() lost by the addition of multiqueue.

Only configurations that define MULITQUEUE need the extra overhead in
netdevice structure and the loss of the netdev_priv optimization.

Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>


--- a/include/linux/netdevice.h	2007-08-17 16:45:39.000000000 -0700
+++ b/include/linux/netdevice.h	2007-08-17 17:31:51.000000000 -0700
@@ -574,17 +574,22 @@ struct net_device
 	const struct rtnl_link_ops *rtnl_link_ops;
 
 	/* The TX queue control structures */
-	unsigned int			egress_subqueue_count;
+#ifdef CONFIG_NETDEVICES_MULTIQUEUE
+	unsigned short			egress_subqueue_count;
 	struct net_device_subqueue	egress_subqueue[1];
+#endif
 };
 #define to_net_dev(d) container_of(d, struct net_device, dev)
 
 #define	NETDEV_ALIGN		32
-#define	NETDEV_ALIGN_CONST	(NETDEV_ALIGN - 1)
 
 static inline void *netdev_priv(const struct net_device *dev)
 {
+#ifdef CONFIG_NETDEVICES_MULTIQUEUE
 	return dev->priv;
+#else
+	return (void *)dev + ALIGN(sizeof(struct net_device), NETDEV_ALIGN);
+#endif
 }
 
 #define SET_MODULE_OWNER(dev) do { } while (0)
--- a/net/core/dev.c	2007-08-17 16:45:39.000000000 -0700
+++ b/net/core/dev.c	2007-08-17 17:44:02.000000000 -0700
@@ -3706,7 +3706,7 @@ EXPORT_SYMBOL(dev_get_stats);
  *	@queue_count:	the number of subqueues to allocate
  *
  *	Allocates a struct net_device with private data area for driver use
- *	and performs basic initialization.  Also allocates subquue structs
+ *	and performs basic initialization.  Also allocates subqueue structs
  *	for each queue on the device at the end of the netdevice.
  */
 struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
@@ -3714,15 +3714,20 @@ struct net_device *alloc_netdev_mq(int s
 {
 	void *p;
 	struct net_device *dev;
-	int alloc_size;
+	size_t dev_size, alloc_size;
 
 	BUG_ON(strlen(name) >= sizeof(dev->name));
 
+#ifndef CONFIG_NETDEVICES_MULTIQUEUE
+	BUG_ON(queue_count > 1);
+#endif
+
 	/* ensure 32-byte alignment of both the device and private area */
-	alloc_size = (sizeof(*dev) + NETDEV_ALIGN_CONST +
-		     (sizeof(struct net_device_subqueue) * (queue_count - 1))) &
-		     ~NETDEV_ALIGN_CONST;
-	alloc_size += sizeof_priv + NETDEV_ALIGN_CONST;
+	dev_size = sizeof(*dev)
+		+ (queue_count - 1) * sizeof(struct net_device_subqueue);
+
+	alloc_size = roundup(dev_size, NETDEV_ALIGN);
+	alloc_size += sizeof_priv + NETDEV_ALIGN - 1;
 
 	p = kzalloc(alloc_size, GFP_KERNEL);
 	if (!p) {
@@ -3730,19 +3735,17 @@ struct net_device *alloc_netdev_mq(int s
 		return NULL;
 	}
 
-	dev = (struct net_device *)
-		(((long)p + NETDEV_ALIGN_CONST) & ~NETDEV_ALIGN_CONST);
+	dev = (struct net_device *) ALIGN((unsigned long) p, NETDEV_ALIGN);
 	dev->padded = (char *)dev - (char *)p;
 
-	if (sizeof_priv) {
-		dev->priv = ((char *)dev +
-			     ((sizeof(struct net_device) +
-			       (sizeof(struct net_device_subqueue) *
-				(queue_count - 1)) + NETDEV_ALIGN_CONST)
-			      & ~NETDEV_ALIGN_CONST));
-	}
-
+#ifdef CONFIG_NETDEVICES_MULTIQUEUE
 	dev->egress_subqueue_count = queue_count;
+	if (sizeof_priv)
+		dev->priv = ALIGN((unsigned long)dev + dev_size, NETDEV_ALIGN);
+#else
+	if (sizeof_priv)
+		dev->priv = netdev_priv(dev);
+#endif
 
 	setup(dev);
 	strcpy(dev->name, name);

Re: [RFC] restore netdev_priv optimization (planb)

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Fri, 17 Aug 2007 17:49:09 -0700

> Fix optimization of netdev_priv() lost by the addition of multiqueue.
> 
> Only configurations that define MULITQUEUE need the extra overhead in
> netdevice structure and the loss of the netdev_priv optimization.
> 
> Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>

Every distribution vendor is going to turn MULTIQUEUE on.  Therefore
%99 of Linux users will not see any gain from your patch.

You're walking around in a dark room and hitting walls every
few seconds.  Take a moment and think about how to really deal
with this generically for a second before churning out another
patch.

For example, how about making the multiqueue limit fixed, say at 64
queues, and declare the egress_queues as an array of 64 entries.  Then
you can get constant pointer formation for both the netdev priv and
the queues.

This will also basically eliminate all of the alloc_mqueue() logic,
since struct netdev will now always be a fixed size.

And, amazingly, this deals with the loopback issue transparently as
well. :-)

It would also be nice to just get rid of netdev->priv, it will prevent
more misuses like the cxgb3 case, for example.

Re: [RFC] restore netdev_priv optimization (planb)

[Kok, Auke]

David Miller wrote:
> From: Stephen Hemminger <shemminger@linux-foundation.org>
> Date: Fri, 17 Aug 2007 17:49:09 -0700
> 
>> Fix optimization of netdev_priv() lost by the addition of multiqueue.
>>
>> Only configurations that define MULITQUEUE need the extra overhead in
>> netdevice structure and the loss of the netdev_priv optimization.
>>
>> Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>
> 
> Every distribution vendor is going to turn MULTIQUEUE on.  Therefore
> %99 of Linux users will not see any gain from your patch.
> 
> You're walking around in a dark room and hitting walls every
> few seconds.  Take a moment and think about how to really deal
> with this generically for a second before churning out another
> patch.
> 
> For example, how about making the multiqueue limit fixed, say at 64
> queues, and declare the egress_queues as an array of 64 entries.  Then
> you can get constant pointer formation for both the netdev priv and
> the queues.

this sounds highly optimistic ("64 queues is enough for everyone"?) and probably 
will be quickly outdated by both hardware and demand...

Auke

Re: [RFC] restore netdev_priv optimization (planb)

[Stephen Hemminger]

On Fri, 17 Aug 2007 18:21:25 -0700
"Kok, Auke" <auke-jan.h.kok@intel.com> wrote:

> David Miller wrote:
> > From: Stephen Hemminger <shemminger@linux-foundation.org>
> > Date: Fri, 17 Aug 2007 17:49:09 -0700
> > 
> >> Fix optimization of netdev_priv() lost by the addition of multiqueue.
> >>
> >> Only configurations that define MULITQUEUE need the extra overhead in
> >> netdevice structure and the loss of the netdev_priv optimization.
> >>
> >> Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>
> > 
> > Every distribution vendor is going to turn MULTIQUEUE on.  Therefore
> > %99 of Linux users will not see any gain from your patch.
> > 
> > You're walking around in a dark room and hitting walls every
> > few seconds.  Take a moment and think about how to really deal
> > with this generically for a second before churning out another
> > patch.
> > 
> > For example, how about making the multiqueue limit fixed, say at 64
> > queues, and declare the egress_queues as an array of 64 entries.  Then
> > you can get constant pointer formation for both the netdev priv and
> > the queues.
> 
> this sounds highly optimistic ("64 queues is enough for everyone"?) and probably 
> will be quickly outdated by both hardware and demand...
> 

Plan C was replacing MULTIQUEUE boolean with a int value 1 ... 256.
All this was a one day "what if" exercise, not really a big churn..

-- 
Stephen Hemminger <shemminger@linux-foundation.org>

Re: [RFC] restore netdev_priv optimization (planb)

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Fri, 17 Aug 2007 18:28:07 -0700

> Plan C was replacing MULTIQUEUE boolean with a int value 1 ... 256.
> All this was a one day "what if" exercise, not really a big churn..

Yes, that's another reasonable approach.

Re: [RFC] restore netdev_priv optimization (planb)

[David Miller]

From: "Kok, Auke" <auke-jan.h.kok@intel.com>
Date: Fri, 17 Aug 2007 18:21:25 -0700

> this sounds highly optimistic ("64 queues is enough for everyone"?)
> and probably will be quickly outdated by both hardware and demand...

As such drivers appear in the tree we can adjust the value.

Even the most aggressively multi-queued virtualization and 10GB
ethernet chips I am aware of, both in production and in development,
do not exceed this limit.

Since you think this is worth complaining about, you must know of some
exceptions? :-)

Re: [RFC] restore netdev_priv optimization (planb)

[Kok, Auke]

David Miller wrote:
> From: "Kok, Auke" <auke-jan.h.kok@intel.com>
> Date: Fri, 17 Aug 2007 18:21:25 -0700
> 
>> this sounds highly optimistic ("64 queues is enough for everyone"?)
>> and probably will be quickly outdated by both hardware and demand...
> 
> As such drivers appear in the tree we can adjust the value.
> 
> Even the most aggressively multi-queued virtualization and 10GB
> ethernet chips I am aware of, both in production and in development,
> do not exceed this limit.
> 
> Since you think this is worth complaining about, you must know of some
> exceptions? :-)

I actually don't, but I assume that demand for queues will quickly increase once 
the feature becomes available. e.g. in e1000 hardware (pci-e) we support 2, and 
this is really old hardware already (laugh), 82575 has 4.

the ixgbe driver me and Ayyappan posted already supports 64 rx queues and 32 tx...

I can only expect the next generation to take a bigger jump and implement at 
least 128 queues... :)

Auke

Re: [RFC] restore netdev_priv optimization

[Benjamin Thery]

Hi,

David Miller wrote:
> From: Stephen Hemminger <shemminger@linux-foundation.org>
> Date: Fri, 17 Aug 2007 15:40:22 -0700
> 
>> Compile tested only!!!
> 
> Obviously.  The first loopback transmit is guarenteed to crash.
> 
> [...]
> 
> And this also breaks loopback again, which uses a static struct netdev
> in the kernel image, it doesn't use alloc_netdev(), so egress_subqueue
> of loopback will be NULL.

Talking about loopback, don't you think it could be the right time
to make it behave like any other kind of net devices, and allocate it
dynamically.

Having a dynamically allocated loopback could make maintenance easier
(removing special cases).
Also this is something we'll need to support multiple loopbacks for 
example for network namespaces.

Eric Biederman has written a nice patch that does this.
I'm using it on 2.6.23-rc2.

Benjamin

-- 
B e n j a m i n   T h e r y  - BULL/DT/Open Software R&D

    http://www.bull.com