Messages in this thread | | | Date | Fri, 10 Dec 2021 09:20:09 -0800 | From | Luis Chamberlain <> | Subject | Re: [PATCH v2] sysctl: Add a group of macro functions to initcall the sysctl table of each feature |
| |
On Fri, Dec 10, 2021 at 04:58:49PM +0800, Xiaoming Ni wrote: > To avoid duplicated code, add a set of macro functions to initialize the > sysctl table for each feature. > > The system initialization process is as follows: > > start_kernel () { > ... > /* init proc and sysctl base, > * proc_root_init()-->proc_sys_init()-->sysctl_init_bases() > */ > proc_root_init(); /* init proc and sysctl base */ > ... > arch_call_rest_init(); > } > > arch_call_rest_init()-->rest_init()-->kernel_init() > kernel_init() { > ... > kernel_init_freeable(); /* do all initcalls */ > ... > do_sysctl_args(); /* Process the sysctl parameter: sysctl.*= */ > } > > kernel_init_freeable()--->do_basic_setup()-->do_initcalls() > do_initcalls() { > for (level = 0; level < ARRAY_SIZE(initcall_levels) - 1; level++) { > do_initcall_level > }
It was nice to have this documented in the commit log, however you don't provide a developer documentation for this in your changes. Can you justify through documentation why we can use init levels with the above information for the sysctl_initcall() macro?
> The sysctl interface of each subfeature should be registered after > sysctl_init_bases() and before do_sysctl_args().
Indeed.
> It seems
Seems is poor judgement for a change in the kernel. It is or not.
> that the sysctl > interface does not depend on initcall_levels. To prevent the sysctl > interface from being initialized before the feature itself. The > lowest-level
Lower to me means early, but since we are talking about time, best to clarify and say the latest init level during kernel bootup.
> late_initcall() is used as the common sysctl interface > registration level. > > Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com> > > --- > v2: > Add a simple checkpatch check. > Add code comment. > v1: > https://lore.kernel.org/lkml/20211207011320.100102-1-nixiaoming@huawei.com/ > --- > fs/coredump.c | 7 +------ > fs/dcache.c | 7 +------ > fs/exec.c | 8 +------- > fs/file_table.c | 7 +------ > fs/inode.c | 7 +------ > fs/locks.c | 7 +------ > fs/namei.c | 8 +------- > fs/namespace.c | 7 +------ > include/linux/sysctl.h | 19 +++++++++++++++++++ > kernel/stackleak.c | 7 +------ > scripts/checkpatch.pl | 6 ++++++ > 11 files changed, 34 insertions(+), 56 deletions(-) > > diff --git a/fs/coredump.c b/fs/coredump.c > index 570d98398668..8f6c6322651d 100644 > --- a/fs/coredump.c > +++ b/fs/coredump.c > @@ -943,12 +943,7 @@ static struct ctl_table coredump_sysctls[] = { > { } > }; > > -static int __init init_fs_coredump_sysctls(void) > -{ > - register_sysctl_init("kernel", coredump_sysctls); > - return 0; > -} > -fs_initcall(init_fs_coredump_sysctls); > +kernel_sysctl_initcall(coredump_sysctls);
Nice.
Yes, although I went with fs_initcall() your documentation above does give us certainty that this is fine as well. No need to kick this through earlier.
> #endif /* CONFIG_SYSCTL */ > > /* > diff --git a/fs/dcache.c b/fs/dcache.c > index 0eef1102f460..c1570243aaee 100644 > --- a/fs/dcache.c > +++ b/fs/dcache.c > @@ -195,12 +195,7 @@ static struct ctl_table fs_dcache_sysctls[] = { > { } > }; > > -static int __init init_fs_dcache_sysctls(void) > -{ > - register_sysctl_init("fs", fs_dcache_sysctls); > - return 0; > -} > -fs_initcall(init_fs_dcache_sysctls); > +fs_sysctl_initcall(fs_dcache_sysctls);
Seems fine by me using the same logic as above and I like that you are splitting this by bases. Likewise for the others, this is looking good.
> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h > index acf0805cf3a0..ce33e61a8287 100644 > --- a/include/linux/sysctl.h > +++ b/include/linux/sysctl.h > @@ -231,6 +231,25 @@ extern int sysctl_init_bases(void); > extern void __register_sysctl_init(const char *path, struct ctl_table *table, > const char *table_name);
Yes please take the time to write some documentation here which can explain to developers *why* we use the init levels specified.
> #define register_sysctl_init(path, table) __register_sysctl_init(path, table, #table) > + > +#define sysctl_initcall(path, table) \ > + static int __init init_##table(void) \ > + { \ > + register_sysctl_init(path, table); \ > + return 0;\ > + } \ > + late_initcall(init_##table) > + > +/* > + * Use xxx_sysctl_initcall() to initialize your sysctl interface unless you want > + * to register the sysctl directory and share it with other features. > + */ > +#define kernel_sysctl_initcall(table) sysctl_initcall("kernel", table) > +#define fs_sysctl_initcall(table) sysctl_initcall("fs", table) > +#define vm_sysctl_initcall(table) sysctl_initcall("vm", table) > +#define debug_sysctl_initcall(table) sysctl_initcall("debug", table) > +#define dev_sysctl_initcall(table) sysctl_initcall("dev", table) > + > extern struct ctl_table_header *register_sysctl_mount_point(const char *path); > > void do_sysctl_args(void);
Luis
| |