[patch 3/4] Configure out ethtool support

[patch 3/4] Configure out ethtool support

[Thomas Petazzoni]

[-- Attachment #1: configure-out-ethtool-support --]
[-- Type: text/plain, Size: 5790 bytes --]

This patchs add the CONFIG_ETHTOOL option which allows to remove
support for ethtool, not necessarly used on embedded systems. As this
is a size-reduction option, it depends on CONFIG_EMBEDDED. It allows
to save ~6 kilobytes of kernel code:

   text	   data	    bss	    dec	    hex	filename
1258447	 123592	 212992	1595031	 185697	vmlinux
1252147	 123592	 212992	1588731	 183dfb	vmlinux.new
  -6300       0       0   -6300   -189C +/-

Bonding and bridging both depends on Ethtool functionnality, so
ETHTOOL is selected automatically when either bonding and bridging are
selected.

Question: should we also remove ethtool-related functions from all
network drivers ?

This patch has been originally written by Matt Mackall
<mpm@selenic.com>, and is part of the Linux Tiny project.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Signed-off-by: Matt Mackall <mpm@selenic.com>
Cc: jgarzik@pobox.com
Cc: netdev@vger.kernel.org
Cc: davem@davemloft.net
Cc: mpm@selenic.com
Cc: akpm@linux-foundation.org

---
 drivers/net/Kconfig     |    1 +
 include/linux/ethtool.h |   16 ++++++++++++++++
 init/Kconfig            |    8 ++++++++
 net/bridge/Kconfig      |    1 +
 net/core/Makefile       |    3 ++-
 net/core/dev.c          |    4 ++++
 6 files changed, 32 insertions(+), 1 deletion(-)

Index: linuxdev/drivers/net/Kconfig
===================================================================
--- linuxdev.orig/drivers/net/Kconfig
+++ linuxdev/drivers/net/Kconfig
@@ -61,6 +61,7 @@
 config BONDING
 	tristate "Bonding driver support"
 	depends on INET
+	select ETHTOOL
 	---help---
 	  Say 'Y' or 'M' if you wish to be able to 'bond' multiple Ethernet
 	  Channels together. This is called 'Etherchannel' by Cisco,
Index: linuxdev/include/linux/ethtool.h
===================================================================
--- linuxdev.orig/include/linux/ethtool.h
+++ linuxdev/include/linux/ethtool.h
@@ -283,6 +283,7 @@
 struct net_device;
 
 /* Some generic methods drivers may use in their ethtool_ops */
+#ifdef CONFIG_ETHTOOL
 u32 ethtool_op_get_link(struct net_device *dev);
 u32 ethtool_op_get_tx_csum(struct net_device *dev);
 int ethtool_op_set_tx_csum(struct net_device *dev, u32 data);
@@ -296,6 +297,21 @@
 int ethtool_op_set_ufo(struct net_device *dev, u32 data);
 u32 ethtool_op_get_flags(struct net_device *dev);
 int ethtool_op_set_flags(struct net_device *dev, u32 data);
+#else
+static inline u32 ethtool_op_get_link(struct net_device *dev) { return 0; }
+static inline u32 ethtool_op_get_tx_csum(struct net_device *dev) { return 0; }
+static inline int ethtool_op_set_tx_csum(struct net_device *dev, u32 data) { return 0; }
+static inline int ethtool_op_set_tx_hw_csum(struct net_device *dev, u32 data) { return 0; }
+static inline int ethtool_op_set_tx_ipv6_csum(struct net_device *dev, u32 data) { return 0; }
+static inline u32 ethtool_op_get_sg(struct net_device *dev) { return 0; }
+static inline int ethtool_op_set_sg(struct net_device *dev, u32 data) { return 0; }
+static inline u32 ethtool_op_get_tso(struct net_device *dev) { return 0; }
+static inline int ethtool_op_set_tso(struct net_device *dev, u32 data) { return 0; }
+static inline u32 ethtool_op_get_ufo(struct net_device *dev) { return 0; }
+static inline int ethtool_op_set_ufo(struct net_device *dev, u32 data) { return 0; }
+static inline u32 ethtool_op_get_flags(struct net_device *dev) { return 0; }
+static inline int ethtool_op_set_flags(struct net_device *dev, u32 data) { return 0; }
+#endif
 
 /**
  * &ethtool_ops - Alter and report network device settings
Index: linuxdev/init/Kconfig
===================================================================
--- linuxdev.orig/init/Kconfig
+++ linuxdev/init/Kconfig
@@ -740,6 +740,14 @@
           for filesystems like NFS and for the flock() system
           call. Disabling this option saves about 11k.
 
+config ETHTOOL
+	bool "Enable ethtool support" if EMBEDDED
+	depends on NET
+	default y
+	help
+	  Disabling this option removes support for configuring
+	  ethernet device features via ethtool. Saves about 6k.
+
 config VM_EVENT_COUNTERS
 	default y
 	bool "Enable VM event counters for /proc/vmstat" if EMBEDDED
Index: linuxdev/net/bridge/Kconfig
===================================================================
--- linuxdev.orig/net/bridge/Kconfig
+++ linuxdev/net/bridge/Kconfig
@@ -6,6 +6,7 @@
 	tristate "802.1d Ethernet Bridging"
 	select LLC
 	select STP
+	select ETHTOOL
 	---help---
 	  If you say Y here, then your Linux box will be able to act as an
 	  Ethernet bridge, which means that the different Ethernet segments it
Index: linuxdev/net/core/Makefile
===================================================================
--- linuxdev.orig/net/core/Makefile
+++ linuxdev/net/core/Makefile
@@ -7,10 +7,11 @@
 
 obj-$(CONFIG_SYSCTL) += sysctl_net_core.o
 
-obj-y		     += dev.o ethtool.o dev_mcast.o dst.o netevent.o \
+obj-y		     += dev.o dev_mcast.o dst.o netevent.o \
 			neighbour.o rtnetlink.o utils.o link_watch.o filter.o
 
 obj-$(CONFIG_XFRM) += flow.o
+obj-$(CONFIG_ETHTOOL) += ethtool.o
 obj-y += net-sysfs.o
 obj-$(CONFIG_NET_PKTGEN) += pktgen.o
 obj-$(CONFIG_NETPOLL) += netpoll.o
Index: linuxdev/net/core/dev.c
===================================================================
--- linuxdev.orig/net/core/dev.c
+++ linuxdev/net/core/dev.c
@@ -3669,6 +3669,7 @@
 			return ret;
 
 		case SIOCETHTOOL:
+#ifdef CONFIG_ETHTOOL
 			dev_load(net, ifr.ifr_name);
 			rtnl_lock();
 			ret = dev_ethtool(net, &ifr);
@@ -3681,6 +3682,9 @@
 					ret = -EFAULT;
 			}
 			return ret;
+#else
+			return -EINVAL;
+#endif
 
 		/*
 		 *	These ioctl calls:

-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers and embedded Linux development,
consulting, training and support.
http://free-electrons.com

[patch 4/4] Configure out IGMP support

[Thomas Petazzoni]

[-- Attachment #1: configure-out-igmp-support --]
[-- Type: text/plain, Size: 5718 bytes --]

This patchs adds the CONFIG_IGMP option which allows to remove support
for the Internet Group Management Protocol, used in
multicast. Multicast is not necessarly used by applications,
particularly on embedded devices. As this is a size-reduction option,
it depends on CONFIG_EMBEDDED. It allows to save ~10 kilobytes of
kernel code/data:

   text	   data	    bss	    dec	    hex	filename
1718857	 143672	 221184	2083713	 1fcb81	vmlinux
1708838	 143640	 221184	2073662	 1fa43e	vmlinux.new
 -10019     -32       0  -10051   -2743 +/-

This patch has been originally written by Matt Mackall
<mpm@selenic.com>, and is part of the Linux Tiny project.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Signed-off-by: Matt Mackall <mpm@selenic.com>
Cc: netdev@vger.kernel.org
Cc: davem@davemloft.net
Cc: mpm@selenic.com
Cc: akpm@linux-foundation.org

---
 include/linux/igmp.h       |   20 ++++++++++++++++++++
 init/Kconfig               |    8 ++++++++
 net/ipv4/Makefile          |    3 ++-
 net/ipv4/af_inet.c         |    2 --
 net/ipv4/ip_sockglue.c     |    4 ++++
 net/ipv4/sysctl_net_ipv4.c |    2 ++
 6 files changed, 36 insertions(+), 3 deletions(-)

Index: linuxdev/include/linux/igmp.h
===================================================================
--- linuxdev.orig/include/linux/igmp.h
+++ linuxdev/include/linux/igmp.h
@@ -215,6 +215,7 @@
 #define IGMPV3_QQIC(value) IGMPV3_EXP(0x80, 4, 3, value)
 #define IGMPV3_MRC(value) IGMPV3_EXP(0x80, 4, 3, value)
 
+#ifdef CONFIG_IGMP
 extern int ip_check_mc(struct in_device *dev, __be32 mc_addr, __be32 src_addr, u16 proto);
 extern int igmp_rcv(struct sk_buff *);
 extern int ip_mc_join_group(struct sock *sk, struct ip_mreqn *imr);
@@ -235,6 +236,25 @@
 extern void ip_mc_dec_group(struct in_device *in_dev, __be32 addr);
 extern void ip_mc_inc_group(struct in_device *in_dev, __be32 addr);
 extern void ip_mc_rejoin_group(struct ip_mc_list *im);
+#else /* !CONFIG_IGMP */
+#define ip_check_mc(a, b, c, d) (0)
+#define igmp_rcv(a) (0)
+#define ip_mc_join_group(a, b) (0)
+#define ip_mc_leave_group(a, b) (0)
+#define ip_mc_drop_socket(a)
+#define ip_mc_source(a, b, c, d, e) (0)
+#define ip_mc_msfilter(a, b, c) (0)
+#define ip_mc_msfget(a, b, c, d) (0)
+#define ip_mc_gsfget(a, b, c, d) (0)
+#define ip_mc_sf_allow(a, b, c, d) (0)
+#define ip_mc_init_dev(a)
+#define ip_mc_destroy_dev(a)
+#define ip_mc_up(a)
+#define ip_mc_down(a)
+#define ip_mc_dec_group(a)
+#define ip_mc_inc_group(a)
+#define ip_mc_rejoin_group(a)
+#endif /* CONFIG_IGMP */
 
 #endif
 #endif
Index: linuxdev/init/Kconfig
===================================================================
--- linuxdev.orig/init/Kconfig
+++ linuxdev/init/Kconfig
@@ -748,6 +748,14 @@
 	  Disabling this option removes support for configuring
 	  ethernet device features via ethtool. Saves about 6k.
 
+config IGMP
+	depends on INET
+	bool "Enable IGMP support" if EMBEDDED && !IP_MULTICAST
+	default y
+	help
+	  Disabling this option removes support for the Internet group
+          management protocol, used for multicast. Saves about 10k.
+
 config VM_EVENT_COUNTERS
 	default y
 	bool "Enable VM event counters for /proc/vmstat" if EMBEDDED
Index: linuxdev/net/ipv4/Makefile
===================================================================
--- linuxdev.orig/net/ipv4/Makefile
+++ linuxdev/net/ipv4/Makefile
@@ -9,13 +9,14 @@
 	     tcp.o tcp_input.o tcp_output.o tcp_timer.o tcp_ipv4.o \
 	     tcp_minisocks.o tcp_cong.o \
 	     datagram.o raw.o udp.o udplite.o \
-	     arp.o icmp.o devinet.o af_inet.o  igmp.o \
+	     arp.o icmp.o devinet.o af_inet.o \
 	     fib_frontend.o fib_semantics.o \
 	     inet_fragment.o
 
 obj-$(CONFIG_SYSCTL) += sysctl_net_ipv4.o
 obj-$(CONFIG_IP_FIB_HASH) += fib_hash.o
 obj-$(CONFIG_IP_FIB_TRIE) += fib_trie.o
+obj-$(CONFIG_IGMP) += igmp.o
 obj-$(CONFIG_PROC_FS) += proc.o
 obj-$(CONFIG_IP_MULTIPLE_TABLES) += fib_rules.o
 obj-$(CONFIG_IP_MROUTE) += ipmr.o
Index: linuxdev/net/ipv4/af_inet.c
===================================================================
--- linuxdev.orig/net/ipv4/af_inet.c
+++ linuxdev/net/ipv4/af_inet.c
@@ -115,8 +115,6 @@
 #include <linux/mroute.h>
 #endif
 
-extern void ip_mc_drop_socket(struct sock *sk);
-
 /* The inetsw table contains everything that inet_create needs to
  * build a new socket.
  */
Index: linuxdev/net/ipv4/ip_sockglue.c
===================================================================
--- linuxdev.orig/net/ipv4/ip_sockglue.c
+++ linuxdev/net/ipv4/ip_sockglue.c
@@ -640,6 +640,7 @@
 			err = ip_mc_leave_group(sk, &mreq);
 		break;
 	}
