diff mbox series

[v2,5/8] printk: move printk sysctl to printk/sysctl.c

Message ID 20211124231435.1445213-6-mcgrof@kernel.org (mailing list archive)
State Not Applicable
Headers show
Series sysctl: 3rd set of kernel/sysctl cleanups | expand

Commit Message

Luis Chamberlain Nov. 24, 2021, 11:14 p.m. UTC 
From: Xiaoming Ni <nixiaoming@huawei.com>

The kernel/sysctl.c is a kitchen sink where everyone leaves
their dirty dishes, this makes it very difficult to maintain.

To help with this maintenance let's start by moving sysctls to
places where they actually belong. The proc sysctl maintainers
do not want to know what sysctl knobs you wish to add for your own
piece of code, we just care about the core logic.

So move printk sysctl from kernel/sysctl.c to kernel/printk/sysctl.c.
Use register_sysctl() to register the sysctl interface.

Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
[mcgrof: fixed compile issues when PRINTK is not set, commit log update]
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 kernel/printk/Makefile   |  5 ++-
 kernel/printk/internal.h |  6 +++
 kernel/printk/printk.c   |  1 +
 kernel/printk/sysctl.c   | 85 ++++++++++++++++++++++++++++++++++++++++
 kernel/sysctl.c          | 68 --------------------------------
 5 files changed, 96 insertions(+), 69 deletions(-)
 create mode 100644 kernel/printk/sysctl.c

Comments

Petr Mladek Nov. 26, 2021, 12:51 p.m. UTC | #1 
On Wed 2021-11-24 15:14:32, Luis Chamberlain wrote:
> From: Xiaoming Ni <nixiaoming@huawei.com>
> 
> The kernel/sysctl.c is a kitchen sink where everyone leaves
> their dirty dishes, this makes it very difficult to maintain.
> 
> To help with this maintenance let's start by moving sysctls to
> places where they actually belong. The proc sysctl maintainers
> do not want to know what sysctl knobs you wish to add for your own
> piece of code, we just care about the core logic.
> 
> So move printk sysctl from kernel/sysctl.c to kernel/printk/sysctl.c.
> Use register_sysctl() to register the sysctl interface.
> 
> diff --git a/kernel/printk/Makefile b/kernel/printk/Makefile
> index d118739874c0..f5b388e810b9 100644
> --- a/kernel/printk/Makefile
> +++ b/kernel/printk/Makefile
> @@ -2,5 +2,8 @@
>  obj-y	= printk.o
>  obj-$(CONFIG_PRINTK)	+= printk_safe.o
>  obj-$(CONFIG_A11Y_BRAILLE_CONSOLE)	+= braille.o
> -obj-$(CONFIG_PRINTK)	+= printk_ringbuffer.o
>  obj-$(CONFIG_PRINTK_INDEX)	+= index.o
> +
> +obj-$(CONFIG_PRINTK)                 += printk_support.o
> +printk_support-y	             := printk_ringbuffer.o
> +printk_support-$(CONFIG_SYSCTL)	     += sysctl.o

I have never seen this trick. It looks like a dirty hack ;-)
Anyway, I do not see it described in the documentation. I wonder
if it works only by chance.

IMHO, a cleaner solution would be to add the following
into init/Kconfig:

config BUILD_PRINTK_SYSCTL
	bool
	default (PRINTK && SYSCTL)

and then use:

obj-$(CONFIG_BUILD_PRINTK_SYSCTL)    += sysctl.o


> diff --git a/kernel/printk/sysctl.c b/kernel/printk/sysctl.c
> new file mode 100644
> index 000000000000..653ae04aab7f
> --- /dev/null
> +++ b/kernel/printk/sysctl.c
> @@ -0,0 +1,85 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * sysctl.c: General linux system control interface
> + */
> +
> +#include <linux/sysctl.h>
> +#include <linux/printk.h>
> +#include <linux/capability.h>
> +#include <linux/ratelimit.h>
> +#include "internal.h"
> +
> +static const int ten_thousand = 10000;

The patch should also remove the variable in kernel/sysctl.c.

Otherwise, it looks like a really nice clean up.

