| Message ID | 20220224133217.1755-1-tangmeng@uniontech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/2] fs/proc: Optimize arrays defined by struct ctl_path | expand |
Please include Eric and Willy (Willy because he suggested this) on future iterations. On Thu, Feb 24, 2022 at 09:32:16PM +0800, Meng Tang wrote: > Previously, arrays defined by struct ctl_path is terminated Not previousy, as it is today. > with an empty one. When we actually only register one ctl_path, > we've gone from 8 bytes to 16 bytes. This doesn't real well, can you elaborate that this implies we are adding an extra 8 bytes by requiring the termination of a path to be empty? You can even quantify the effect of this by building a allyesconfig kernel, measuring the size before and then after your patches to see how many bytes are saved and reflecting that on your commit log. Another thing your commit log should mention is that while this minor optimization might not be so critical before, as we start moving sysctls out from kernel/sysctl.c to their own files we are often then also bloating the kernel slightly when doing this. This work prevents that. > So, I use ARRAY_SIZE() as a boundary condition to optimize it. > > Since the original __register_sysctl_paths is only used in > fs/proc/proc_sysctl.c, in order to not change the usage of > register_sysctl_paths, delete __register_sysctl_paths from > include/linux/sysctl.h, change it to __register_sysctl_paths_init > in fs/proc/proc_sysctl.c, and modify it with static. > The register_sysctl_paths becomes __register_sysctl_paths, > and the macro definition is used in include/linux/sysctl.h > to expand register_sysctl_paths(path, table) to > __register_sysctl_paths(path, ARRAY_SIZE(path), table). Please split this up into a 2 patches, one which moves __register_sysctl_paths() to fs/proc/proc_sysctl.c and make it static. Doing this separately will make the other work easier to review. Then your second patch can use do the other stuff. Add: Suggested-by: Matthew Wilcox <willy@infradead.org> > Signed-off-by: Meng Tang <tangmeng@uniontech.com> > --- > fs/proc/proc_sysctl.c | 22 +++++++++++++--------- > include/linux/sysctl.h | 9 ++++----- > 2 files changed, 17 insertions(+), 14 deletions(-) > > diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c > index 9ecd5c87e8dd..721a8bec63d6 100644 > --- a/fs/proc/proc_sysctl.c > +++ b/fs/proc/proc_sysctl.c > @@ -1589,9 +1589,10 @@ static int register_leaf_sysctl_tables(const char *path, char *pos, > } > > /** > - * __register_sysctl_paths - register a sysctl table hierarchy > + * __register_sysctl_paths_init - register a sysctl table hierarchy > * @set: Sysctl tree to register on > * @path: The path to the directory the sysctl table is in. > + * @ctl_path_num: The numbers(ARRAY_SIZE(path)) of ctl_path > * @table: the top-level table structure > * > * Register a sysctl table hierarchy. @table should be a filled in ctl_table > @@ -1599,22 +1600,23 @@ static int register_leaf_sysctl_tables(const char *path, char *pos, > * > * See __register_sysctl_table for more details. > */ > -struct ctl_table_header *__register_sysctl_paths( > +static struct ctl_table_header *__register_sysctl_paths_init( > struct ctl_table_set *set, > - const struct ctl_path *path, struct ctl_table *table) > + const struct ctl_path *path, int ctl_path_num, struct ctl_table *table) > { > struct ctl_table *ctl_table_arg = table; > int nr_subheaders = count_subheaders(table); > struct ctl_table_header *header = NULL, **subheaders, **subheader; > const struct ctl_path *component; > char *new_path, *pos; > + int i; > > pos = new_path = kmalloc(PATH_MAX, GFP_KERNEL); > if (!new_path) > return NULL; > > pos[0] = '\0'; > - for (component = path; component->procname; component++) { > + for (component = path, i = 0; component->procname && i < ctl_path_num; component++, i++) { I'd refer if the bounds check is done first, so i < ctl_path_num before the component->procname check. > pos = append_path(new_path, pos, component->procname); > if (!pos) > goto out; > @@ -1663,20 +1665,22 @@ struct ctl_table_header *__register_sysctl_paths( > /** > * register_sysctl_paths - register a sysctl table hierarchy You are changing the routine name below but not here. Luis
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c index 9ecd5c87e8dd..721a8bec63d6 100644 --- a/fs/proc/proc_sysctl.c +++ b/fs/proc/proc_sysctl.c @@ -1589,9 +1589,10 @@ static int register_leaf_sysctl_tables(const char *path, char *pos, } /** - * __register_sysctl_paths - register a sysctl table hierarchy + * __register_sysctl_paths_init - register a sysctl table hierarchy * @set: Sysctl tree to register on * @path: The path to the directory the sysctl table is in. + * @ctl_path_num: The numbers(ARRAY_SIZE(path)) of ctl_path * @table: the top-level table structure * * Register a sysctl table hierarchy. @table should be a filled in ctl_table @@ -1599,22 +1600,23 @@ static int register_leaf_sysctl_tables(const char *path, char *pos, * * See __register_sysctl_table for more details. */ -struct ctl_table_header *__register_sysctl_paths( +static struct ctl_table_header *__register_sysctl_paths_init( struct ctl_table_set *set, - const struct ctl_path *path, struct ctl_table *table) + const struct ctl_path *path, int ctl_path_num, struct ctl_table *table) { struct ctl_table *ctl_table_arg = table; int nr_subheaders = count_subheaders(table); struct ctl_table_header *header = NULL, **subheaders, **subheader; const struct ctl_path *component; char *new_path, *pos; + int i; pos = new_path = kmalloc(PATH_MAX, GFP_KERNEL); if (!new_path) return NULL; pos[0] = '\0'; - for (component = path; component->procname; component++) { + for (component = path, i = 0; component->procname && i < ctl_path_num; component++, i++) { pos = append_path(new_path, pos, component->procname); if (!pos) goto out; @@ -1663,20 +1665,22 @@ struct ctl_table_header *__register_sysctl_paths( /** * register_sysctl_paths - register a sysctl table hierarchy * @path: The path to the directory the sysctl table is in. + * @ctl_path_num: The numbers(ARRAY_SIZE(path)) of ctl_path * @table: the top-level table structure * * Register a sysctl table hierarchy. @table should be a filled in ctl_table * array. A completely 0 filled entry terminates the table. * - * See __register_sysctl_paths for more details. + * See __register_sysctl_paths_init for more details. */ -struct ctl_table_header *register_sysctl_paths(const struct ctl_path *path, +struct ctl_table_header *__register_sysctl_paths(const struct ctl_path *path, + int ctl_path_num, struct ctl_table *table) { - return __register_sysctl_paths(&sysctl_table_root.default_set, - path, table); + return __register_sysctl_paths_init(&sysctl_table_root.default_set, + path, ctl_path_num, table); } -EXPORT_SYMBOL(register_sysctl_paths); +EXPORT_SYMBOL(__register_sysctl_paths); /** * register_sysctl_table - register a sysctl table hierarchy diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h index 889c995d8a08..37958aeecfb5 100644 --- a/include/linux/sysctl.h +++ b/include/linux/sysctl.h @@ -219,13 +219,12 @@ extern void retire_sysctl_set(struct ctl_table_set *set); struct ctl_table_header *__register_sysctl_table( struct ctl_table_set *set, const char *path, struct ctl_table *table); -struct ctl_table_header *__register_sysctl_paths( - struct ctl_table_set *set, - const struct ctl_path *path, struct ctl_table *table); struct ctl_table_header *register_sysctl(const char *path, struct ctl_table *table); struct ctl_table_header *register_sysctl_table(struct ctl_table * table); -struct ctl_table_header *register_sysctl_paths(const struct ctl_path *path, - struct ctl_table *table); +#define register_sysctl_paths(path, table) \ + __register_sysctl_paths(path, ARRAY_SIZE(path), table) +extern struct ctl_table_header *__register_sysctl_paths(const struct ctl_path *path, + int ctl_path_num, struct ctl_table *table); void unregister_sysctl_table(struct ctl_table_header * table);
Previously, arrays defined by struct ctl_path is terminated with an empty one. When we actually only register one ctl_path, we've gone from 8 bytes to 16 bytes. So, I use ARRAY_SIZE() as a boundary condition to optimize it. Since the original __register_sysctl_paths is only used in fs/proc/proc_sysctl.c, in order to not change the usage of register_sysctl_paths, delete __register_sysctl_paths from include/linux/sysctl.h, change it to __register_sysctl_paths_init in fs/proc/proc_sysctl.c, and modify it with static. The register_sysctl_paths becomes __register_sysctl_paths, and the macro definition is used in include/linux/sysctl.h to expand register_sysctl_paths(path, table) to __register_sysctl_paths(path, ARRAY_SIZE(path), table). Signed-off-by: Meng Tang <tangmeng@uniontech.com> --- fs/proc/proc_sysctl.c | 22 +++++++++++++--------- include/linux/sysctl.h | 9 ++++----- 2 files changed, 17 insertions(+), 14 deletions(-)