+#ifdef CONFIG_IGMP
 	case IP_MSFILTER:
 	{
 		extern int sysctl_igmp_max_msf;
@@ -677,6 +678,7 @@
 		kfree(msf);
 		break;
 	}
+#endif
 	case IP_BLOCK_SOURCE:
 	case IP_UNBLOCK_SOURCE:
 	case IP_ADD_SOURCE_MEMBERSHIP:
@@ -794,6 +796,7 @@
 				   greqs.gsr_interface);
 		break;
 	}
+#ifdef CONFIG_IGMP
 	case MCAST_MSFILTER:
 	{
 		extern int sysctl_igmp_max_msf;
@@ -860,6 +863,7 @@
 		kfree(gsf);
 		break;
 	}
+#endif
 	case IP_ROUTER_ALERT:
 		err = ip_ra_control(sk, val ? 1 : 0, NULL);
 		break;
Index: linuxdev/net/ipv4/sysctl_net_ipv4.c
===================================================================
--- linuxdev.orig/net/ipv4/sysctl_net_ipv4.c
+++ linuxdev/net/ipv4/sysctl_net_ipv4.c
@@ -412,6 +412,7 @@
 	},
 
 #endif
+#ifdef CONFIG_IGMP
 	{
 		.ctl_name	= NET_IPV4_IGMP_MAX_MSF,
 		.procname	= "igmp_max_msf",
@@ -420,6 +421,7 @@
 		.mode		= 0644,
 		.proc_handler	= &proc_dointvec
 	},
