[PATCH 0/6] s390: simplify sysctl registration

[PATCH 0/6] s390: simplify sysctl registration

[Luis Chamberlain]

s390 is the last architecture and one of the last users of
register_sysctl_table(). It was last becuase it had one use case
with dynamic memory allocation and it just required a bit more
thought.

This is all being done to help reduce code and avoid usage of API
calls for sysctl registration that can incur recusion. The recursion
only happens when you have subdirectories with entries and s390 does
not have any of that so either way recursion is avoided. Long term
though we can do away with just removing register_sysctl_table()
and then using ARRAY_SIZE() and save us tons of memory all over the
place by not having to add an extra empty entry all over.

Hopefully that commit log suffices for the dynamic allocation
conversion, but I would really like someone to test it as I haven't
tested a single patch, I'm super guiltly to accept I've just waited for
patches to finish compile testing and that's not over yet.

Anyway the changes other than the dynamic allocation one are pretty
trivial. That one could use some good review.

With all this out of the way we have just one stupid last user of
register_sysctl_table(): drivers/parport/procfs.c

That one is also dynamic. Hopefully the maintainer will be motivated
to do that conversion with all the examples out there now and this
having a complex one.

For more information refer to the new docs:

https://lore.kernel.org/all/20230310223947.3917711-1-mcgrof@kernel.org/T/#u     
 
Luis Chamberlain (6):
  s390: simplify one-level sysctl registration for topology_ctl_table
  s390: simplify one-level syctl registration for s390dbf_table
  s390: simplify one-level sysctl registration for appldata_table
  s390: simplify one level sysctl registration for cmm_table
  s390: simplify one-level sysctl registration for page_table_sysctl
  s390: simplify dynamic sysctl registration for appldata_register_ops

 arch/s390/appldata/appldata_base.c | 30 ++++++++----------------------
 arch/s390/kernel/debug.c           | 12 +-----------
 arch/s390/kernel/topology.c        | 12 +-----------
 arch/s390/mm/cmm.c                 | 12 +-----------
 arch/s390/mm/pgalloc.c             | 12 +-----------
 5 files changed, 12 insertions(+), 66 deletions(-)

-- 
2.39.1

[PATCH 1/6] s390: simplify one-level sysctl registration for topology_ctl_table

[Luis Chamberlain]

There is no need to declare an extra tables to just create directory,
this can be easily be done with a prefix path with register_sysctl().

Simplify this registration.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 arch/s390/kernel/topology.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/arch/s390/kernel/topology.c b/arch/s390/kernel/topology.c
index c6eecd4a5302..e5d6a1c25d13 100644
--- a/arch/s390/kernel/topology.c
+++ b/arch/s390/kernel/topology.c
@@ -637,16 +637,6 @@ static struct ctl_table topology_ctl_table[] = {
 	{ },
 };
 
-static struct ctl_table topology_dir_table[] = {
-	{
-		.procname	= "s390",
-		.maxlen		= 0,
-		.mode		= 0555,
-		.child		= topology_ctl_table,
-	},
-	{ },
-};
-
 static int __init topology_init(void)
 {
 	timer_setup(&topology_timer, topology_timer_fn, TIMER_DEFERRABLE);
@@ -654,7 +644,7 @@ static int __init topology_init(void)
 		set_topology_timer();
 	else
 		topology_update_polarization_simple();
-	register_sysctl_table(topology_dir_table);
+	register_sysctl("s390", topology_ctl_table);
 	return device_create_file(cpu_subsys.dev_root, &dev_attr_dispatching);
 }
 device_initcall(topology_init);
-- 
2.39.1

[PATCH 2/6] s390: simplify one-level syctl registration for s390dbf_table

[Luis Chamberlain]

There is no need to declare an extra tables to just create directory,
this can be easily be done with a prefix path with register_sysctl().

Simplify this registration.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 arch/s390/kernel/debug.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/arch/s390/kernel/debug.c b/arch/s390/kernel/debug.c
index b376f0377a2c..221c865785c2 100644
--- a/arch/s390/kernel/debug.c
+++ b/arch/s390/kernel/debug.c
@@ -981,16 +981,6 @@ static struct ctl_table s390dbf_table[] = {
 	{ }
 };
 
-static struct ctl_table s390dbf_dir_table[] = {
-	{
-		.procname	= "s390dbf",
-		.maxlen		= 0,
-		.mode		= S_IRUGO | S_IXUGO,
-		.child		= s390dbf_table,
-	},
-	{ }
-};
-
 static struct ctl_table_header *s390dbf_sysctl_header;
 
 /**
@@ -1574,7 +1564,7 @@ static int debug_sprintf_format_fn(debug_info_t *id, struct debug_view *view,
  */
 static int __init debug_init(void)
 {
-	s390dbf_sysctl_header = register_sysctl_table(s390dbf_dir_table);
+	s390dbf_sysctl_header = register_sysctl("s390dbf", s390dbf_table);
 	mutex_lock(&debug_mutex);
 	debug_debugfs_root_entry = debugfs_create_dir(DEBUG_DIR_ROOT, NULL);
 	initialized = 1;
-- 
2.39.1

[PATCH 3/6] s390: simplify one-level sysctl registration for appldata_table

[Luis Chamberlain]

There is no need to declare an extra tables to just create directory,
this can be easily be done with a prefix path with register_sysctl().

Simplify this registration.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 arch/s390/appldata/appldata_base.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/arch/s390/appldata/appldata_base.c b/arch/s390/appldata/appldata_base.c
index c0fd29133f27..c593f2228083 100644
--- a/arch/s390/appldata/appldata_base.c
+++ b/arch/s390/appldata/appldata_base.c
@@ -66,16 +66,6 @@ static struct ctl_table appldata_table[] = {
 	{ },
 };
 
-static struct ctl_table appldata_dir_table[] = {
-	{
-		.procname	= appldata_proc_name,
-		.maxlen		= 0,
-		.mode		= S_IRUGO | S_IXUGO,
-		.child		= appldata_table,
-	},
-	{ },
-};
-
 /*
  * Timer
  */
@@ -422,7 +412,7 @@ static int __init appldata_init(void)
 	appldata_wq = alloc_ordered_workqueue("appldata", 0);
 	if (!appldata_wq)
 		return -ENOMEM;
-	appldata_sysctl_header = register_sysctl_table(appldata_dir_table);
+	appldata_sysctl_header = register_sysctl(appldata_proc_name, appldata_table);
 	return 0;
 }
 
-- 
2.39.1

[PATCH 4/6] s390: simplify one level sysctl registration for cmm_table

[Luis Chamberlain]

There is no need to declare an extra tables to just create directory,
this can be easily be done with a prefix path with register_sysctl().

Simplify this registration.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 arch/s390/mm/cmm.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/arch/s390/mm/cmm.c b/arch/s390/mm/cmm.c
index 9141ed4c52e9..5300c6867d5e 100644
--- a/arch/s390/mm/cmm.c
+++ b/arch/s390/mm/cmm.c
@@ -335,16 +335,6 @@ static struct ctl_table cmm_table[] = {
 	{ }
 };
 
-static struct ctl_table cmm_dir_table[] = {
-	{
-		.procname	= "vm",
-		.maxlen		= 0,
-		.mode		= 0555,
-		.child		= cmm_table,
-	},
-	{ }
-};
-
 #ifdef CONFIG_CMM_IUCV
 #define SMSG_PREFIX "CMM"
 static void cmm_smsg_target(const char *from, char *msg)
@@ -389,7 +379,7 @@ static int __init cmm_init(void)
 {
 	int rc = -ENOMEM;
 
-	cmm_sysctl_header = register_sysctl_table(cmm_dir_table);
+	cmm_sysctl_header = register_sysctl("vm", cmm_table);
 	if (!cmm_sysctl_header)
 		goto out_sysctl;
 #ifdef CONFIG_CMM_IUCV
-- 
2.39.1

[PATCH 5/6] s390: simplify one-level sysctl registration for page_table_sysctl

[Luis Chamberlain]

There is no need to declare an extra tables to just create directory,
this can be easily be done with a prefix path with register_sysctl().

Simplify this registration.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 arch/s390/mm/pgalloc.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/arch/s390/mm/pgalloc.c b/arch/s390/mm/pgalloc.c
index 2de48b2c1b04..0f68b7257e08 100644
--- a/arch/s390/mm/pgalloc.c
+++ b/arch/s390/mm/pgalloc.c
@@ -33,19 +33,9 @@ static struct ctl_table page_table_sysctl[] = {
 	{ }
 };
 
-static struct ctl_table page_table_sysctl_dir[] = {
-	{
-		.procname	= "vm",
-		.maxlen		= 0,
-		.mode		= 0555,
-		.child		= page_table_sysctl,
-	},
-	{ }
-};
-
 static int __init page_table_register_sysctl(void)
 {
-	return register_sysctl_table(page_table_sysctl_dir) ? 0 : -ENOMEM;
+	return register_sysctl("vm", page_table_sysctl) ? 0 : -ENOMEM;
 }
 __initcall(page_table_register_sysctl);
 
-- 
2.39.1

[PATCH 6/6] s390: simplify dynamic sysctl registration for appldata_register_ops

[Luis Chamberlain]

The routine appldata_register_ops() allocates a sysctl table
with 4 entries. The firsts one,   ops->ctl_table[0] is the parent directory
with an empty entry following it, ops->ctl_table[1]. The next entry is
for the the ops->name and that is ops->ctl_table[2]. It needs an empty
entry following that, and that is ops->ctl_table[3]. And so hence the
kcalloc(4, sizeof(struct ctl_table), GFP_KERNEL).

We can simplify this considerably since sysctl_register("foo", table)
can create the parent directory for us if it does not exist. So we
can just remove the first two entries and move back the ops->name to
the first entry, and just use kcalloc(2, ...).

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 arch/s390/appldata/appldata_base.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/arch/s390/appldata/appldata_base.c b/arch/s390/appldata/appldata_base.c
index c593f2228083..a60c1e093039 100644
--- a/arch/s390/appldata/appldata_base.c
+++ b/arch/s390/appldata/appldata_base.c
@@ -351,7 +351,8 @@ int appldata_register_ops(struct appldata_ops *ops)
 	if (ops->size > APPLDATA_MAX_REC_SIZE)
 		return -EINVAL;
 
-	ops->ctl_table = kcalloc(4, sizeof(struct ctl_table), GFP_KERNEL);
+	/* The last entry must be an empty one */
+	ops->ctl_table = kcalloc(2, sizeof(struct ctl_table), GFP_KERNEL);
 	if (!ops->ctl_table)
 		return -ENOMEM;
 
@@ -359,17 +360,12 @@ int appldata_register_ops(struct appldata_ops *ops)
 	list_add(&ops->list, &appldata_ops_list);
 	mutex_unlock(&appldata_ops_mutex);
 
-	ops->ctl_table[0].procname = appldata_proc_name;
-	ops->ctl_table[0].maxlen   = 0;
-	ops->ctl_table[0].mode     = S_IRUGO | S_IXUGO;
-	ops->ctl_table[0].child    = &ops->ctl_table[2];
+	ops->ctl_table[0].procname = ops->name;
+	ops->ctl_table[0].mode     = S_IRUGO | S_IWUSR;
+	ops->ctl_table[0].proc_handler = appldata_generic_handler;
+	ops->ctl_table[0].data = ops;
 
-	ops->ctl_table[2].procname = ops->name;
-	ops->ctl_table[2].mode     = S_IRUGO | S_IWUSR;
-	ops->ctl_table[2].proc_handler = appldata_generic_handler;
-	ops->ctl_table[2].data = ops;
-
-	ops->sysctl_header = register_sysctl_table(ops->ctl_table);
+	ops->sysctl_header = register_sysctl(appldata_proc_name, ops->ctl_table);
 	if (!ops->sysctl_header)
 		goto out;
 	return 0;
-- 
2.39.1

Re: [PATCH 6/6] s390: simplify dynamic sysctl registration for appldata_register_ops

[Vasily Gorbik]

On Fri, Mar 10, 2023 at 03:45:25PM -0800, Luis Chamberlain wrote:
> The routine appldata_register_ops() allocates a sysctl table
> with 4 entries. The firsts one,   ops->ctl_table[0] is the parent directory
> with an empty entry following it, ops->ctl_table[1]. The next entry is
> for the the ops->name and that is ops->ctl_table[2]. It needs an empty

for the ops->name

> entry following that, and that is ops->ctl_table[3]. And so hence the
> kcalloc(4, sizeof(struct ctl_table), GFP_KERNEL).
> 
> We can simplify this considerably since sysctl_register("foo", table)
> can create the parent directory for us if it does not exist. So we
> can just remove the first two entries and move back the ops->name to
> the first entry, and just use kcalloc(2, ...).
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>  arch/s390/appldata/appldata_base.c | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/s390/appldata/appldata_base.c b/arch/s390/appldata/appldata_base.c
> index c593f2228083..a60c1e093039 100644
> --- a/arch/s390/appldata/appldata_base.c
> +++ b/arch/s390/appldata/appldata_base.c
> @@ -351,7 +351,8 @@ int appldata_register_ops(struct appldata_ops *ops)
>  	if (ops->size > APPLDATA_MAX_REC_SIZE)
>  		return -EINVAL;
>  
> -	ops->ctl_table = kcalloc(4, sizeof(struct ctl_table), GFP_KERNEL);
> +	/* The last entry must be an empty one */
> +	ops->ctl_table = kcalloc(2, sizeof(struct ctl_table), GFP_KERNEL);
>  	if (!ops->ctl_table)
>  		return -ENOMEM;
>  
> @@ -359,17 +360,12 @@ int appldata_register_ops(struct appldata_ops *ops)
>  	list_add(&ops->list, &appldata_ops_list);
>  	mutex_unlock(&appldata_ops_mutex);
>  
> -	ops->ctl_table[0].procname = appldata_proc_name;
> -	ops->ctl_table[0].maxlen   = 0;
> -	ops->ctl_table[0].mode     = S_IRUGO | S_IXUGO;
> -	ops->ctl_table[0].child    = &ops->ctl_table[2];
> +	ops->ctl_table[0].procname = ops->name;
> +	ops->ctl_table[0].mode     = S_IRUGO | S_IWUSR;
> +	ops->ctl_table[0].proc_handler = appldata_generic_handler;
> +	ops->ctl_table[0].data = ops;
>  
> -	ops->ctl_table[2].procname = ops->name;
> -	ops->ctl_table[2].mode     = S_IRUGO | S_IWUSR;
> -	ops->ctl_table[2].proc_handler = appldata_generic_handler;
> -	ops->ctl_table[2].data = ops;
> -
> -	ops->sysctl_header = register_sysctl_table(ops->ctl_table);
> +	ops->sysctl_header = register_sysctl(appldata_proc_name, ops->ctl_table);
>  	if (!ops->sysctl_header)
>  		goto out;
>  	return 0;
> -- 
> 2.39.1

I'll take it with the commit message change mentioned above and the following fixup,
which addresses the obvious problem found during testing:
---
diff --git a/arch/s390/appldata/appldata_base.c b/arch/s390/appldata/appldata_base.c
index a60c1e093039..f462d60679d7 100644
--- a/arch/s390/appldata/appldata_base.c
+++ b/arch/s390/appldata/appldata_base.c
@@ -281,7 +281,7 @@ appldata_generic_handler(struct ctl_table *ctl, int write,
        mutex_lock(&appldata_ops_mutex);
        list_for_each(lh, &appldata_ops_list) {
                tmp_ops = list_entry(lh, struct appldata_ops, list);
-               if (&tmp_ops->ctl_table[2] == ctl) {
+               if (&tmp_ops->ctl_table[0] == ctl) {
                        found = 1;
                }
        }
--

Re: [PATCH 0/6] s390: simplify sysctl registration

[Vasily Gorbik]

On Fri, Mar 10, 2023 at 03:45:19PM -0800, Luis Chamberlain wrote:
> s390 is the last architecture and one of the last users of
> register_sysctl_table(). It was last becuase it had one use case
> with dynamic memory allocation and it just required a bit more
> thought.
> 
> This is all being done to help reduce code and avoid usage of API
> calls for sysctl registration that can incur recusion. The recursion
> only happens when you have subdirectories with entries and s390 does
> not have any of that so either way recursion is avoided. Long term
> though we can do away with just removing register_sysctl_table()
> and then using ARRAY_SIZE() and save us tons of memory all over the
> place by not having to add an extra empty entry all over.
> 
> Hopefully that commit log suffices for the dynamic allocation
> conversion, but I would really like someone to test it as I haven't
> tested a single patch, I'm super guiltly to accept I've just waited for
> patches to finish compile testing and that's not over yet.
> 
> Anyway the changes other than the dynamic allocation one are pretty
> trivial. That one could use some good review.
> 
> With all this out of the way we have just one stupid last user of
> register_sysctl_table(): drivers/parport/procfs.c
> 
> That one is also dynamic. Hopefully the maintainer will be motivated
> to do that conversion with all the examples out there now and this
> having a complex one.
> 
> For more information refer to the new docs:
> 
> https://lore.kernel.org/all/20230310223947.3917711-1-mcgrof@kernel.org/T/#u     
>  
> Luis Chamberlain (6):
>   s390: simplify one-level sysctl registration for topology_ctl_table
>   s390: simplify one-level syctl registration for s390dbf_table
>   s390: simplify one-level sysctl registration for appldata_table
>   s390: simplify one level sysctl registration for cmm_table
>   s390: simplify one-level sysctl registration for page_table_sysctl
>   s390: simplify dynamic sysctl registration for appldata_register_ops
> 
>  arch/s390/appldata/appldata_base.c | 30 ++++++++----------------------
>  arch/s390/kernel/debug.c           | 12 +-----------
>  arch/s390/kernel/topology.c        | 12 +-----------
>  arch/s390/mm/cmm.c                 | 12 +-----------
>  arch/s390/mm/pgalloc.c             | 12 +-----------
>  5 files changed, 12 insertions(+), 66 deletions(-)

I've added my
Reviewed-by: Vasily Gorbik <gor@linux.ibm.com>
for the entire patch series.

And applied with the fixup for last change (see corresponding reply).
Thank you!