Messages in this thread | | | Subject | Re: [PATCH v2] sysctl: Add a group of macro functions to initcall the sysctl table of each feature | From | Xiaoming Ni <> | Date | Sun, 12 Dec 2021 17:58:48 +0800 |
| |
On 2021/12/11 1:20, Luis Chamberlain wrote: > 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) >> +
/**
* sysctl_initcall() - register and init sysctl leaf node to path
* @path: path name for sysctl base
* @table: This is the sysctl leaf table that needs to be registered to the path *
* Leaf node in the sysctl tree:
* a) File, .child = NULL
* b) Directory, which is not shared by multiple features, .child != NULL *
* The sysctl interface for each subfeature should be in the after
* sysctl_init_bases() and before do_sysctl_args().
* sysctl_init_bases() is executed before early_initcall().
* do_sysctl_args() is executed after late_initcall().
* Therefore, it is safe to add leaves to the sysctl tree using late_initcall(). */
How about that description?
>> +#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 > . >
Thanks Xiaoming Ni
| |