[PATCH -next 0/7] clean up needless use of module infrastructure

[PATCH -next 0/7] clean up needless use of module infrastructure

[Paul Gortmaker]

People can embed modular includes and modular exit functions into code
that never use any of it, and they won't get any errors or warnings.

Using modular infrastructure in non-modules might seem harmless, but some
of the downfalls this leads to are:

 (1) it is easy to accidentally write unused module_exit removal code
 (2) it can be misleading when reading the source, thinking a driver can
     be modular when the Makefile and/or Kconfig prohibit it
 (3) an unused include of the module.h header file will in turn
     include nearly everything else; adding a lot to CPP overhead.
 (4) it gets copied/replicated into other drivers and spreads quickly.

As a data point for #3 above, an empty C file that just includes the
module.h header generates over 750kB of CPP output.  Repeating the same
experiment with init.h and the result is less than 12kB; with export.h
it is only about 1/2kB; with both it still is less than 12kB.  One driver
in this series gets the module.h ---> init.h+export.h conversion.

Worse, are headers in include/linux that in turn include <linux/module.h>
as they can impact a whole fleet of drivers, or a whole subsystem, so
special care should be used in order to avoid that.  Such headers should
only include what they need to be stand-alone; they should not be trying
to anticipate the various header needs of their possible end users.

In this series, four include/linux headers have module.h removed from
them because they don't strictly need it.  Then three chunks of net
related code have modular infrastructure that isn't used, removed.

There are no runtime changes, so the biggest risk is a genuine consumer
of module.h content relying on implicitly getting it from one of the
include/linux instances removed here - thus resulting in a build fail.

With that in mind, allmodconfig build testing was done on x86-64, arm64,
x86-32, arm. powerpc, and mips on linux-next (and hence net-next).

Paul.

---

Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Daniel Wagner <daniel.wagner@bmw-carit.de>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: Martin KaFai Lau <kafai@fb.com>
Cc: "Rosen, Rami" <rami.rosen@intel.com>
Cc: Song Liu <songliubraving@fb.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Yonghong Song <yhs@fb.com>
Cc: Yotam Gigi <yotam.gi@gmail.com>

Paul Gortmaker (7):
  net: psample: drop include of module.h from psample.h
  net: ife: drop include of module.h from net/ife.h
  net: fib: drop include of module.h from fib_notifier.h
  net: tc_act: drop include of module.h from tc_ife.h
  cgroup: net: remove left over MODULE_LICENSE tag
  net: bpfilter: dont use module_init in non-modular code
  net: strparser: make it explicitly non-modular

 include/net/fib_notifier.h  |  3 ++-
 include/net/ife.h           |  1 -
 include/net/psample.h       |  1 -
 include/net/tc_act/tc_ife.h |  3 ++-
 net/core/netprio_cgroup.c   |  2 --
 net/ipv4/bpfilter/sockopt.c |  3 +--
 net/strparser/strparser.c   | 14 ++++----------
 7 files changed, 9 insertions(+), 18 deletions(-)

-- 
2.7.4

[PATCH 1/7] net: psample: drop include of module.h from psample.h

[Paul Gortmaker]

Ideally, header files under include/linux shouldn't be adding
includes of other headers, in anticipation of their consumers,
but just the headers needed for the header itself to pass
parsing with CPP.

The module.h is particularly bad in this sense, as it itself does
include a whole bunch of other headers, due to the complexity of
module support.

There doesn't appear to be anything in psample.h that is module
related, and build coverage doesn't appear to show any other
files/drivers relying implicitly on getting it from here.

So it appears we are simply free to just remove it in this case.

Cc: Yotam Gigi <yotam.gi@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 include/net/psample.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/net/psample.h b/include/net/psample.h
index 9b80f814ab04..37a4df2325b2 100644
--- a/include/net/psample.h
+++ b/include/net/psample.h
@@ -3,7 +3,6 @@
 #define __NET_PSAMPLE_H
 
 #include <uapi/linux/psample.h>
