diff mbox series

[v3,1/2] fs/proc: Optimize arrays defined by struct ctl_path

Message ID 20220224133217.1755-1-tangmeng@uniontech.com (mailing list archive)
State Not Applicable
Headers show
Series [v3,1/2] fs/proc: Optimize arrays defined by struct ctl_path | expand

Commit Message

Meng Tang Feb. 24, 2022, 1:32 p.m. UTC
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(-)

Comments

Luis Chamberlain Feb. 26, 2022, 8:57 p.m. UTC | #1
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 mbox series

Patch

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);