Best Regards,
Petr
Luis Chamberlain Nov. 29, 2021, 8:48 p.m. UTC | #2 
On Fri, Nov 26, 2021 at 01:51:38PM +0100, Petr Mladek wrote:
> On Wed 2021-11-24 15:14:32, Luis Chamberlain wrote:
> > From: Xiaoming Ni <nixiaoming@huawei.com>
> > 
> > The kernel/sysctl.c is a kitchen sink where everyone leaves
> > their dirty dishes, this makes it very difficult to maintain.
> > 
> > To help with this maintenance let's start by moving sysctls to
> > places where they actually belong. The proc sysctl maintainers
> > do not want to know what sysctl knobs you wish to add for your own
> > piece of code, we just care about the core logic.
> > 
> > So move printk sysctl from kernel/sysctl.c to kernel/printk/sysctl.c.
> > Use register_sysctl() to register the sysctl interface.
> > 
> > diff --git a/kernel/printk/Makefile b/kernel/printk/Makefile
> > index d118739874c0..f5b388e810b9 100644
> > --- a/kernel/printk/Makefile
> > +++ b/kernel/printk/Makefile
> > @@ -2,5 +2,8 @@
> >  obj-y	= printk.o
> >  obj-$(CONFIG_PRINTK)	+= printk_safe.o
> >  obj-$(CONFIG_A11Y_BRAILLE_CONSOLE)	+= braille.o
> > -obj-$(CONFIG_PRINTK)	+= printk_ringbuffer.o
> >  obj-$(CONFIG_PRINTK_INDEX)	+= index.o
> > +
> > +obj-$(CONFIG_PRINTK)                 += printk_support.o
> > +printk_support-y	             := printk_ringbuffer.o
> > +printk_support-$(CONFIG_SYSCTL)	     += sysctl.o
> 
> I have never seen this trick. It looks like a dirty hack ;-)

It has been used in mac80211 for over a decade now :) See
net/mac80211/Makefile

> Anyway, I do not see it described in the documentation. I wonder
> if it works only by chance.
> 
> IMHO, a cleaner solution would be to add the following
> into init/Kconfig:
> 
> config BUILD_PRINTK_SYSCTL
> 	bool
> 	default (PRINTK && SYSCTL)
> 
> and then use:
> 
> obj-$(CONFIG_BUILD_PRINTK_SYSCTL)    += sysctl.o

I suppose it is a matter of taste, either way works with me,
but I think less kconfig logic is better here.

> > diff --git a/kernel/printk/sysctl.c b/kernel/printk/sysctl.c
> > new file mode 100644
> > index 000000000000..653ae04aab7f
> > --- /dev/null
> > +++ b/kernel/printk/sysctl.c
> > @@ -0,0 +1,85 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * sysctl.c: General linux system control interface
> > + */
> > +
> > +#include <linux/sysctl.h>
> > +#include <linux/printk.h>
> > +#include <linux/capability.h>
> > +#include <linux/ratelimit.h>
> > +#include "internal.h"
> > +
> > +static const int ten_thousand = 10000;
> 
> The patch should also remove the variable in kernel/sysctl.c.
> 
> Otherwise, it looks like a really nice clean up.

Ah yes that variable is now unused there. Thanks

  Luis
diff mbox series

Patch

diff --git a/kernel/printk/Makefile b/kernel/printk/Makefile
index d118739874c0..f5b388e810b9 100644
--- a/kernel/printk/Makefile
+++ b/kernel/printk/Makefile
@@ -2,5 +2,8 @@ 
 obj-y	= printk.o
 obj-$(CONFIG_PRINTK)	+= printk_safe.o
 obj-$(CONFIG_A11Y_BRAILLE_CONSOLE)	+= braille.o
-obj-$(CONFIG_PRINTK)	+= printk_ringbuffer.o
 obj-$(CONFIG_PRINTK_INDEX)	+= index.o
+
+obj-$(CONFIG_PRINTK)                 += printk_support.o
+printk_support-y	             := printk_ringbuffer.o
+printk_support-$(CONFIG_SYSCTL)	     += sysctl.o
diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index 9f3ed2fdb721..6b1c4b399845 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -4,6 +4,12 @@ 
  */
 #include <linux/percpu.h>
 
+#if defined(CONFIG_PRINTK) && defined(CONFIG_SYSCTL)
+void __init printk_sysctl_init(void);
+#else
+#define printk_sysctl_init() do { } while (0)
+#endif
+
 #ifdef CONFIG_PRINTK
 
 /* Flags for a single printk record. */
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index cbc35d586afb..dbb44086ba65 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3202,6 +3202,7 @@  static int __init printk_late_init(void)
 	ret = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN, "printk:online",
 					console_cpu_notify, NULL);
 	WARN_ON(ret < 0);
+	printk_sysctl_init();
 	return 0;
 }
 late_initcall(printk_late_init);