-#include <linux/module.h>
 #include <linux/list.h>
 
 struct psample_group {
-- 
2.7.4

[PATCH 2/7] net: ife: drop include of module.h from net/ife.h

[Paul Gortmaker]

Ideally, header files under include/linux shouldn't be adding
includes of other headers, in anticipation of their consumers,
but just the headers needed for the header itself to pass
parsing with CPP.

The module.h is particularly bad in this sense, as it itself does
include a whole bunch of other headers, due to the complexity of
module support.

There doesn't appear to be anything in net/ife.h that is module
related, and build coverage doesn't appear to show any other
files/drivers relying implicitly on getting it from here.

So it appears we are simply free to just remove it in this case.

Cc: Yotam Gigi <yotam.gi@gmail.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 include/net/ife.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/net/ife.h b/include/net/ife.h
index e117617e3c34..7e2538d8585b 100644
--- a/include/net/ife.h
+++ b/include/net/ife.h
@@ -4,7 +4,6 @@
 
 #include <linux/etherdevice.h>
 #include <linux/rtnetlink.h>
-#include <linux/module.h>
 #include <uapi/linux/ife.h>
 
 #if IS_ENABLED(CONFIG_NET_IFE)
-- 
2.7.4

[PATCH 3/7] net: fib: drop include of module.h from fib_notifier.h

[Paul Gortmaker]

Ideally, header files under include/linux shouldn't be adding
includes of other headers, in anticipation of their consumers,
but just the headers needed for the header itself to pass
parsing with CPP.

The module.h is particularly bad in this sense, as it itself does
include a whole bunch of other headers, due to the complexity of
module support.

Since fib_notifier.h is not going into a module struct looking for
specific fields, we can just let it know that module is a struct,
just like about 60 other include/linux headers already do.

Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 include/net/fib_notifier.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/net/fib_notifier.h b/include/net/fib_notifier.h
index c91ec732afd6..c49d7bfb5c30 100644
--- a/include/net/fib_notifier.h
+++ b/include/net/fib_notifier.h
@@ -2,10 +2,11 @@
 #define __NET_FIB_NOTIFIER_H
 
 #include <linux/types.h>
-#include <linux/module.h>
 #include <linux/notifier.h>
 #include <net/net_namespace.h>
 
+struct module;
+
 struct fib_notifier_info {
 	struct net *net;
 	int family;
-- 
2.7.4

[PATCH 4/7] net: tc_act: drop include of module.h from tc_ife.h

[Paul Gortmaker]

Ideally, header files under include/linux shouldn't be adding
includes of other headers, in anticipation of their consumers,
but just the headers needed for the header itself to pass
parsing with CPP.

The module.h is particularly bad in this sense, as it itself does
include a whole bunch of other headers, due to the complexity of
module support.

Since tc_ife.h is not going into a module struct looking for
specific fields, we can just let it know that module is a struct,
just like about 60 other include/linux headers already do.

Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 include/net/tc_act/tc_ife.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/net/tc_act/tc_ife.h b/include/net/tc_act/tc_ife.h
index 86d13b01b39d..c7f24a2da1ca 100644
--- a/include/net/tc_act/tc_ife.h
+++ b/include/net/tc_act/tc_ife.h
@@ -5,7 +5,8 @@
 #include <net/act_api.h>
 #include <linux/etherdevice.h>
 #include <linux/rtnetlink.h>
-#include <linux/module.h>
+
+struct module;
 
 struct tcf_ife_params {
 	u8 eth_dst[ETH_ALEN];
-- 
2.7.4

[PATCH 5/7] cgroup: net: remove left over MODULE_LICENSE tag

[Paul Gortmaker]

The Kconfig currently controlling compilation of this code is:

net/Kconfig:config CGROUP_NET_PRIO
net/Kconfig:    bool "Network priority cgroup"

...meaning that it currently is not being built as a module by anyone,
as module support was discontinued in 2014.

We delete the MODULE_LICENSE tag since all that information is already
contained at the top of the file in the comments.

We don't delete module.h from the includes since it was no longer there
to begin with.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Tejun Heo <tj@kernel.org>
Cc: "Rosen, Rami" <rami.rosen@intel.com>
Cc: Daniel Wagner <daniel.wagner@bmw-carit.de>
Cc: netdev@vger.kernel.org
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 net/core/netprio_cgroup.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c
index b9057478d69c..7e3d0d99dfae 100644
--- a/net/core/netprio_cgroup.c
+++ b/net/core/netprio_cgroup.c
@@ -301,6 +301,4 @@ static int __init init_cgroup_netprio(void)
 	register_netdevice_notifier(&netprio_device_notifier);
 	return 0;
 }
-
 subsys_initcall(init_cgroup_netprio);
-MODULE_LICENSE("GPL v2");
-- 
2.7.4

[PATCH 6/7] net: bpfilter: dont use module_init in non-modular code

[Paul Gortmaker]

The Kconfig controlling this code is:

bpfilter/Kconfig:menuconfig BPFILTER
bpfilter/Kconfig:   bool "BPF based packet filtering framework (BPFILTER)"

Since it isn't a module, we shouldn't use module_init().  Instead we
use device_initcall() - which is exactly what module_init() defaults
to for non-modular code/builds.

We don't remove <linux/module.h> from the includes since this file does
a request_module() and hence is a valid user of that header file, even
though it is not modular itself.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 net/ipv4/bpfilter/sockopt.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/ipv4/bpfilter/sockopt.c b/net/ipv4/bpfilter/sockopt.c
index 1e976bb93d99..15427163a041 100644
--- a/net/ipv4/bpfilter/sockopt.c
+++ b/net/ipv4/bpfilter/sockopt.c
@@ -77,5 +77,4 @@ static int __init bpfilter_sockopt_init(void)
 
 	return 0;
 }
-
-module_init(bpfilter_sockopt_init);
+device_initcall(bpfilter_sockopt_init);
-- 
2.7.4

[PATCH 7/7] net: strparser: make it explicitly non-modular

[Paul Gortmaker]

The Kconfig currently controlling compilation of this code is:

net/strparser/Kconfig:config STREAM_PARSER
net/strparser/Kconfig:  def_bool n

...meaning that it currently is not being built as a module by anyone.

Lets remove the modular code that is essentially orphaned, so that
when reading the driver there is no doubt it is builtin-only.

Since module_init translates to device_initcall in the non-modular
case, the init ordering remains unchanged with this commit.  For
clarity, we change the fcn name mod_init to dev_init at the same time.

We replace module.h with init.h and export.h ; the latter since this
file exports some syms.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Martin KaFai Lau <kafai@fb.com>
Cc: Song Liu <songliubraving@fb.com>
Cc: Yonghong Song <yhs@fb.com>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 net/strparser/strparser.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/net/strparser/strparser.c b/net/strparser/strparser.c
index 0ba363624339..e137698e8aef 100644
--- a/net/strparser/strparser.c
+++ b/net/strparser/strparser.c
@@ -14,7 +14,8 @@
 #include <linux/file.h>
 #include <linux/in.h>
 #include <linux/kernel.h>
-#include <linux/module.h>
+#include <linux/export.h>
+#include <linux/init.h>
 #include <linux/net.h>
 #include <linux/netdevice.h>
 #include <linux/poll.h>
@@ -545,7 +546,7 @@ void strp_check_rcv(struct strparser *strp)
 }
 EXPORT_SYMBOL_GPL(strp_check_rcv);
 
-static int __init strp_mod_init(void)
+static int __init strp_dev_init(void)
 {
 	strp_wq = create_singlethread_workqueue("kstrp");
 	if (unlikely(!strp_wq))
@@ -553,11 +554,4 @@ static int __init strp_mod_init(void)
 
 	return 0;
 }
-
-static void __exit strp_mod_exit(void)
-{
-	destroy_workqueue(strp_wq);
-}
-module_init(strp_mod_init);
-module_exit(strp_mod_exit);
-MODULE_LICENSE("GPL");
+device_initcall(strp_dev_init);
-- 
2.7.4

Re: [PATCH -next 0/7] clean up needless use of module infrastructure

[David Miller]

From: Paul Gortmaker <paul.gortmaker@windriver.com>
Date: Sat, 20 Apr 2019 23:29:41 -0400

> People can embed modular includes and modular exit functions into code
> that never use any of it, and they won't get any errors or warnings.
> 
> Using modular infrastructure in non-modules might seem harmless, but some
> of the downfalls this leads to are:
> 
>  (1) it is easy to accidentally write unused module_exit removal code
>  (2) it can be misleading when reading the source, thinking a driver can
>      be modular when the Makefile and/or Kconfig prohibit it
>  (3) an unused include of the module.h header file will in turn
>      include nearly everything else; adding a lot to CPP overhead.
>  (4) it gets copied/replicated into other drivers and spreads quickly.
> 
> As a data point for #3 above, an empty C file that just includes the
> module.h header generates over 750kB of CPP output.  Repeating the same
> experiment with init.h and the result is less than 12kB; with export.h
> it is only about 1/2kB; with both it still is less than 12kB.  One driver
> in this series gets the module.h ---> init.h+export.h conversion.
> 
> Worse, are headers in include/linux that in turn include <linux/module.h>
> as they can impact a whole fleet of drivers, or a whole subsystem, so
> special care should be used in order to avoid that.  Such headers should
> only include what they need to be stand-alone; they should not be trying
> to anticipate the various header needs of their possible end users.
> 
> In this series, four include/linux headers have module.h removed from
> them because they don't strictly need it.  Then three chunks of net
> related code have modular infrastructure that isn't used, removed.
> 
> There are no runtime changes, so the biggest risk is a genuine consumer
> of module.h content relying on implicitly getting it from one of the
> include/linux instances removed here - thus resulting in a build fail.
> 
> With that in mind, allmodconfig build testing was done on x86-64, arm64,
> x86-32, arm. powerpc, and mips on linux-next (and hence net-next).

Series applied, thanks Paul.