[RFC][PATCH 0/3] Make loopback device weight twice as little.

[RFC][PATCH 0/3] Make loopback device weight twice as little.

[Pavel Emelyanov]

I noticed, that on a 32bit box with a non-debug config the 
sizeof(struct net_device) is slightly more than 1024. This 
means, that for example loopback device (with sizeof_priv == 0) 
is allocated from the size-2048 cache and thus we have ~1000
wasted bytes.

I know, that this is not that much, all the more so on most of
the setups the lo device is single :) but with the net-namespaced
kernel each namespace has its own loopback device, so this problem
becomes more relevant.

I also guess, that the net_device struct allows many ways to get
shrunk, but I noticed, that is has ~20 pointers on functions for
different purposes, like init, hard_start_xmit, neigh_setup, etc.

The proposal is to _move_ all these callbacks on a new structure
called net_device_ops and put a pointer on it on the net_device.
I've checked how much will the net_device weigh after this and
it turned out, that this difference (80 bytes) is enough to get
the 1024-sized net_device and thus a halved loopback.

To make this change smooth, I propose the following plan (however,
three patches I sent will demonstrate the idea better). 

* First - declare an empty nd_default_ops and make all new devices 
  point on this new ops temporarily. 
* Then replace each call to dev->xxx() with the dev->nd_ops->xxx() 
  one step by step. The ns_ops->xxx() will be the nd_default_ops->xxx
  then and they all will (temporarily) look like
	static void nd_default_xxx(struct net_device *dev, ...)
	{
		if (dev->xxx)
			dev->xxx(dev, ...)
	}
* After this patch switch all the drivers (lots of work :( but it can
  be done driver-by-driver, i.e. without HUGE patches) from on-device
  ops to nd_ops pointer.
* Finally - remove all the ops from the net_device and the default 
  stubs described above.

Does this idea worth being developed further?

Thanks,
Pavel

[RFC][PATCH 1/3] Introduce the net_device_ops structure.

[Pavel Emelyanov]

And fill it with copied from net_device.

Also make newly created devices be assigned to the 
(currently empty) nd_default_ops.

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>

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

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 8b17ed4..33f00ff 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -447,6 +447,43 @@ static inline void napi_synchronize(const struct napi_struct *n)
  *	moves out.
  */
 
+struct net_device_ops {
+	int			(*init)(struct net_device *dev);
+	struct net_device_stats* (*get_stats)(struct net_device *dev);
+	int			(*hard_start_xmit) (struct sk_buff *skb,
+						    struct net_device *dev);
+	void			(*uninit)(struct net_device *dev);
+	void			(*destructor)(struct net_device *dev);
+	int			(*open)(struct net_device *dev);
+	int			(*stop)(struct net_device *dev);
+	void			(*change_rx_flags)(struct net_device *dev,
+						   int flags);
+	void			(*set_rx_mode)(struct net_device *dev);
+	void			(*set_multicast_list)(struct net_device *dev);
+	int			(*set_mac_address)(struct net_device *dev,
+						   void *addr);
+	int			(*validate_addr)(struct net_device *dev);
+	int			(*do_ioctl)(struct net_device *dev,
+					    struct ifreq *ifr, int cmd);
+	int			(*set_config)(struct net_device *dev,
+					      struct ifmap *map);
+	int			(*change_mtu)(struct net_device *dev,
+						int new_mtu);
+	void			(*tx_timeout) (struct net_device *dev);
+
+	void			(*vlan_rx_register)(struct net_device *dev,
+						    struct vlan_group *grp);
+	void			(*vlan_rx_add_vid)(struct net_device *dev,
+						   unsigned short vid);
+	void			(*vlan_rx_kill_vid)(struct net_device *dev,
+						    unsigned short vid);
+	int			(*neigh_setup)(struct net_device *dev,
+						struct neigh_parms *);
+#ifdef CONFIG_NET_POLL_CONTROLLER
+	void                    (*poll_controller)(struct net_device *dev);
+#endif
+};
+
 struct net_device
 {
 
@@ -662,6 +699,8 @@ struct net_device
 	       NETREG_RELEASED,		/* called free_netdev */
 	} reg_state;
 
+	struct net_device_ops	*nd_ops;
+
 	/* Called after device is detached from network. */
 	void			(*uninit)(struct net_device *dev);
 	/* Called after last user reference disappears. */
diff --git a/net/core/dev.c b/net/core/dev.c
index 7aa0112..5e7bf79 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3976,6 +3976,13 @@ static struct net_device_stats *internal_stats(struct net_device *dev)
 	return &dev->stats;
 }
 