diff --git a/kernel/printk/sysctl.c b/kernel/printk/sysctl.c
new file mode 100644
index 000000000000..653ae04aab7f
--- /dev/null
+++ b/kernel/printk/sysctl.c
@@ -0,0 +1,85 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * sysctl.c: General linux system control interface
+ */
+
+#include <linux/sysctl.h>
+#include <linux/printk.h>
+#include <linux/capability.h>
+#include <linux/ratelimit.h>
+#include "internal.h"
+
+static const int ten_thousand = 10000;
+
+static int proc_dointvec_minmax_sysadmin(struct ctl_table *table, int write,
+				void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	if (write && !capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	return proc_dointvec_minmax(table, write, buffer, lenp, ppos);
+}
+
+static struct ctl_table printk_sysctls[] = {
+	{
+		.procname	= "printk",
+		.data		= &console_loglevel,
+		.maxlen		= 4*sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec,
+	},
+	{
+		.procname	= "printk_ratelimit",
+		.data		= &printk_ratelimit_state.interval,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_jiffies,
+	},
+	{
+		.procname	= "printk_ratelimit_burst",
+		.data		= &printk_ratelimit_state.burst,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec,
+	},
+	{
+		.procname	= "printk_delay",
+		.data		= &printk_delay_msec,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= (void *)&ten_thousand,
+	},
+	{
+		.procname	= "printk_devkmsg",
+		.data		= devkmsg_log_str,
+		.maxlen		= DEVKMSG_STR_MAX_SIZE,
+		.mode		= 0644,
+		.proc_handler	= devkmsg_sysctl_set_loglvl,
+	},
+	{
+		.procname	= "dmesg_restrict",
+		.data		= &dmesg_restrict,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax_sysadmin,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_ONE,
+	},
+	{
+		.procname	= "kptr_restrict",
+		.data		= &kptr_restrict,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax_sysadmin,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_TWO,
+	},
+	{}
+};
+
+void __init printk_sysctl_init(void)
+{
+	register_sysctl_init("kernel", printk_sysctls);
+}
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 7745c9b72bda..02ef27804601 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -907,17 +907,6 @@  static int proc_taint(struct ctl_table *table, int write,
 	return err;
 }
 
-#ifdef CONFIG_PRINTK
-static int proc_dointvec_minmax_sysadmin(struct ctl_table *table, int write,
-				void *buffer, size_t *lenp, loff_t *ppos)
-{
-	if (write && !capable(CAP_SYS_ADMIN))
-		return -EPERM;
-
-	return proc_dointvec_minmax(table, write, buffer, lenp, ppos);
-}
-#endif
-
 /**
  * struct do_proc_dointvec_minmax_conv_param - proc_dointvec_minmax() range checking structure
  * @min: pointer to minimum allowable value
@@ -2209,63 +2198,6 @@  static struct ctl_table kern_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_doulongvec_minmax,
 	},
-#if defined CONFIG_PRINTK
-	{
-		.procname	= "printk",
-		.data		= &console_loglevel,
-		.maxlen		= 4*sizeof(int),
-		.mode		= 0644,
-		.proc_handler	= proc_dointvec,
-	},
-	{
-		.procname	= "printk_ratelimit",
-		.data		= &printk_ratelimit_state.interval,
-		.maxlen		= sizeof(int),
-		.mode		= 0644,
-		.proc_handler	= proc_dointvec_jiffies,
-	},
-	{
-		.procname	= "printk_ratelimit_burst",
-		.data		= &printk_ratelimit_state.burst,
-		.maxlen		= sizeof(int),
-		.mode		= 0644,
-		.proc_handler	= proc_dointvec,
-	},
-	{
-		.procname	= "printk_delay",
-		.data		= &printk_delay_msec,
-		.maxlen		= sizeof(int),
-		.mode		= 0644,
-		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= SYSCTL_ZERO,
-		.extra2		= (void *)&ten_thousand,
-	},
-	{
-		.procname	= "printk_devkmsg",
-		.data		= devkmsg_log_str,
-		.maxlen		= DEVKMSG_STR_MAX_SIZE,
-		.mode		= 0644,
-		.proc_handler	= devkmsg_sysctl_set_loglvl,
-	},
-	{
-		.procname	= "dmesg_restrict",
-		.data		= &dmesg_restrict,
-		.maxlen		= sizeof(int),
-		.mode		= 0644,
-		.proc_handler	= proc_dointvec_minmax_sysadmin,
-		.extra1		= SYSCTL_ZERO,
-		.extra2		= SYSCTL_ONE,
-	},
-	{
-		.procname	= "kptr_restrict",
-		.data		= &kptr_restrict,
-		.maxlen		= sizeof(int),
-		.mode		= 0644,
-		.proc_handler	= proc_dointvec_minmax_sysadmin,
-		.extra1		= SYSCTL_ZERO,
-		.extra2		= SYSCTL_TWO,
-	},
-#endif
 	{
 		.procname	= "ngroups_max",
 		.data		= (void *)&ngroups_max,