Remove more code when IP_MULTICAST=n

[RFC] Remove more code when IP_MULTICAST=n

The idea of this patch originates from a patch by Matt Mackall in the Linux Tiny project, adding a new CONFIG_IGMP option to compile out net/ipv4/igmp.o on embedded devices running applications that don't care about multicast.

After discussion with David Woodhouse, we wondered why introducing a new option is necessary: couldn't all the IGMP code be compiled out when IP_MULTICAST=n (IP_MULTICAST is an already existing option) ?

This patch implements this idea: net/ipv4/igmp.o and multicast-related socket operations get compiled out when IP_MULTICAST=N. However, my understanding of the network stack internals is too limited to be sure that it actually makes sense, which is why I'm sending this patch simply for comments on what could be done. In particular :

* I'm not sure why net/ipv4/igmp.o was needed when IP_MULTICAST=n ;

* I'm not sure that returning -ENOPROTOOPT for socket operations related to multicast management is correct when IP_MULTICAST=n.

For reference, the size savings are as follows, before and after the patch, for a x86 kernel with IP_MULTICAST=n.

text data bss dec hex filename
2034992 157896 270336 2463224 2595f8 vmlinux
2024260 157856 270336 2452452 256be4 vmlinux.new
-10732 -40 0 -10772 -2A14 +/-

Remaining to fix:

* Virtual server support, using ip_mc_join_group() in ipv4/ipvs/ip_vs_sync.c

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

Index: 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_IP_MULTICAST
 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,16 @@
 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
+#define ip_check_mc(a, b, c, d) ({ 0; })
+#define ip_mc_sf_allow(a, b, c, d) ({ 1; })
+#define ip_mc_init_dev(a) ({ })
+#define ip_mc_up(a) ({ })
+#define ip_mc_down(a) ({ })
+#define ip_mc_destroy_dev(a) ({ })
+#define ip_mc_init_dev(a) ({ })
+#define ip_mc_drop_socket(a) ({ })
+#endif
 
 #endif
 #endif

Index: linuxdev/net/ipv4/Makefile
===================================================================
@@ -9,7 +9,7 @@
 	     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
 

@@ -19,6 +19,7 @@
 obj-$(CONFIG_PROC_FS) += proc.o
 obj-$(CONFIG_IP_MULTIPLE_TABLES) += fib_rules.o
 obj-$(CONFIG_IP_MROUTE) += ipmr.o
+obj-$(CONFIG_IP_MULTICAST) += igmp.o
 obj-$(CONFIG_NET_IPIP) += ipip.o
 obj-$(CONFIG_NET_IPGRE) += ip_gre.o
 obj-$(CONFIG_SYN_COOKIES) += syncookies.o

Index: 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
===================================================================
@@ -544,6 +544,7 @@
 		if (!val)
 			skb_queue_purge(&sk->sk_error_queue);
 		break;
+#ifdef CONFIG_IP_MULTICAST
 	case IP_MULTICAST_TTL:
 		if (sk->sk_type == SOCK_STREAM)
 			goto e_inval;

@@ -860,6 +861,7 @@
 		kfree(gsf);
 		break;
 	}
+#endif
 	case IP_ROUTER_ALERT:
 		err = ip_ra_control(sk, val ? 1 : 0, NULL);
 		break;

@@ -1044,6 +1046,7 @@
 	case IP_RECVERR:
 		val = inet->recverr;
 		break;
+#ifdef CONFIG_IP_MULTICAST
 	case IP_MULTICAST_TTL:
 		val = inet->mc_ttl;
 		break;

@@ -1099,6 +1102,7 @@
 		release_sock(sk);
 		return err;
 	}
+#endif
 	case IP_PKTOPTIONS:
 	{
 		struct msghdr msg;

Index: linuxdev/net/ipv4/sysctl_net_ipv4.c
===================================================================
@@ -411,8 +411,6 @@
 		.mode		= 0644,
 		.proc_handler	= &proc_dointvec
 	},
-
-#endif
 	{
 		.ctl_name	= NET_IPV4_IGMP_MAX_MSF,
 		.procname	= "igmp_max_msf",

@@ -421,6 +419,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

Probably you wanted to cc [email protected]?

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   [email protected]
Internet: http://www.sony-europe.com/

A division of Sony Europe (Belgium) N.V. VAT BE 0413.825.160 · RPR Brussels Fortis · BIC GEBABEBB · IBAN BE41293037680010

Not necessarly at the beginning: I first wanted to get the feedback of embedded-concerned developers, who might have a better understanding than me of the networking stack. Last time I submitted a size-reduction patch to Dave Miller concerning IGMP, the answer was:

«
I'm not applying this.

This removes core parts of the BSD socket API from applications. Like TCP and UDP, multicast capabilities are something applications can always depend upon being available.

If you want a broken networking implementation, you have the source code, so you can do it in your own tree.
»

So, I'd prefer to send a good patch from the beginning.

Thanks,

Thomas

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

Out of that bit of criticism, it's the validity of the approach in particular that's being called in to question, rather than the patch itself. How to clean up the patch itself is irrelevant if the idea itself is being shot down by the folks that will merge it. This is something that needs to be resolved first, and lists outside of the scope of netdev are really the wrong place to do this.

This is generally the way a lot of the size reduction work seems to be going these days. Now that most of the low-hanging fruit is out of the way, it's mostly down to micro-optimizations aimed at things perceived to be core functionality by others. Expect to continue running in to these sorts of problems if you choose to continue down this path.

This is a good observation.

I agree that we will continue to face opposition like this. However, IIRC, near the end of this thread David Miller did say that if the patch got cleaned up he would take another look.

With regard to things perceived to be core functionality by others, this is exactly correct. In this case, David Miller asserts that without IGMP functionality, the network stack is incorrect (and that "people" expect and depend on a fully functional stack.) He doesn't acknowledge (and some other kernel developer don't either) that there might be developers willing to forego stack features in the interest of other aims. This is something that will require ongoing education with kernel developers about.