+/*
+ * temporary devices operations - to be removed very soon
+ */
+
+static struct net_device_ops nd_default_ops = {
+};
+
 /**
  *	alloc_netdev_mq - allocate network device
  *	@sizeof_priv:	size of private data to allocate space for
@@ -4027,6 +4034,8 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
 	dev->get_stats = internal_stats;
 	netpoll_netdev_init(dev);
 	setup(dev);
+	if (dev->nd_ops == NULL)
+		dev->nd_ops = &nd_default_ops;
 	strcpy(dev->name, name);
 	return dev;
 }
-- 
1.5.3.4

Re: [RFC][PATCH 1/3] Introduce the net_device_ops structure.

[Stephen Hemminger]

On Fri, 04 Apr 2008 18:10:52 +0400
Pavel Emelyanov <xemul@openvz.org> wrote:

> And fill it with copied from net_device.
> 
> Also make newly created devices be assigned to the 
> (currently empty) nd_default_ops.
> 
> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>

Thanks, I started this a while back but never got to the bottom.
Please use const where possible (on dev->nd_ops) and in devices.

Re: [RFC][PATCH 1/3] Introduce the net_device_ops structure.

[Patrick McHardy]

Stephen Hemminger wrote:
> On Fri, 04 Apr 2008 18:10:52 +0400
> Pavel Emelyanov <xemul@openvz.org> wrote:
> 
>> And fill it with copied from net_device.
>>
>> Also make newly created devices be assigned to the 
>> (currently empty) nd_default_ops.
>>
>> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
> 
> Thanks, I started this a while back but never got to the bottom.
> Please use const where possible (on dev->nd_ops) and in devices.


It might make sense to keep hard_start_xmit and hard_header
in struct net_device since VLAN and Bonding overload them.

Re: [RFC][PATCH 1/3] Introduce the net_device_ops structure.

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Fri, 04 Apr 2008 17:54:55 +0200

> Stephen Hemminger wrote:
> > On Fri, 04 Apr 2008 18:10:52 +0400
> > Pavel Emelyanov <xemul@openvz.org> wrote:
> > 
> >> And fill it with copied from net_device.
> >>
> >> Also make newly created devices be assigned to the 
> >> (currently empty) nd_default_ops.
> >>
> >> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
> > 
> > Thanks, I started this a while back but never got to the bottom.
> > Please use const where possible (on dev->nd_ops) and in devices.
> 
> 
> It might make sense to keep hard_start_xmit and hard_header
> in struct net_device since VLAN and Bonding overload them.

I echo the sentiments of Stephen (use const as much as possible)
and Patrick here.

In particular what Patrick is mentioning deserves special emphasis.

There are so many weird cases that take an existing device and then
overload one of the method pointers arbitrarily.  And I know this is
not at all limited to hard_start_xmit and hard_header.

It may therefore be very difficult to get things all the way over to
nd_ops except for a very small number pointers.  And unfortunately, if
this cannot be overcome, it limits the usefullness of the change. :-/

I encourage this work, nonetheless.

[RFC][PATCH 2/3] Move net_device->init callback on net_device_ops.

[Pavel Emelyanov]

This is how all the other temporary operations will be done.
The return code is still 0 in case init is not set by driver.

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>

---
 net/core/dev.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 5e7bf79..a01c078 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3701,8 +3701,8 @@ int register_netdevice(struct net_device *dev)
 	dev->iflink = -1;
 
 	/* Init, if this function is available */
-	if (dev->init) {
-		ret = dev->init(dev);
+	if (dev->nd_ops->init) {
+		ret = dev->nd_ops->init(dev);
 		if (ret) {
 			if (ret > 0)
 				ret = -EIO;
@@ -3980,7 +3980,16 @@ static struct net_device_stats *internal_stats(struct net_device *dev)
  * temporary devices operations - to be removed very soon
  */
 
+static int nd_default_init(struct net_device *dev)
+{
+	if (dev->init)
+		return dev->init(dev);
+	else
+		return 0;
+}
+
 static struct net_device_ops nd_default_ops = {
+	.init			= nd_default_init,
 };
 
 /**
-- 
1.5.3.4

[RFC][PATCH 3/3] Move net_device->uninit callback on net_device_ops.

[Pavel Emelyanov]

The void calls are even simpler.

Plus ~18 more patches and all the ops will be moved on the
net_device_ops and it will be possible to start converting
device drivers...

So, what is your opinion about all this?

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>

---
 net/core/dev.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index a01c078..3428534 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3644,8 +3644,8 @@ static void rollback_registered(struct net_device *dev)
 	 */
 	dev_addr_discard(dev);
 
-	if (dev->uninit)
-		dev->uninit(dev);
+	if (dev->nd_ops->uninit)
+		dev->nd_ops->uninit(dev);
 
 	/* Notifier chain MUST detach us from master device. */
 	BUG_TRAP(!dev->master);
@@ -3804,8 +3804,8 @@ out:
 	return ret;
 
 err_uninit:
-	if (dev->uninit)
-		dev->uninit(dev);
+	if (dev->nd_ops->uninit)
+		dev->nd_ops->uninit(dev);
 	goto out;
 }
 
@@ -3988,8 +3988,15 @@ static int nd_default_init(struct net_device *dev)
 		return 0;
 }
 
+static void nd_default_uninit(struct net_device *dev)
+{
+	if (dev->uninit)
+		dev->uninit(dev);
+}
+
 static struct net_device_ops nd_default_ops = {
 	.init			= nd_default_init,
+	.uninit			= nd_default_uninit,
 };
 
 /**
-- 
1.5.3.4