Skip to contentSkip to conversation

make non-modular code explicitly non-modular

PATCH In review Paul Gortmaker 3 participants 8 messages Oct 7, 2015 – Oct 9, 2015

Subsystems (5)
lore
Share & tools
lore
#0 lore raw

v2 cover make non-modular code explicitly non-modular

[v2: drop m68k patches that Geert converted to modules; add one ARM
driver patch ; update net-next baseline to today; switch to ARM for build testing.]

In a previous merge window, we made changes to allow better delineation between modular and non-modular code in commit 0fd972a7d91d6e15393c449492a04d94c0b89351 ("module: relocate module_init from init.h to module.h"). This allows us to now ensure module code looks modular and non-modular code does not accidentally look modular just to avoid suffering build breakage.

Here we target code that is, by nature of their Makefile and/or Kconfig settings, only available to be built-in, but implicitly presenting itself as being possibly modular by way of using modular headers, macros, and functions.

The goal here is to remove that illusion of modularity from these files, but in a way that leaves the actual runtime unchanged. In doing so, we remove code that has never been tested and adds no value to the tree. And we continue the process of expecting a level of consistency between the Kconfig/Makefile of code and the code in use itself.

Fortuntately the net subsystem has relatively few instances, given the overall amount of code and drivers it contains. For comparison there are over 300 instances tree wide, resulting in a possible net removal of on the order of 5000 lines of unused code.

Build tested on net-next from today, on ARM, since that is the arch where the one ethernet driver changed here is available.

Paul.
--

Cc Alexei Starovoitov <[email protected]>
Cc Anish Bhatt <[email protected]>
Cc Craig Gallek <[email protected]>
Cc Daniel Borkmann <[email protected]>
Cc "David S. Miller" <[email protected]>
Cc Eric Dumazet <[email protected]>
Cc Jamal Hadi Salim <[email protected]>
Cc John Fastabend <[email protected]>
Cc Nicolas Dichtel <[email protected]>
Cc Or Gerlitz <[email protected]>
Cc Shani Michaeli <[email protected]>
Cc Varka Bhadram <[email protected]>

Paul Gortmaker (4):
net/core: make sock_diag.c explicitly non-modular net/dcb: make dcbnl.c explicitly non-modular
net/sched: make sch_blackhole.c explicitly non-modular drivers/net/ethernet: make ti/cpsw-phy-sel.c explicitly non-modular

 drivers/net/ethernet/ti/cpsw-phy-sel.c | 10 ++++------
 net/core/sock_diag.c                   | 14 +++-----------
 net/dcb/dcbnl.c                        | 30 +++---------------------------
 net/sched/sch_blackhole.c              | 15 +++------------
 4 files changed, 13 insertions(+), 56 deletions(-)
Signature · 1 line
2.6.0.rc3
#1 lore raw

PATCH v2 1/4 net/core: make sock_diag.c explicitly non-modular

The Makefile currently controlling compilation of this code lists it under "obj-y" ...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. We can change to one of the other priority initcalls (subsys?) at any later date, if desired.

We can't remove module.h since the file uses other module related stuff even though it is not modular itself.

We move the information from the MODULE_LICENSE tag to the top of the file, since that information is not captured anywhere else. The MODULE_ALIAS_NET_PF_PROTO becomes a no-op in the non modular case, so it is removed.

Cc "David S. Miller" <[email protected]>
Cc Eric Dumazet <[email protected]>
Cc Nicolas Dichtel <[email protected]>
Cc Daniel Borkmann <[email protected]>
Cc Alexei Starovoitov <[email protected]>
Cc Craig Gallek <[email protected]>
---
 net/core/sock_diag.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)
net/core/sock_diag.c +3 -11
index 817622f3dbb7..0c1d58d43f67 100644
@@ -1,3 +1,5 @@
+/* License: GPL */
+
 #include <linux/mutex.h>
 #include <linux/socket.h>
 #include <linux/skbuff.h>
@@ -323,14 +325,4 @@ static int __init sock_diag_init(void)
 	BUG_ON(!broadcast_wq);
 	return register_pernet_subsys(&diag_net_ops);
 }
-
-static void __exit sock_diag_exit(void)
-{
-	unregister_pernet_subsys(&diag_net_ops);
-	destroy_workqueue(broadcast_wq);
-}
-
-module_init(sock_diag_init);
-module_exit(sock_diag_exit);
-MODULE_LICENSE("GPL");
-MODULE_ALIAS_NET_PF_PROTO(PF_NETLINK, NETLINK_SOCK_DIAG);
+device_initcall(sock_diag_init);
Signature · 1 line
2.6.0.rc3
#2 lore raw

PATCH v2 2/4 net/dcb: make dcbnl.c explicitly non-modular

In reply to Paul Gortmaker

The Kconfig currently controlling compilation of this code is:

net/dcb/Kconfig:config DCB
net/dcb/Kconfig: bool "Data Center Bridging support"

...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. We can change to one of the other priority initcalls (subsys?) at any later date, if desired.