+#endif
 	{
 		.ctl_name	= NET_IPV4_INET_PEER_THRESHOLD,
 		.procname	= "inet_peer_threshold",

-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers and embedded Linux development,
consulting, training and support.
http://free-electrons.com

Re: [patch 3/4] Configure out ethtool support

[Ben Hutchings]

Thomas Petazzoni wrote:
[...]
> --- linuxdev.orig/include/linux/ethtool.h
> +++ linuxdev/include/linux/ethtool.h
> @@ -283,6 +283,7 @@
>  struct net_device;
>  
>  /* Some generic methods drivers may use in their ethtool_ops */
> +#ifdef CONFIG_ETHTOOL
>  u32 ethtool_op_get_link(struct net_device *dev);
>  u32 ethtool_op_get_tx_csum(struct net_device *dev);
>  int ethtool_op_set_tx_csum(struct net_device *dev, u32 data);
> @@ -296,6 +297,21 @@
>  int ethtool_op_set_ufo(struct net_device *dev, u32 data);
>  u32 ethtool_op_get_flags(struct net_device *dev);
>  int ethtool_op_set_flags(struct net_device *dev, u32 data);
> +#else
> +static inline u32 ethtool_op_get_link(struct net_device *dev) { return 0; }
> +static inline u32 ethtool_op_get_tx_csum(struct net_device *dev) { return 0; }
> +static inline int ethtool_op_set_tx_csum(struct net_device *dev, u32 data) { return 0; }
> +static inline int ethtool_op_set_tx_hw_csum(struct net_device *dev, u32 data) { return 0; }
> +static inline int ethtool_op_set_tx_ipv6_csum(struct net_device *dev, u32 data) { return 0; }
> +static inline u32 ethtool_op_get_sg(struct net_device *dev) { return 0; }
> +static inline int ethtool_op_set_sg(struct net_device *dev, u32 data) { return 0; }
> +static inline u32 ethtool_op_get_tso(struct net_device *dev) { return 0; }
> +static inline int ethtool_op_set_tso(struct net_device *dev, u32 data) { return 0; }
> +static inline u32 ethtool_op_get_ufo(struct net_device *dev) { return 0; }
> +static inline int ethtool_op_set_ufo(struct net_device *dev, u32 data) { return 0; }
> +static inline u32 ethtool_op_get_flags(struct net_device *dev) { return 0; }
> +static inline int ethtool_op_set_flags(struct net_device *dev, u32 data) { return 0; }

The dummy setter functions should return -EOPNOTSUPP.  The getter functions
just read device feature flags and could be made inline.  They have no way
of returning failure.

[...]
> ===================================================================
> --- linuxdev.orig/net/core/dev.c
> +++ linuxdev/net/core/dev.c
> @@ -3669,6 +3669,7 @@
>  			return ret;
>  
>  		case SIOCETHTOOL:
> +#ifdef CONFIG_ETHTOOL
>  			dev_load(net, ifr.ifr_name);
>  			rtnl_lock();
>  			ret = dev_ethtool(net, &ifr);
> @@ -3681,6 +3682,9 @@
>  					ret = -EFAULT;
>  			}
>  			return ret;
> +#else
> +			return -EINVAL;
> +#endif
>  
>  		/*
>  		 *	These ioctl calls:

You also need to conditionalise dev_disable_lro().

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [patch 3/4] Configure out ethtool support

[David Woodhouse]

On Thu, 2008-07-31 at 11:27 +0200, Thomas Petazzoni wrote:
> 
> +#else
> +static inline u32 ethtool_op_get_link(struct net_device *dev) { return 0; }
> +static inline u32 ethtool_op_get_tx_csum(struct net_device *dev) { return 0; }
> +static inline int ethtool_op_set_tx_csum(struct net_device *dev, u32 data) { return 0; }
> +static inline int ethtool_op_set_tx_hw_csum(struct net_device *dev, u32 data) { return 0; }
> +static inline int ethtool_op_set_tx_ipv6_csum(struct net_device *dev, u32 data) { return 0; }
> +static inline u32 ethtool_op_get_sg(struct net_device *dev) { return 0; }
> +static inline int ethtool_op_set_sg(struct net_device *dev, u32 data) { return 0; }
> +static inline u32 ethtool_op_get_tso(struct net_device *dev) { return 0; }
> +static inline int ethtool_op_set_tso(struct net_device *dev, u32 data) { return 0; }
> +static inline u32 ethtool_op_get_ufo(struct net_device *dev) { return 0; }
> +static inline int ethtool_op_set_ufo(struct net_device *dev, u32 data) { return 0; }
> +static inline u32 ethtool_op_get_flags(struct net_device *dev) { return 0; }
> +static inline int ethtool_op_set_flags(struct net_device *dev, u32 data) { return 0; }
> +#endif

It's alleged that these functions are called from 'core' network code in
some places, although I can't actually see any evidence of that anywhere
in Linus' tree except for vlans and bridging.

If that's actually the case, perhaps it makes sense to add a
WARN_ON_ONCE() to these empty functions, so that a developer who
disables CONFIG_ETHTOOL when they shouldn't will see a nasty warning
about it rather than a silent failure?

Btw, I see you've made bridging 'select ETHTOOL'; did you do the same
for vlan support?

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [patch 3/4] Configure out ethtool support

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Thu, 31 Jul 2008 11:40:05 +0100

> You also need to conditionalise dev_disable_lro().

That can only be done once the CONFIG_ETHTOOL select statement
is added for CONFIG_INET.

Which basically makes this CONFIG_ETHTOOL thing completely pointless.

Re: [patch 3/4] Configure out ethtool support

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Thu, 31 Jul 2008 11:42:47 +0100

> It's alleged that these functions are called from 'core' network code in
> some places, although I can't actually see any evidence of that anywhere
> in Linus' tree except for vlans and bridging.
> 
> If that's actually the case, perhaps it makes sense to add a
> WARN_ON_ONCE() to these empty functions, so that a developer who
> disables CONFIG_ETHTOOL when they shouldn't will see a nasty warning
> about it rather than a silent failure?
> 
> Btw, I see you've made bridging 'select ETHTOOL'; did you do the same
> for vlan support?

CONFIG_INET needs it too.

dev_disable_lro() calls the ethtool ops directly.

But it still needs to be conditional, because as I said what I see
happening next is this CONFIG_ETHTOOL thing getting jammed into each
and every network driver.  (in fact, ethtool support code in a single
driver probably drarfs the 6K savings this initial patch is giving
us).

And at which point you'll have a broken system for drivers that
enable LRO and the user enables forwarding.

Re: [patch 3/4] Configure out ethtool support

[David Woodhouse]

On Thu, 2008-07-31 at 03:49 -0700, David Miller wrote:
> From: Ben Hutchings <bhutchings@solarflare.com>
> Date: Thu, 31 Jul 2008 11:40:05 +0100
> 
> > You also need to conditionalise dev_disable_lro().
> 
> That can only be done once the CONFIG_ETHTOOL select statement
> is added for CONFIG_INET.
> 
> Which basically makes this CONFIG_ETHTOOL thing completely pointless.

Other potential approaches include not enabling LRO by default if 
!CONFIG_ETHTOOL. Or having the driver(s) which _do_ enable LRO by
default 'select ETHTOOL'.

-- 
dwmw2

Re: [patch 3/4] Configure out ethtool support

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Thu, 31 Jul 2008 11:54:24 +0100

> Other potential approaches include not enabling LRO by default if 
> !CONFIG_ETHTOOL. Or having the driver(s) which _do_ enable LRO by
> default 'select ETHTOOL'.

It is possible for us to generically enable LRO for every device,
since it is a software algorithm.  And likely we will do something
like this in the not too distant future.

Re: [patch 3/4] Configure out ethtool support

[David Woodhouse]

On Thu, 2008-07-31 at 03:51 -0700, David Miller wrote:
> CONFIG_INET needs it too.
> 
> dev_disable_lro() calls the ethtool ops directly.

Ah, right. That's why it didn't show up in my grep.

There are solutions to that, as I said. Either we could 'select ETHTOOL'
in those drivers which enable LRO by default, or we could just make sure
they _don't_ enable LRO by default when CONFIG_ETHTOOL isn't set.

And if we end up doing LRO generically in software where the hardware
doesn't handle it, presumably we can do that without having to use
ethtool to change the hardware behaviour?

> But it still needs to be conditional, because as I said what I see
> happening next is this CONFIG_ETHTOOL thing getting jammed into each
> and every network driver.  (in fact, ethtool support code in a single
> driver probably drarfs the 6K savings this initial patch is giving
> us).

I think we can avoid that. If the actual functions and the struct
ethtool_ops are static, and if we have something like...

#ifdef CONFIG_ETHTOOL
#define dev_set_ethtool_ops(dev, ops) dev->ethtool_ops = ops
#else
#define dev_set_ethtool_ops(dev, ops) (void)ops
#endif

... then it should all fall out silently. (Yeah, those definitions would
need 'hardening' but they're a proof of concept).

> And at which point you'll have a broken system for drivers that
> enable LRO and the user enables forwarding.

Obviously that needs avoiding. Thanks for the technical feedback.

After an offline discussion, I understand that if we can sort out the
actual technical issues, you'll carry this in the net tree. Thanks.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [patch 3/4] Configure out ethtool support

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Thu, 31 Jul 2008 12:29:30 +0100

> After an offline discussion, I understand that if we can sort out the
> actual technical issues, you'll carry this in the net tree. Thanks.

I will, but only because you threatened to bypass me and send them
directly to Linus.  And frankly fighting someone willing to do things
like that is simply not worth my time, so I'll just merge them blindly.
You just let me know when you think they are ready for inclusion.

Re: [patch 3/4] Configure out ethtool support

[David Woodhouse]

On Thu, 2008-07-31 at 04:33 -0700, David Miller wrote:
> From: David Woodhouse <dwmw2@infradead.org>
> Date: Thu, 31 Jul 2008 12:29:30 +0100
> 
> > After an offline discussion, I understand that if we can sort out the
> > actual technical issues, you'll carry this in the net tree. Thanks.
> 
> I will, but only because you threatened to bypass me and send them
> directly to Linus.  And frankly fighting someone willing to do things
> like that is simply not worth my time, so I'll just merge them
> blindly.

That doesn't make a lot of sense, Dave. Just because I _submit_ them to
Linus, that doesn't mean he automatically takes them.

I only said I'd submit them directly to Linus because I _think_ he'd
agree with Andrew and I, and take them despite your objections. And
because I think that's the right thing for him to do.

I wasn't going to hack hera and just put them into his tree by myself.

But don't let me talk you out of taking the patches... :)

> You just let me know when you think they are ready for inclusion.

I'll do that; thanks.

-- 
dwmw2

Re: [patch 3/4] Configure out ethtool support

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Thu, 31 Jul 2008 12:46:41 +0100

> I only said I'd submit them directly to Linus because I _think_ he'd
> agree with Andrew and I, and take them despite your objections. And
> because I think that's the right thing for him to do.

I guess Linus is unable to participate in the discussion, voice his
opinion, and sort this out with the rest of us unless you submit the
patches directly to him for inclusion, right?

Can you at least begin to see why your doing that might irritate me?

> > You just let me know when you think they are ready for inclusion.
> 
> I'll do that; thanks.

No problem.

Re: [patch 3/4] Configure out ethtool support

[Adrian Bunk]

On Thu, Jul 31, 2008 at 12:46:41PM +0100, David Woodhouse wrote:
> On Thu, 2008-07-31 at 04:33 -0700, David Miller wrote:
> > From: David Woodhouse <dwmw2@infradead.org>
> > Date: Thu, 31 Jul 2008 12:29:30 +0100
> > 
> > > After an offline discussion, I understand that if we can sort out the
> > > actual technical issues, you'll carry this in the net tree. Thanks.
> > 
> > I will, but only because you threatened to bypass me and send them
> > directly to Linus.  And frankly fighting someone willing to do things
> > like that is simply not worth my time, so I'll just merge them
> > blindly.
> 
> That doesn't make a lot of sense, Dave. Just because I _submit_ them to
> Linus, that doesn't mean he automatically takes them.
> 
> I only said I'd submit them directly to Linus because I _think_ he'd
> agree with Andrew and I, and take them despite your objections. And
> because I think that's the right thing for him to do.
>...

I'm sure we can find simpler and less controversial ways to save 6 kB in 
the network stack. E.g. Ilpo's past work on making inline functions in 
the network stack out-of-line had far bigger effects than the patch in 
this discussion. And as a bonus, his work brings benefits to everyone.

It might have also made more sense to spend some of the energy used in 
this discussion instead on checking where the global 24 kB size increase 
Thomas reported for 2.6.27-rc1 compared to 2.6.26 comes from.

It's kinda silly to spend time on creating and arguing about non-trivial 
patches that might save a few kB for very few people while noone seems 
to work on reducing the continuous size increase of the kernel that 
affects everyone...

> dwmw2

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [patch 3/4] Configure out ethtool support

[Thomas Petazzoni]

Le Thu, 31 Jul 2008 18:58:20 +0300,
Adrian Bunk <bunk@kernel.org> a écrit :

> It might have also made more sense to spend some of the energy used
> in this discussion instead on checking where the global 24 kB size
> increase Thomas reported for 2.6.27-rc1 compared to 2.6.26 comes from.

I agree. This is something I'm working on. I hope to have results soon.
But I *fear* that the results will be: the size growth is 10 bytes
here, 22 bytes there, 35 bytes here, spread over hundreds of patches.
Something we can do anything against.

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers and embedded Linux development,
consulting, training and support.
http://free-electrons.com

Re: [patch 4/4] Configure out IGMP support

[David Woodhouse]

On Thu, 2008-07-31 at 11:27 +0200, Thomas Petazzoni wrote:
> This patchs adds the CONFIG_IGMP option which allows to remove support
> for the Internet Group Management Protocol, used in
> multicast. Multicast is not necessarly used by applications,
> particularly on embedded devices. As this is a size-reduction option,
> it depends on CONFIG_EMBEDDED. It allows to save ~10 kilobytes of
> kernel code/data:

The config option probably lives in net/Kconfig, not init/Kconfig.

And please could you make it clear how this interacts with IP_MULTICAST?

We already have a CONFIG_IP_MULTICAST option, for which the help text
says "For more people, it's safe to say N'. And I think it defaults to
that too. What more does CONFIG_IGMP remove? It's not made clear by the
help text.

-- 
dwmw2

Re: [patch 4/4] Configure out IGMP support

[Thomas Petazzoni]

Le Fri, 01 Aug 2008 20:41:55 +0100,
David Woodhouse <dwmw2@infradead.org> a écrit :

> The config option probably lives in net/Kconfig, not init/Kconfig.

Yes, it could. But AFAIK, until now, all CONFIG_EMBEDDED-related
options have been put in init/Kconfig. But if it's preferred, I can of
course change the patch to move the config option to net/Kconfig.

> And please could you make it clear how this interacts with
> IP_MULTICAST?
> 
> We already have a CONFIG_IP_MULTICAST option, for which the help text
> says "For more people, it's safe to say N'. And I think it defaults to
> that too. What more does CONFIG_IGMP remove? It's not made clear by
> the help text.

The interaction of IGMP support with CONFIG_IP_MULTICAST is fairly
unclear to me.

A large portion of igmp.c is already under #ifdef CONFIG_IP_MULTICAST:
all the igmp_*() functions, amongst which is igmp_rcv(), referenced in
igmp_protocol in net/ipv4/af_inet.c, which is compiled-out
when !CONFIG_IP_MULTICAST.

All the proc-related code at the end of the file is only conditionnaly
compiled on CONFIG_PROC_FS, but seems to in fact be only used if both
CONFIG_IP_MULTICAST and CONFIG_PROC_FS are selected:
igmp_mc_proc_init() in net/ipv4/ip_output.c is only called when
CONFIG_IP_MULTICAST and CONFIG_PROC_FS are selected.

Besides that, it's unclear to me why the ip_mc_*() functions are useful
when !CONFIG_IP_MULTICAST, but I'm probably missing something. They are
used to implement setsockopt-operations related to multicast, hooks for
the routing code to handle multicast-related traffic, etc.

Sincerly,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers and embedded Linux development,
consulting, training and support.
http://free-electrons.com

Re: [patch 4/4] Configure out IGMP support

[Adrian Bunk]

On Mon, Aug 04, 2008 at 02:48:07PM +0200, Thomas Petazzoni wrote:
> Le Fri, 01 Aug 2008 20:41:55 +0100,
> David Woodhouse <dwmw2@infradead.org> a écrit :
> 
> > The config option probably lives in net/Kconfig, not init/Kconfig.
> 
> Yes, it could. But AFAIK, until now, all CONFIG_EMBEDDED-related
> options have been put in init/Kconfig.
>...

No, the vast majority is actually outside of init/Kconfig.

> Sincerly,
> 
> Thomas

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [patch 4/4] Configure out IGMP support

[David Woodhouse]

On Mon, 2008-08-04 at 14:48 +0200, Thomas Petazzoni wrote:
> Le Fri, 01 Aug 2008 20:41:55 +0100,
> David Woodhouse <dwmw2@infradead.org> a écrit :
> 
> > The config option probably lives in net/Kconfig, not init/Kconfig.
> 
> Yes, it could. But AFAIK, until now, all CONFIG_EMBEDDED-related
> options have been put in init/Kconfig. But if it's preferred, I can of
> course change the patch to move the config option to net/Kconfig.

It clearly lives in net/Kconfig.

> > And please could you make it clear how this interacts with
> > IP_MULTICAST?
> > 
> > We already have a CONFIG_IP_MULTICAST option, for which the help text
> > says "For more people, it's safe to say N'. And I think it defaults to
> > that too. What more does CONFIG_IGMP remove? It's not made clear by
> > the help text.
> 
> The interaction of IGMP support with CONFIG_IP_MULTICAST is fairly
> unclear to me.
> 
> A large portion of igmp.c is already under #ifdef CONFIG_IP_MULTICAST:
> all the igmp_*() functions, amongst which is igmp_rcv(), referenced in
> igmp_protocol in net/ipv4/af_inet.c, which is compiled-out
> when !CONFIG_IP_MULTICAST.
> 
> All the proc-related code at the end of the file is only conditionnaly
> compiled on CONFIG_PROC_FS, but seems to in fact be only used if both
> CONFIG_IP_MULTICAST and CONFIG_PROC_FS are selected:
> igmp_mc_proc_init() in net/ipv4/ip_output.c is only called when
> CONFIG_IP_MULTICAST and CONFIG_PROC_FS are selected.
> 
> Besides that, it's unclear to me why the ip_mc_*() functions are useful
> when !CONFIG_IP_MULTICAST, but I'm probably missing something. 

Most of them aren't, as far as I can tell. 

> They are used to implement setsockopt-operations related to multicast,
> hooks for the routing code to handle multicast-related traffic, etc.

I wonder if those options should return errors now, rather than silently
failing but returning zero. Or maybe that _would_ cause a stock build of
ntpd to fail? Not that it really matters if it _does_, though.

It sounds like 'CONFIG_IGMP' is a bad name for the option, too -- and
the help text is similarly misleading. I think you need to work out how
it all fits together with CONFIG_IP_MULTICAST, fix it up, and resubmit
it.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation