[PATCH v3 0/5] sunrpc: simplfy sysctl registrations

[PATCH v3 0/5] sunrpc: simplfy sysctl registrations

[Luis Chamberlain]

This is my v3 series to simplify sysctl registration for sunrpc. The
first series was posted just yesterday [0] but 0-day found an issue with
CONFIG_SUNRPC_DEBUG. After this fix I poasted a fix for v2 [1] but alas
0-day then found an issue when CONFIG_SUNRPC_DEBUG is disabled. This
fixes both cases... hopefully that's it.
                                                                                                                                                                                              
Changes on v3:

   o Fix compilation when CONFIG_SUNRPC_DEBUG is disabled.. forgot to
     keep all the sysctl stuff under the #ifdef.

Changes on v2:

   o Fix compilation when CONFIG_SUNRPC_DEBUG is enabled, I forgot to move the
     proc routines above, and so the 4th patch now does that too.
                                                                                                                                                                                              
Feel free to take these patches or let me know and I'm happy to also
take these in through sysctl-next. Typically I use sysctl-next for
core sysctl changes or for kernel/sysctl.c cleanup to avoid conflicts.
All these syctls however are well contained to sunrpc so they can also
go in separately. Let me know how you'd like to go about these patches.
                                                                                                                                                                                              
[0] https://lkml.kernel.org/r/20230310225236.3939443-1-mcgrof@kernel.org

Luis Chamberlain (5):
  sunrpc: simplify two-level sysctl registration for tsvcrdma_parm_table
  sunrpc: simplify one-level sysctl registration for xr_tunables_table
  sunrpc: simplify one-level sysctl registration for xs_tunables_table
  sunrpc: move sunrpc_table and proc routines above
  sunrpc: simplify one-level sysctl registration for debug_table

 net/sunrpc/sysctl.c             | 42 ++++++++++++---------------------
 net/sunrpc/xprtrdma/svc_rdma.c  | 21 ++---------------
 net/sunrpc/xprtrdma/transport.c | 11 +--------
 net/sunrpc/xprtsock.c           | 13 ++--------
 4 files changed, 20 insertions(+), 67 deletions(-)

-- 
2.39.1

[PATCH v3 1/5] sunrpc: simplify two-level sysctl registration for tsvcrdma_parm_table

[Luis Chamberlain]

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

Simplify this registration.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 net/sunrpc/xprtrdma/svc_rdma.c | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/net/sunrpc/xprtrdma/svc_rdma.c b/net/sunrpc/xprtrdma/svc_rdma.c
index 5bc20e9d09cd..f0d5eeed4c88 100644
--- a/net/sunrpc/xprtrdma/svc_rdma.c
+++ b/net/sunrpc/xprtrdma/svc_rdma.c
@@ -212,24 +212,6 @@ static struct ctl_table svcrdma_parm_table[] = {
 	{ },
 };
 