We also delete the MODULE_LICENSE tag etc. since all that information is (or is now) already contained at the top of the file in the comments.

Cc "David S. Miller" <[email protected]>
Cc Or Gerlitz <[email protected]>
Cc Anish Bhatt <[email protected]>
Cc John Fastabend <[email protected]>
Cc Shani Michaeli <[email protected]>
---
 net/dcb/dcbnl.c | 30 +++---------------------------
 1 file changed, 3 insertions(+), 27 deletions(-)
net/dcb/dcbnl.c +3 -27
index 5b21f6f88e97..4f6c1862dfd2 100644
@@ -13,6 +13,7 @@
  * You should have received a copy of the GNU General Public License along with
  * this program; if not, see <http://www.gnu.org/licenses/>.
  *
+ * Description: Data Center Bridging netlink interface
  * Author: Lucy Liu <[email protected]>
  */
 
@@ -24,7 +25,7 @@
 #include <linux/dcbnl.h>
 #include <net/dcbevent.h>
 #include <linux/rtnetlink.h>
-#include <linux/module.h>
+#include <linux/init.h>
 #include <net/sock.h>
 
 /* Data Center Bridging (DCB) is a collection of Ethernet enhancements
@@ -48,10 +49,6 @@
  * features for capable devices.
  */
 
-MODULE_AUTHOR("Lucy Liu, <[email protected]>");
-MODULE_DESCRIPTION("Data Center Bridging netlink interface");
-MODULE_LICENSE("GPL");
-
 /**************** DCB attribute policies *************************************/
 
 /* DCB netlink attributes policy */
@@ -1935,19 +1932,6 @@ int dcb_ieee_delapp(struct net_device *dev, struct dcb_app *del)
 }
 EXPORT_SYMBOL(dcb_ieee_delapp);
 
-static void dcb_flushapp(void)
-{
-	struct dcb_app_type *app;
-	struct dcb_app_type *tmp;
-
-	spin_lock_bh(&dcb_lock);
-	list_for_each_entry_safe(app, tmp, &dcb_app_list, list) {
-		list_del(&app->list);
-		kfree(app);
-	}
-	spin_unlock_bh(&dcb_lock);
-}
-
 static int __init dcbnl_init(void)
 {
 	INIT_LIST_HEAD(&dcb_app_list);
@@ -1957,12 +1941,4 @@ static int __init dcbnl_init(void)
 
 	return 0;
 }
-module_init(dcbnl_init);
-
-static void __exit dcbnl_exit(void)
-{
-	rtnl_unregister(PF_UNSPEC, RTM_GETDCB);
-	rtnl_unregister(PF_UNSPEC, RTM_SETDCB);
-	dcb_flushapp();
-}
-module_exit(dcbnl_exit);
+device_initcall(dcbnl_init);
Signature · 1 line
2.6.0.rc3
#3 lore raw

PATCH v2 3/4 net/sched: make sch_blackhole.c explicitly non-modular

In reply to Paul Gortmaker

The Kconfig currently controlling compilation of this code is:

net/sched/Kconfig:menuconfig NET_SCHED
net/sched/Kconfig: bool "QoS and/or fair queueing"

...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. We can change to one of the other priority initcalls (subsys?) at any later date, if desired.

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

Cc Jamal Hadi Salim <[email protected]>
Cc "David S. Miller" <[email protected]>
---
 net/sched/sch_blackhole.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)
net/sched/sch_blackhole.c +3 -12
index 094a874b48bc..3fee70d9814f 100644
@@ -11,7 +11,7 @@
  * Note: Quantum tunneling is not supported.
  */
 
-#include <linux/module.h>
+#include <linux/init.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
 #include <linux/skbuff.h>
@@ -37,17 +37,8 @@ static struct Qdisc_ops blackhole_qdisc_ops __read_mostly = {
 	.owner		= THIS_MODULE,
 };
 
-static int __init blackhole_module_init(void)
+static int __init blackhole_init(void)
 {
 	return register_qdisc(&blackhole_qdisc_ops);
 }
-
-static void __exit blackhole_module_exit(void)
-{
-	unregister_qdisc(&blackhole_qdisc_ops);
-}
-
-module_init(blackhole_module_init)
-module_exit(blackhole_module_exit)
-
-MODULE_LICENSE("GPL");
+device_initcall(blackhole_init)
Signature · 1 line
2.6.0.rc3
#4 lore raw

Re: [PATCH-next v2 3/4] net/sched: make sch_blackhole.c explicitly non-modular

Quoted 6 lines · Paul Gortmaker

The Kconfig currently controlling compilation of this code is:

net/sched/Kconfig:menuconfig NET_SCHED
net/sched/Kconfig: bool "QoS and/or fair queueing"

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

Is there any reason why sch_blackhole can't be a module like other qdisc's?

If not, I'd rather making it be a module. It is small but not often used.

#5 lore raw

Re: [PATCH-next v2 3/4] net/sched: make sch_blackhole.c explicitly non-modular