There are two sides to this battle: 1) convincing people that size matters (AND that small increments can have a cumulative impact that matters), and
2) that the kernel can function "correctly" with the affected feature removed. It is quite common for kernel developers to raise use cases unrelated to embedded, which is frustrating. But the burden of proof is on us to show that the slicing and dicing we're requesting doesn't fundamentally break the remaining functionality in ways that will negatively impact the maintainers of those function areas.

In this particular case, I think we need to show that
there are valid cases were an embedded product can use networking just fine, without IGMP support but with support for multicast. (I believe that's the slice that this particular patch makes). IMHO, what's needed is a test to show that this works. This can then be presented to the netdev guys, who are, after all much more experienced with the networking stuff. If they can show that multicast does indeed *need* IGMP to work correctly, then we need to back off. The last thing I want is to find out in the field that some streaming feature I've built into a Sony camera that relies on multicast won't work in lots of network configurations. If that's true, then David is doing me a favor by saying No. (Note that I might still make the decision to forego a feature for size reasons if the number of instances of non-workingness was small.)

IMHO, we need to acknowledge that the network guys can help us avoid problems like the above. Avoiding confrontation is good, but we should still be ready to assert the value of our patches, when needed.
-- Tim

P.S. We should also be careful about taking claims of incorrectness at face value. In this particular thread, David asserted that NTP breaks with this patch applied, but that turned out not to be the case.

=============================
Tim Bird
Architecture Group Chair, CE Linux Forum Senior Staff Engineer, Sony Corporation of America
=============================

Hi,

As a networking sysadmin monkey I can tell you that:
* you _need_ IGMP to *join* multicast groups
* you do not need to know anything about multicast to send data to a

	group (except to consider a sensible value for the TTL in your
	IP header)[1]

* you need PIM support *route* multicast traffic[2]

So as long as your embedded device does not need to route or *receive* a multicast stream then you do not need IGMP support.

A reasonable example of this is a IPTV (source) streaming device, such as a DVB->IP gateway.

To be honest, especially so in an embedded device, multicast is going to become more and more important. Zero configuration along with DNS and even now a multicast enabled DHCP RFC are here. Multicast is dead handy for devices to automatically discover each other without using dreadful broadcast traffic that of course is restricted to the same VLAN.

Real PnP plus service discovery lives in the world of multicast, the lusers want it and it's the only way to provision such facilities. My personal recommendation is that patches that affect 'net/' go to netdev@.

Just the comments of a daytime networking sysadmin :)

Cheers

Alex

[1] an exception, iirc, is that you can speed up group creation with the

	"Create Group Request" igmp message type, but I could be wrong;
	this is most definiately optional though.

[2] if your router/switch does not support multicast then you will find it floods your switch ports with the traffic, effectively making it broadcast traffic

Le Tue, 26 Aug 2008 09:44:31 -0700,
Tim Bird <[email protected]> a écrit :

The patch doesn't try to get multicast support to work without IGMP, but tries to remove as much code as possible when multicast support is not needed.

Two approaches have been tried :

* The first one, by Matt Mackall, was to add a new CONFIG_IGMP option next to the existing CONFIG_MULTICAST option, to disable the parts of the IGMP protocol support that were still compiled-in when CONFIG_MULTICAST=n

* The second one, this patch, simply removes the IGMP protocol code when CONFIG_MULTICAST=n, because my understanding is that the IGMP protocol code is useless when multicast is not used.

I might try to send this second approach to netdev, but it seems that not everybody agrees on the approach of removing things for the kernel by adding more and more configuration options. If the network maintainers don't agree with this approach, then I'm not sure how we can make this patch progress in any way (and this is not an accusation towards the network maintainers, they also have valid and good arguments against the addition of dozens of configuration options to disable a few KB of code).

Sincerly,

Thomas

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

Ok, that's different than I thought. Sorry I missed that in your original message. I thought this was making an
extra slice (a'la approach one). Instead it's just making the existing slice more accurate.

This is correct. IGMP ONLY makes sense if you have multicast support!

That would be irrelevant for this patch.

Well, if we're not adding a new config option, but just making the existing option better, I have a hard time seeing the problem, even if the savings are small.

It's kind of dumb to have dead code lying around. Since the new patch adds nothing to the configuration space, the only downside I can see is the possible increase in maintenance burden caused by conditional code. But, then again, I'm not a networking guy. I think we should submit this. If the networking
guys want to see CONFIG_MULTICAST removed, that's a completely separate argument (and one I would disagree with). But we shouldn't need to fight that fight here.

My understanding is that the network guys have not see this new patch yet. I think we should make sure they see it and get their response. (Not in any confrontational way).
They may yet have valid reasons why slicing this stuff out will hurt something. But if their only complaint is that they don't like multicast going away, that's something else.
-- Tim

=============================
Tim Bird
Architecture Group Chair, CE Linux Forum Senior Staff Engineer, Sony Corporation of America
=============================