move unneeded data to initdata section

PATCH Applied Denis V. Lunev 6 participants 9 messages Nov 7, 2007 – Nov 15, 2007 1 message not in the archive

Subsystems (3)

Networking Network drivers Proc filesystem

lore

Share & tools

lore

Denis V. Lunev

author

2007-11-07 11:57 UTC #0 lore raw

PATCH 2/2 move unneeded data to initdata section

This patch reverts Eric's commit 2b008b0a8e96b726c603c5e1a5a7a509b5f61e35

It diets .text & .data section of the kernel if CONFIG_NET_NS is not set. This is safe after list operations cleanup.

Signed-of-by Denis V. Lunev <[email protected]>

drivers/net/loopback.c renamed from drivers/net/loopback.c.reversed +1 -1

@@ -284,7 +284,7 @@ static __net_exit void loopback_net_exit
 	unregister_netdev(dev);
 }
 
-static struct pernet_operations loopback_net_ops = {
+static struct pernet_operations __net_initdata loopback_net_ops = {
        .init = loopback_net_init,
        .exit = loopback_net_exit,
 };

fs/proc/proc_net.c renamed from fs/proc/proc_net.c.reversed +1 -1

@@ -185,7 +185,7 @@ static __net_exit void proc_net_ns_exit(
 	kfree(net->proc_net_root);
 }
 
-static struct pernet_operations proc_net_ns_ops = {
+static struct pernet_operations __net_initdata proc_net_ns_ops = {
 	.init = proc_net_ns_init,
 	.exit = proc_net_ns_exit,
 };

include/net/net_namespace.h renamed from include/net/net_namespace.h.reversed +2 -0

@@ -102,9 +102,11 @@ static inline void release_net(struct ne
 #ifdef CONFIG_NET_NS
 #define __net_init
 #define __net_exit
+#define __net_initdata
 #else
 #define __net_init	__init
 #define __net_exit	__exit_refok
+#define __net_initdata	__initdata
 #endif
 
 struct pernet_operations {

net/core/dev.c renamed from net/core/dev.c.reversed +3 -3

@@ -2676,7 +2676,7 @@ static void __net_exit dev_proc_net_exit
 	proc_net_remove(net, "dev");
 }
 
-static struct pernet_operations dev_proc_ops = {
+static struct pernet_operations __net_initdata dev_proc_ops = {
 	.init = dev_proc_net_init,
 	.exit = dev_proc_net_exit,
 };

@@ -4336,7 +4336,7 @@ static void __net_exit netdev_exit(struc
 	kfree(net->dev_index_head);
 }
 
-static struct pernet_operations  netdev_net_ops = {
+static struct pernet_operations __net_initdata netdev_net_ops = {
 	.init = netdev_init,
 	.exit = netdev_exit,
 };

@@ -4367,7 +4367,7 @@ static void __net_exit default_device_ex
 	rtnl_unlock();
 }
 
-static struct pernet_operations  default_device_ops = {
+static struct pernet_operations __net_initdata default_device_ops = {
 	.exit = default_device_exit,
 };

net/core/dev_mcast.c renamed from net/core/dev_mcast.c.reversed +1 -1

@@ -285,7 +285,7 @@ static void __net_exit dev_mc_net_exit(s
 	proc_net_remove(net, "dev_mcast");
 }
 
-static struct pernet_operations dev_mc_net_ops = {
+static struct pernet_operations __net_initdata dev_mc_net_ops = {
 	.init = dev_mc_net_init,
 	.exit = dev_mc_net_exit,
 };

net/netlink/af_netlink.c renamed from net/netlink/af_netlink.c.reversed +1 -1

@@ -1888,7 +1888,7 @@ static void __net_exit netlink_net_exit(
 #endif
 }
 
-static struct pernet_operations netlink_net_ops = {
+static struct pernet_operations __net_initdata netlink_net_ops = {
 	.init = netlink_net_init,
 	.exit = netlink_net_exit,
 };

David Miller

maintainer Applied

2007-11-13 11:24 UTC #1 lore raw

From: "Denis V. Lunev" <[email protected]>
Date: Wed, 7 Nov 2007 15:01:00 +0300

Quoted 6 lines

This patch reverts Eric's commit 2b008b0a8e96b726c603c5e1a5a7a509b5f61e35
It diets .text & .data section of the kernel if CONFIG_NET_NS is not set. This is safe after list operations cleanup.
Signed-of-by Denis V. Lunev <[email protected]>

Applied, thanks Denis.

Eric W. Biederman

2007-11-15 14:32 UTC #2 lore raw

In reply to Denis V. Lunev

Quoted 4 lines · Denis V. Lunev

This patch reverts Eric's commit 2b008b0a8e96b726c603c5e1a5a7a509b5f61e35
It diets .text & .data section of the kernel if CONFIG_NET_NS is not set. This is safe after list operations cleanup.

Ok. This patch is technically safe because none of the touched code can live in a module and so we never touch the exit code path.

However in the general case and as a code idiom this __net_initdata on struct pernet_operations is fundamentally horribly broken.

Look at what happens if we use this idiom in module. There is only one definition of __initdata ".init.data". The module loader places all sections that begin with .init in a region of memory that will be discarded after module initialization.

So in register_pernet_operations we pass in the a pointer to struct pernet_operations and call the init method. Later when we remove the module we again pass in the pointer to struct pernet_operations which lived in an init section so it has been discarded. We dereference that pointer to find the exit method and KABOOM!!!!

So I'm still opposed to __net_initdata on the grounds that at best it is like putting our head under a guillotine and reaching up and sawing at the row that holds the blade up with a pocket knife. It is a think rope and a puny knife so you are safe for a while....

Eric

Denis V. Lunev

2007-11-15 14:38 UTC #3 lore raw

Quoted 10 lines · Eric W. Biederman

Quoted 4 lines · Denis V. Lunev
This patch reverts Eric's commit 2b008b0a8e96b726c603c5e1a5a7a509b5f61e35
It diets .text & .data section of the kernel if CONFIG_NET_NS is not set. This is safe after list operations cleanup.
Ok. This patch is technically safe because none of the touched code can live in a module and so we never touch the exit code path.

Quoted 7 lines

However in the general case and as a code idiom this __net_initdata on struct pernet_operations is fundamentally horribly broken.
Look at what happens if we use this idiom in module. There is only one definition of __initdata ".init.data". The module loader places all sections that begin with .init in a region of memory that will be discarded after module initialization.

nothing is discarded after module load. Though, I can be wrong. Could you point me to the exact place?

Regards,
Den

Sam Ravnborg

2007-11-15 15:13 UTC #4 lore raw

Quoted 14 lines · Denis V. Lunev

Quoted 13 lines · Eric W. Biederman
Quoted 4 lines · Denis V. Lunev
This patch reverts Eric's commit 2b008b0a8e96b726c603c5e1a5a7a509b5f61e35
It diets .text & .data section of the kernel if CONFIG_NET_NS is not set. This is safe after list operations cleanup.
Ok. This patch is technically safe because none of the touched code can live in a module and so we never touch the exit code path.
However in the general case and as a code idiom this __net_initdata on struct pernet_operations is fundamentally horribly broken.

Quoted 7 lines

Quoted 4 lines
Look at what happens if we use this idiom in module. There is only one definition of __initdata ".init.data". The module loader places all sections that begin with .init in a region of memory that will be discarded after module initialization.
nothing is discarded after module load. Though, I can be wrong. Could you point me to the exact place?

If __initdata is not discarded after module load then we should do it. There is no reason to waste __initdata RAM when the module is loaded.

Sam

Eric W. Biederman

2007-11-15 18:19 UTC #5 lore raw

Quoted 6 lines · Sam Ravnborg

Quoted 3 lines · Denis V. Lunev
nothing is discarded after module load. Though, I can be wrong. Could you point me to the exact place?
If __initdata is not discarded after module load then we should do it. There is no reason to waste __initdata RAM when the module is loaded.

Down at the bottom of sys_init_module we have:

	/* Drop initial reference. */
	module_put(mod);
	unwind_remove_table(mod->unwind_info, 1);

	module_free(mod, mod->module_init);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
	mod->module_init = NULL;
	mod->init_size = 0;
	mod->init_text_size = 0;
	mutex_unlock(&module_mutex);

	return 0;

Which frees the memory for the .init sections.

Eric

Sam Ravnborg

2007-11-15 18:41 UTC #6 lore raw

Quoted 24 lines · Eric W. Biederman

Quoted 6 lines · Sam Ravnborg
Quoted 3 lines · Denis V. Lunev
nothing is discarded after module load. Though, I can be wrong. Could you point me to the exact place?
If __initdata is not discarded after module load then we should do it. There is no reason to waste __initdata RAM when the module is loaded.
Down at the bottom of sys_init_module we have:
	/* Drop initial reference. */
	module_put(mod);
	unwind_remove_table(mod->unwind_info, 1);

	module_free(mod, mod->module_init);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
	mod->module_init = NULL;
	mod->init_size = 0;
	mod->init_text_size = 0;
	mutex_unlock(&module_mutex);

	return 0;

Quoted 1 line

Which frees the memory for the .init sections.

Thanks for clarifying this Eric - should have looked myself..

Sam

A message that is not in the archive (sent off-list or not yet received): 20071115184334.GC23914-QabhHTsIXMSnlFQ6Q1D1Y0B+6BGkLq7r@public.gmane.org

Denis V. Lunev

2007-11-15 19:17 UTC #8 lore raw

Quoted 21 lines · Sam Ravnborg

Quoted 20 lines · Eric W. Biederman
Quoted 5 lines · Sam Ravnborg
Quoted 2 lines · Denis V. Lunev
nothing is discarded after module load. Though, I can be wrong. Could you point me to the exact place?
If __initdata is not discarded after module load then we should do it. There is no reason to waste __initdata RAM when the module is loaded.
Down at the bottom of sys_init_module we have:
	/* Drop initial reference. */
	module_put(mod);
	unwind_remove_table(mod->unwind_info, 1);

	module_free(mod, mod->module_init);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
	mod->module_init = NULL;
	mod->init_size = 0;
	mod->init_text_size = 0;
	mutex_unlock(&module_mutex);

Quoted 5 lines

Quoted 3 lines
	return 0;
Which frees the memory for the .init sections.
Thanks for clarifying this Eric - should have looked myself..

clear :) I was wrong... Thank you for pointing this out.

will you mind against this?

include/net/net_namespace.h +5 -1

index 5dd6d90..d136707 100644
@@ -119,10 +119,14 @@ static inline struct net *maybe_get_net(struct net *net)
 #ifdef CONFIG_NET_NS
 #define __net_init
 #define __net_exit
-#define __net_initdata
 #else
 #define __net_init	__init
 #define __net_exit	__exit_refok
+#endif
+
+#if defined(CONFIG_NET_NS) || defined(MODULE)
+#define __net_initdata
+#else
 #define __net_initdata	__initdata
 #endif

_______________________________________________
Containers mailing list Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Sam Ravnborg

2007-11-15 19:32 UTC #9 lore raw

Quoted 12 lines · Denis V. Lunev

will you mind against this?

include/net/net_namespace.h +0 -1

index 5dd6d90..d136707 100644
@@ -119,10 +119,14 @@ static inline struct net *maybe_get_net(struct net *net)
 #ifdef CONFIG_NET_NS
 #define __net_init
 #define __net_exit
-#define __net_initdata

include/net/net_namespace.h static inline struct net *maybe_get_net(struct net *net) · line 131

Quoted 10 lines

include/net/net_namespace.h +5 -0

 #else
 #define __net_init	__init
 #define __net_exit	__exit_refok
+#endif
+
+#if defined(CONFIG_NET_NS) || defined(MODULE)
+#define __net_initdata
+#else
 #define __net_initdata	__initdata
 #endif

n principle I am against this approach. __initdata is far too overloaded with different stuff.

A much more preferred approach should be to create new sections named for example .init.data.net and .init.data.net.module

And then in include/asm-generic/vmlinux.lds.h decide the location of these sections.

On top of this we would have to teach modpost about these new sections. But the advantage of this approach is that the section mismatch checks are *independent* of the module being a MODULE or build-in. The check will still happen.

In this way we avoid the situation where a warning only pops up in certain configurations.

To do so will obviously require a bit more linker script consolidation but if you or some else could step in a do this it would be great!

Sam