Quoted 7 lines · [Re: [PATCH-next v2 3/4] net/sched: make sch_blackhole.c explicitly…
Quoted 5 lines · Paul Gortmaker

The Kconfig currently controlling compilation of this code is:

net/sched/Kconfig:menuconfig NET_SCHED
net/sched/Kconfig: bool "QoS and/or fair queueing"

Quoted 6 lines
Quoted 1 line

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

Is there any reason why sch_blackhole can't be a module like other qdisc's?

If not, I'd rather making it be a module. It is small but not often used.

As I've said in other similar threads, there are some 300+ places where code that can't ever be modular uses modular calls and/or introduces dead module remove code.

Hence here I am making the code consistent with its current limitations. I'm not looking to extend functionality in code that I don't know intimately. I can't do that and do it reliably and guarantee it works as a module when it has never been used as such before in 300+ places all across the kernel.

If there are interested users who want their code tristate and can vouch that their code works OK as such, I can drop the patch(es) here ; this is what happened with 4 of the patches I originally had in v1 of this very series. But a good number of the 300+ instances have been this way since before git history began (2005) and so I wonder the value in say extending instances like old ISA drivers from bool to tristate...

Paul.
--

#6 lore raw

PATCH v2 4/4 drivers/net/ethernet: make ti/cpsw-phy-sel.c explicitly non-modular

In reply to Paul Gortmaker

The Kconfig currently controlling compilation of this code is:

drivers/net/ethernet/ti/Kconfig:config TI_CPSW_PHY_SEL
drivers/net/ethernet/ti/Kconfig: bool "TI CPSW Switch Phy sel Support"

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

Lets remove the couple traces of modularity so that when reading the driver there is no doubt it is builtin-only.

Since module_platform_driver() uses the same init level priority as builtin_platform_driver() the init ordering remains unchanged with this commit.

Also note that MODULE_DEVICE_TABLE is a no-op for non-modular code.

We also delete the MODULE_LICENSE tag etc. since all that information was (or is now) contained at the top of the file in the comments.

---
 drivers/net/ethernet/ti/cpsw-phy-sel.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)
drivers/net/ethernet/ti/cpsw-phy-sel.c +4 -6
index 0ea78326cc21..e9cc61e1ec74 100644
@@ -2,6 +2,8 @@
  *
  * Copyright (C) 2013 Texas Instruments
  *
+ * Module Author: Mugunthan V N <[email protected]>
+ *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
  * version 2 as published by the Free Software Foundation.
@@ -13,7 +15,7 @@
  */
 
 #include <linux/platform_device.h>
-#include <linux/module.h>
+#include <linux/init.h>
 #include <linux/netdevice.h>
 #include <linux/phy.h>
 #include <linux/of.h>
@@ -173,7 +175,6 @@ static const struct of_device_id cpsw_phy_sel_id_table[] = {
 	},
 	{}
 };
-MODULE_DEVICE_TABLE(of, cpsw_phy_sel_id_table);
 
 static int cpsw_phy_sel_probe(struct platform_device *pdev)
 {
@@ -214,7 +215,4 @@ static struct platform_driver cpsw_phy_sel_driver = {
 		.of_match_table = cpsw_phy_sel_id_table,
 	},
 };
-
-module_platform_driver(cpsw_phy_sel_driver);
-MODULE_AUTHOR("Mugunthan V N <[email protected]>");
-MODULE_LICENSE("GPL v2");
+builtin_platform_driver(cpsw_phy_sel_driver);
Signature · 1 line
2.6.0.rc3
maintainer
#7 lore raw
In reply to Paul Gortmaker

From: Paul Gortmaker <[email protected]>
Date: Wed, 7 Oct 2015 17:27:42 -0400

Quoted 23 lines

[v2: drop m68k patches that Geert converted to modules; add one ARM
driver patch ; update net-next baseline to today; switch to ARM for build testing.]

In a previous merge window, we made changes to allow better delineation between modular and non-modular code in commit 0fd972a7d91d6e15393c449492a04d94c0b89351 ("module: relocate module_init from init.h to module.h"). This allows us to now ensure module code looks modular and non-modular code does not accidentally look modular just to avoid suffering build breakage.

Here we target code that is, by nature of their Makefile and/or Kconfig settings, only available to be built-in, but implicitly presenting itself as being possibly modular by way of using modular headers, macros, and functions.

The goal here is to remove that illusion of modularity from these files, but in a way that leaves the actual runtime unchanged. In doing so, we remove code that has never been tested and adds no value to the tree. And we continue the process of expecting a level of consistency between the Kconfig/Makefile of code and the code in use itself.

Quoted 7 lines

Fortuntately the net subsystem has relatively few instances, given the overall amount of code and drivers it contains. For comparison there are over 300 instances tree wide, resulting in a possible net removal of on the order of 5000 lines of unused code.

Build tested on net-next from today, on ARM, since that is the arch where the one ethernet driver changed here is available.

Series applied, thanks Paul.