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 <[email protected]>
---
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++) {
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);
--
2.20.1
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.
The optimization has been implemented in the previous patch,
here to remove unnecessary terminate ctl_path with an empty one.
Signed-off-by: Meng Tang <[email protected]>
---
arch/csky/abiv1/alignment.c | 5 ++---
arch/nds32/mm/alignment.c | 5 ++---
fs/verity/signature.c | 3 +--
kernel/pid_namespace.c | 2 +-
kernel/seccomp.c | 3 +--
security/apparmor/lsm.c | 3 +--
security/loadpin/loadpin.c | 3 +--
security/yama/yama_lsm.c | 3 +--
8 files changed, 10 insertions(+), 17 deletions(-)
diff --git a/arch/csky/abiv1/alignment.c b/arch/csky/abiv1/alignment.c
index 2df115d0e210..5c2936b29d29 100644
--- a/arch/csky/abiv1/alignment.c
+++ b/arch/csky/abiv1/alignment.c
@@ -340,9 +340,8 @@ static struct ctl_table sysctl_table[2] = {
{}
};
-static struct ctl_path sysctl_path[2] = {
- {.procname = "csky"},
- {}
+static struct ctl_path sysctl_path[1] = {
+ {.procname = "csky"}
};
static int __init csky_alignment_init(void)
diff --git a/arch/nds32/mm/alignment.c b/arch/nds32/mm/alignment.c
index 1eb7ded6992b..5e79c01b91d6 100644
--- a/arch/nds32/mm/alignment.c
+++ b/arch/nds32/mm/alignment.c
@@ -560,9 +560,8 @@ static struct ctl_table nds32_sysctl_table[2] = {
{}
};
-static struct ctl_path nds32_path[2] = {
- {.procname = "nds32"},
- {}
+static struct ctl_path nds32_path[1] = {
+ {.procname = "nds32"}
};
/*
diff --git a/fs/verity/signature.c b/fs/verity/signature.c
index 143a530a8008..6cdad230c438 100644
--- a/fs/verity/signature.c
+++ b/fs/verity/signature.c
@@ -92,8 +92,7 @@ static struct ctl_table_header *fsverity_sysctl_header;
static const struct ctl_path fsverity_sysctl_path[] = {
{ .procname = "fs", },
- { .procname = "verity", },
- { }
+ { .procname = "verity", }
};
static struct ctl_table fsverity_sysctl_table[] = {
diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
index a46a3723bc66..f4f6db65bf81 100644
--- a/kernel/pid_namespace.c
+++ b/kernel/pid_namespace.c
@@ -294,7 +294,7 @@ static struct ctl_table pid_ns_ctl_table[] = {
},
{ }
};
-static struct ctl_path kern_path[] = { { .procname = "kernel", }, { } };
+static struct ctl_path kern_path[] = { { .procname = "kernel", } };
#endif /* CONFIG_CHECKPOINT_RESTORE */
int reboot_pid_ns(struct pid_namespace *pid_ns, int cmd)
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index db10e73d06e0..03f88d0b79f1 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -2333,8 +2333,7 @@ static int seccomp_actions_logged_handler(struct ctl_table *ro_table, int write,
static struct ctl_path seccomp_sysctl_path[] = {
{ .procname = "kernel", },
- { .procname = "seccomp", },
- { }
+ { .procname = "seccomp", }
};
static struct ctl_table seccomp_sysctl_table[] = {
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 4f0eecb67dde..e35c3b29742d 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -1729,8 +1729,7 @@ static int apparmor_dointvec(struct ctl_table *table, int write,
}
static struct ctl_path apparmor_sysctl_path[] = {
- { .procname = "kernel", },
- { }
+ { .procname = "kernel", }
};
static struct ctl_table apparmor_sysctl_table[] = {
diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
index b12f7d986b1e..0471b177d2e1 100644
--- a/security/loadpin/loadpin.c
+++ b/security/loadpin/loadpin.c
@@ -48,8 +48,7 @@ static DEFINE_SPINLOCK(pinned_root_spinlock);
static struct ctl_path loadpin_sysctl_path[] = {
{ .procname = "kernel", },
- { .procname = "loadpin", },
- { }
+ { .procname = "loadpin", }
};
static struct ctl_table loadpin_sysctl_table[] = {
diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
index 06e226166aab..b42b61e801b1 100644
--- a/security/yama/yama_lsm.c
+++ b/security/yama/yama_lsm.c
@@ -449,8 +449,7 @@ static int max_scope = YAMA_SCOPE_NO_ATTACH;
static struct ctl_path yama_sysctl_path[] = {
{ .procname = "kernel", },
- { .procname = "yama", },
- { }
+ { .procname = "yama", }
};
static struct ctl_table yama_sysctl_table[] = {
--
2.20.1
On Thu, Feb 24, 2022 at 09:32:17PM +0800, Meng Tang wrote:
> 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.
>
> The optimization has been implemented in the previous patch,
> here to remove unnecessary terminate ctl_path with an empty one.
>
> Signed-off-by: Meng Tang <[email protected]>
If these things are built-in, can you verify you're saving bytes
with size before and after the patch?
I wonder if having something like DECLARE_SYSCTL_PATH_ONE() would make
this nicer on the eyes, and also useful for the other changes you
are making. Do you have many other single path entries you are changing
later?
Luis
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 <[email protected]>
> Signed-off-by: Meng Tang <[email protected]>
> ---
> 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