-static struct ctl_table svcrdma_table[] = {
-	{
-		.procname	= "svc_rdma",
-		.mode		= 0555,
-		.child		= svcrdma_parm_table
-	},
-	{ },
-};
-
-static struct ctl_table svcrdma_root_table[] = {
-	{
-		.procname	= "sunrpc",
-		.mode		= 0555,
-		.child		= svcrdma_table
-	},
-	{ },
-};
-
 static void svc_rdma_proc_cleanup(void)
 {
 	if (!svcrdma_table_header)
@@ -263,7 +245,8 @@ static int svc_rdma_proc_init(void)
 	if (rc)
 		goto out_err;
 
-	svcrdma_table_header = register_sysctl_table(svcrdma_root_table);
+	svcrdma_table_header = register_sysctl("sunrpc/svc_rdma",
+					       svcrdma_parm_table);
 	return 0;
 
 out_err:
-- 
2.39.1

Re: [PATCH v3 1/5] sunrpc: simplify two-level sysctl registration for tsvcrdma_parm_table

[Chuck Lever III]

> On Mar 11, 2023, at 6:39 PM, Luis Chamberlain <mcgrof@kernel.org> wrote:
> 
> There is no need to declare two tables to just create directories,
> this can be easily be done with a prefix path with register_sysctl().
> 
> Simplify this registration.
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>

I can take this one, but I'm wondering what "tsvcrdma_parm_table"
is (see the short description).


> ---
> net/sunrpc/xprtrdma/svc_rdma.c | 21 ++-------------------
> 1 file changed, 2 insertions(+), 19 deletions(-)
> 
> diff --git a/net/sunrpc/xprtrdma/svc_rdma.c b/net/sunrpc/xprtrdma/svc_rdma.c
> index 5bc20e9d09cd..f0d5eeed4c88 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma.c
> @@ -212,24 +212,6 @@ static struct ctl_table svcrdma_parm_table[] = {
> 	{ },
> };
> 
> -static struct ctl_table svcrdma_table[] = {
> -	{
> -		.procname	= "svc_rdma",
> -		.mode		= 0555,
> -		.child		= svcrdma_parm_table
> -	},
> -	{ },
> -};
> -
> -static struct ctl_table svcrdma_root_table[] = {
> -	{
> -		.procname	= "sunrpc",
> -		.mode		= 0555,
> -		.child		= svcrdma_table
> -	},
> -	{ },
> -};
> -
> static void svc_rdma_proc_cleanup(void)
> {
> 	if (!svcrdma_table_header)
> @@ -263,7 +245,8 @@ static int svc_rdma_proc_init(void)
> 	if (rc)
> 		goto out_err;
> 
> -	svcrdma_table_header = register_sysctl_table(svcrdma_root_table);
> +	svcrdma_table_header = register_sysctl("sunrpc/svc_rdma",
> +					       svcrdma_parm_table);
> 	return 0;
> 
> out_err:
> -- 
> 2.39.1
> 

--
Chuck Lever

Re: [PATCH v3 1/5] sunrpc: simplify two-level sysctl registration for tsvcrdma_parm_table

[Luis Chamberlain]

On Sun, Mar 12, 2023 at 01:06:26AM +0000, Chuck Lever III wrote:
> 
> 
> > On Mar 11, 2023, at 6:39 PM, Luis Chamberlain <mcgrof@kernel.org> wrote:
> > 
> > There is no need to declare two tables to just create directories,
> > this can be easily be done with a prefix path with register_sysctl().
> > 
> > Simplify this registration.
> > 
> > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> 
> I can take this one, but I'm wondering what "tsvcrdma_parm_table"
> is (see the short description).

Heh sorry                                     tsvcrdma_parm_table was
supposed to be                                 svcrdma_parm_table.

Sorry for the typo.

Can you fix or wuold you like me to resend?

  Luis

Re: [PATCH v3 1/5] sunrpc: simplify two-level sysctl registration for tsvcrdma_parm_table

[Chuck Lever III]

> On Mar 11, 2023, at 8:15 PM, Luis Chamberlain <mcgrof@kernel.org> wrote:
> 
> On Sun, Mar 12, 2023 at 01:06:26AM +0000, Chuck Lever III wrote:
>> 
>> 
>>> On Mar 11, 2023, at 6:39 PM, Luis Chamberlain <mcgrof@kernel.org> wrote:
>>> 
>>> There is no need to declare two tables to just create directories,
>>> this can be easily be done with a prefix path with register_sysctl().
>>> 
>>> Simplify this registration.
>>> 
>>> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
>> 
>> I can take this one, but I'm wondering what "tsvcrdma_parm_table"
>> is (see the short description).
> 
> Heh sorry                                     tsvcrdma_parm_table was
> supposed to be                                 svcrdma_parm_table.
> 
> Sorry for the typo.
> 
> Can you fix or wuold you like me to resend?

I'll fix it up, just wanted confirmation that I wasn't missing
something important!

--
Chuck Lever

[PATCH v3 2/5] sunrpc: simplify one-level sysctl registration for xr_tunables_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>
---
 net/sunrpc/xprtrdma/transport.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/net/sunrpc/xprtrdma/transport.c b/net/sunrpc/xprtrdma/transport.c
index 10bb2b929c6d..29b0562d62e7 100644
--- a/net/sunrpc/xprtrdma/transport.c
+++ b/net/sunrpc/xprtrdma/transport.c
@@ -140,15 +140,6 @@ static struct ctl_table xr_tunables_table[] = {
 	{ },
 };
 
-static struct ctl_table sunrpc_table[] = {
-	{
-		.procname	= "sunrpc",
-		.mode		= 0555,
-		.child		= xr_tunables_table
-	},
-	{ },
-};
-
 #endif
 
 static const struct rpc_xprt_ops xprt_rdma_procs;
@@ -799,7 +790,7 @@ int xprt_rdma_init(void)
 
 #if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
 	if (!sunrpc_table_header)
-		sunrpc_table_header = register_sysctl_table(sunrpc_table);
+		sunrpc_table_header = register_sysctl("sunrpc", xr_tunables_table);
 #endif
 	return 0;
 }
-- 
2.39.1

[PATCH v3 3/5] sunrpc: simplify one-level sysctl registration for xs_tunables_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>
---
 net/sunrpc/xprtsock.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index aaa5b2741b79..46bbd6230650 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -77,7 +77,7 @@ static unsigned int xs_tcp_fin_timeout __read_mostly = XS_TCP_LINGER_TO;
 
 /*
  * We can register our own files under /proc/sys/sunrpc by
- * calling register_sysctl_table() again.  The files in that
+ * calling register_sysctl() again.  The files in that
  * directory become the union of all files registered there.
  *
  * We simply need to make sure that we don't collide with
@@ -157,15 +157,6 @@ static struct ctl_table xs_tunables_table[] = {
 	{ },
 };
 
-static struct ctl_table sunrpc_table[] = {
-	{
-		.procname	= "sunrpc",
-		.mode		= 0555,
-		.child		= xs_tunables_table
-	},
-	{ },
-};
-
 /*
  * Wait duration for a reply from the RPC portmapper.
  */
@@ -3174,7 +3165,7 @@ static struct xprt_class	xs_bc_tcp_transport = {
 int init_socket_xprt(void)
 {
 	if (!sunrpc_table_header)
-		sunrpc_table_header = register_sysctl_table(sunrpc_table);
+		sunrpc_table_header = register_sysctl("sunrpc", xs_tunables_table);
 
 	xprt_register_transport(&xs_local_transport);
 	xprt_register_transport(&xs_udp_transport);
-- 
2.39.1

[PATCH v3 4/5] sunrpc: move sunrpc_table and proc routines above

[Luis Chamberlain]

No need to do a forward declaration for sunrpc_table, just move
the sysctls up as everyone else does it. This will make the next
change easier to read. This change produces no functional changes.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 net/sunrpc/sysctl.c | 35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/net/sunrpc/sysctl.c b/net/sunrpc/sysctl.c
index 3aad6ef18504..afdfcc5403af 100644
--- a/net/sunrpc/sysctl.c
+++ b/net/sunrpc/sysctl.c
@@ -40,25 +40,6 @@ EXPORT_SYMBOL_GPL(nlm_debug);
 
 #if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
 
-static struct ctl_table_header *sunrpc_table_header;
-static struct ctl_table sunrpc_table[];
-
-void
-rpc_register_sysctl(void)
-{
-	if (!sunrpc_table_header)
-		sunrpc_table_header = register_sysctl_table(sunrpc_table);
-}
-
-void
-rpc_unregister_sysctl(void)
-{
-	if (sunrpc_table_header) {
-		unregister_sysctl_table(sunrpc_table_header);
-		sunrpc_table_header = NULL;
-	}
-}
-
 static int proc_do_xprt(struct ctl_table *table, int write,
 			void *buffer, size_t *lenp, loff_t *ppos)
 {
@@ -142,6 +123,7 @@ proc_dodebug(struct ctl_table *table, int write, void *buffer, size_t *lenp,
 	return 0;
 }
 
+static struct ctl_table_header *sunrpc_table_header;
 
 static struct ctl_table debug_table[] = {
 	{
@@ -190,4 +172,19 @@ static struct ctl_table sunrpc_table[] = {
 	{ }
 };
 
+void
+rpc_register_sysctl(void)
+{
+	if (!sunrpc_table_header)
+		sunrpc_table_header = register_sysctl_table(sunrpc_table);
+}
+
+void
+rpc_unregister_sysctl(void)
+{
+	if (sunrpc_table_header) {
+		unregister_sysctl_table(sunrpc_table_header);
+		sunrpc_table_header = NULL;
+	}
+}
 #endif
-- 
2.39.1

[PATCH v3 5/5] sunrpc: simplify one-level sysctl registration for debug_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>
---
 net/sunrpc/sysctl.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/net/sunrpc/sysctl.c b/net/sunrpc/sysctl.c
index afdfcc5403af..93941ab12549 100644
--- a/net/sunrpc/sysctl.c
+++ b/net/sunrpc/sysctl.c
@@ -163,20 +163,11 @@ static struct ctl_table debug_table[] = {
 	{ }
 };
 
-static struct ctl_table sunrpc_table[] = {
-	{
-		.procname	= "sunrpc",
-		.mode		= 0555,
-		.child		= debug_table
-	},
-	{ }
-};
-
 void
 rpc_register_sysctl(void)
 {
 	if (!sunrpc_table_header)
-		sunrpc_table_header = register_sysctl_table(sunrpc_table);
+		sunrpc_table_header = register_sysctl("sunrpc", debug_table);
 }
 
 void
-- 
2.39.1

Re: [PATCH v3 0/5] sunrpc: simplfy sysctl registrations

[Jeff Layton]

On Sat, 2023-03-11 at 15:39 -0800, Luis Chamberlain wrote:
> This is my v3 series to simplify sysctl registration for sunrpc. The
> first series was posted just yesterday [0] but 0-day found an issue with
> CONFIG_SUNRPC_DEBUG. After this fix I poasted a fix for v2 [1] but alas
> 0-day then found an issue when CONFIG_SUNRPC_DEBUG is disabled. This
> fixes both cases... hopefully that's it.
>                                                                                                                                                                                               
> Changes on v3:
> 
>    o Fix compilation when CONFIG_SUNRPC_DEBUG is disabled.. forgot to
>      keep all the sysctl stuff under the #ifdef.
> 
> Changes on v2:
> 
>    o Fix compilation when CONFIG_SUNRPC_DEBUG is enabled, I forgot to move the
>      proc routines above, and so the 4th patch now does that too.
>                                                                                                                                                                                               
> Feel free to take these patches or let me know and I'm happy to also
> take these in through sysctl-next. Typically I use sysctl-next for
> core sysctl changes or for kernel/sysctl.c cleanup to avoid conflicts.
> All these syctls however are well contained to sunrpc so they can also
> go in separately. Let me know how you'd like to go about these patches.
>                                                                                                                                                                                               
> [0] https://lkml.kernel.org/r/20230310225236.3939443-1-mcgrof@kernel.org
> 
> Luis Chamberlain (5):
>   sunrpc: simplify two-level sysctl registration for tsvcrdma_parm_table
>   sunrpc: simplify one-level sysctl registration for xr_tunables_table
>   sunrpc: simplify one-level sysctl registration for xs_tunables_table
>   sunrpc: move sunrpc_table and proc routines above
>   sunrpc: simplify one-level sysctl registration for debug_table
> 
>  net/sunrpc/sysctl.c             | 42 ++++++++++++---------------------
>  net/sunrpc/xprtrdma/svc_rdma.c  | 21 ++---------------
>  net/sunrpc/xprtrdma/transport.c | 11 +--------
>  net/sunrpc/xprtsock.c           | 13 ++--------
>  4 files changed, 20 insertions(+), 67 deletions(-)
> 

Nice little cleanup.

Reviewed-by: Jeff Layton <jlayton@kernel.org>