From: Luis Chamberlain <mcgrof@kernel.org>
To: Xiaoming Ni <nixiaoming@huawei.com>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>,
Al Viro <viro@ZenIV.linux.org.uk>,
Kees Cook <keescook@chromium.org>,
Stephen Rothwell <sfr@canb.auug.org.au>,
Helge Deller <deller@gmx.de>,
Parisc List <linux-parisc@vger.kernel.org>,
yzaikin@google.com, linux-fsdevel@vger.kernel.org,
Linux Next Mailing List <linux-next@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Christoph Hellwig <hch@lst.de>,
gregkh@linuxfoundation.org
Subject: Re: linux-next: manual merge of the vfs tree with the parisc-hd tree
Date: Wed, 13 May 2020 12:50:57 +0000 [thread overview]
Message-ID: <20200513125057.GM11244@42.do-not-panic.com> (raw)
In-Reply-To: <3ccd08a5-cac6-3ca1-ed33-3cb62c982443@huawei.com>
On Wed, May 13, 2020 at 12:04:02PM +0800, Xiaoming Ni wrote:
> On 2020/5/13 6:03, Luis Chamberlain wrote:
> > On Tue, May 12, 2020 at 12:40:55PM -0500, Eric W. Biederman wrote:
> > > Luis Chamberlain <mcgrof@kernel.org> writes:
> > >
> > > > On Tue, May 12, 2020 at 06:52:35AM -0500, Eric W. Biederman wrote:
> > > > > Luis Chamberlain <mcgrof@kernel.org> writes:
> > > > >
> > > > > > +static struct ctl_table fs_base_table[] = {
> > > > > > + {
> > > > > > + .procname = "fs",
> > > > > > + .mode = 0555,
> > > > > > + .child = fs_table,
> > > > > > + },
> > > > > > + { }
> > > > > > +};
> > > > > ^^^^^^^^^^^^^^^^^^^^^^^^ You don't need this at all.
> > > > > > > +static int __init fs_procsys_init(void)
> > > > > > +{
> > > > > > + struct ctl_table_header *hdr;
> > > > > > +
> > > > > > + hdr = register_sysctl_table(fs_base_table);
> > > > > ^^^^^^^^^^^^^^^^^^^^^ Please use register_sysctl instead.
> > > > > AKA
> > > > > hdr = register_sysctl("fs", fs_table);
> > > >
> > > > Ah, much cleaner thanks!
> > >
> > > It is my hope you we can get rid of register_sysctl_table one of these
> > > days. It was the original interface but today it is just a
> > > compatibility wrapper.
> > >
> > > I unfortunately ran out of steam last time before I finished converting
> > > everything over.
> >
> > Let's give it one more go. I'll start with the fs stuff.
> >
> > Luis
> >
> > .
> >
>
> If we register each feature in its own feature code file using register() to
> register the sysctl interface. To avoid merge conflicts when different
> features modify sysctl.c at the same time.
> that is, try to Avoid mixing code with multiple features in the same code
> file.
>
> For example, the multiple file interfaces defined in sysctl.c by the
> hung_task feature can be moved to hung_task.c.
>
> Perhaps later, without centralized sysctl.c ?
> Is this better?
>
> Thanks
> Xiaoming Ni
>
> ---
> include/linux/sched/sysctl.h | 8 +----
> kernel/hung_task.c | 78
> +++++++++++++++++++++++++++++++++++++++++++-
> kernel/sysctl.c | 50 ----------------------------
> 3 files changed, 78 insertions(+), 58 deletions(-)
>
> diff --git a/include/linux/sched/sysctl.h b/include/linux/sched/sysctl.h
> index d4f6215..bb4e0d3 100644
> --- a/include/linux/sched/sysctl.h
> +++ b/include/linux/sched/sysctl.h
> @@ -7,14 +7,8 @@
> struct ctl_table;
>
> #ifdef CONFIG_DETECT_HUNG_TASK
> -extern int sysctl_hung_task_check_count;
> -extern unsigned int sysctl_hung_task_panic;
> +/* used for block/ */
> extern unsigned long sysctl_hung_task_timeout_secs;
> -extern unsigned long sysctl_hung_task_check_interval_secs;
> -extern int sysctl_hung_task_warnings;
> -extern int proc_dohung_task_timeout_secs(struct ctl_table *table, int
> write,
> - void __user *buffer,
> - size_t *lenp, loff_t *ppos);
> #else
> /* Avoid need for ifdefs elsewhere in the code */
> enum { sysctl_hung_task_timeout_secs = 0 };
> diff --git a/kernel/hung_task.c b/kernel/hung_task.c
> index 14a625c..53589f2 100644
> --- a/kernel/hung_task.c
> +++ b/kernel/hung_task.c
> @@ -20,10 +20,10 @@
> #include <linux/utsname.h>
> #include <linux/sched/signal.h>
> #include <linux/sched/debug.h>
> +#include <linux/kmemleak.h>
> #include <linux/sched/sysctl.h>
>
> #include <trace/events/sched.h>
> -
> /*
> * The number of tasks checked:
> */
> @@ -296,8 +296,84 @@ static int watchdog(void *dummy)
> return 0;
> }
>
> +/*
> + * This is needed for proc_doulongvec_minmax of
> sysctl_hung_task_timeout_secs
> + * and hung_task_check_interval_secs
> + */
> +static unsigned long hung_task_timeout_max = (LONG_MAX / HZ);
This is not generic so it can stay in this file.
> +static int __maybe_unused neg_one = -1;
This is generic so we can share it, I suggest we just rename this
for now to sysctl_neg_one, export it to a symbol namespace,
EXPORT_SYMBOL_NS_GPL(sysctl_neg_one, SYSCTL) and then import it with
MODULE_IMPORT_NS(SYSCTL)
> +static struct ctl_table hung_task_sysctls[] = {
We want to wrap this around with CONFIG_SYSCTL, so a cleaner solution
is something like this:
diff --git a/kernel/Makefile b/kernel/Makefile
index a42ac3a58994..689718351754 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -88,7 +88,9 @@ obj-$(CONFIG_KCOV) += kcov.o
obj-$(CONFIG_KPROBES) += kprobes.o
obj-$(CONFIG_FAIL_FUNCTION) += fail_function.o
obj-$(CONFIG_KGDB) += debug/
-obj-$(CONFIG_DETECT_HUNG_TASK) += hung_task.o
+obj-$(CONFIG_DETECT_HUNG_TASK) += hung_tasks.o
+hung_tasks-y := hung_task.o
+hung_tasks-$(CONFIG_SYSCTL) += hung_task_sysctl.o
obj-$(CONFIG_LOCKUP_DETECTOR) += watchdog.o
obj-$(CONFIG_HARDLOCKUP_DETECTOR_PERF) += watchdog_hld.o
obj-$(CONFIG_SECCOMP) += seccomp.o
> +/* get /proc/sys/kernel root */
> +static struct ctl_table sysctls_root[] = {
> + {
> + .procname = "kernel",
> + .mode = 0555,
> + .child = hung_task_sysctls,
> + },
> + {}
> +};
> +
And as per Eric, this is not needed, we can simplify this more, as noted
below.
> +static int __init hung_task_sysctl_init(void)
> +{
> + struct ctl_table_header *srt = register_sysctl_table(sysctls_root);
You want instead something like::
struct ctl_table_header *srt;
srt = register_sysctl("kernel", hung_task_sysctls);
> +
> + if (!srt)
> + return -ENOMEM;
> + kmemleak_not_leak(srt);
> + return 0;
> +}
> +
> static int __init hung_task_init(void)
> {
> + int ret = hung_task_sysctl_init();
> +
> + if (ret != 0)
> + return ret;
> +
And just #ifdef this around CONFIG_SYSCTL.
Luis
next prev parent reply other threads:[~2020-05-13 12:51 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-11 1:11 linux-next: manual merge of the vfs tree with the parisc-hd tree Stephen Rothwell
2020-05-11 1:55 ` Xiaoming Ni
2020-05-12 0:33 ` Luis Chamberlain
2020-05-12 5:22 ` Kees Cook
2020-05-12 5:44 ` Luis Chamberlain
2020-05-12 11:52 ` Eric W. Biederman
2020-05-12 17:24 ` Luis Chamberlain
2020-05-12 17:40 ` Eric W. Biederman
2020-05-12 22:03 ` Luis Chamberlain
2020-05-13 4:04 ` Xiaoming Ni
2020-05-13 12:50 ` Luis Chamberlain [this message]
2020-05-14 6:05 ` Xiaoming Ni
2020-05-14 16:17 ` Xiaoming Ni
2020-05-15 16:08 ` Luis Chamberlain
2020-05-13 13:42 ` Eric W. Biederman
2020-05-13 14:14 ` Luis Chamberlain
2020-05-13 14:44 ` Eric W. Biederman
2020-05-13 15:02 ` Luis Chamberlain
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200513125057.GM11244@42.do-not-panic.com \
--to=mcgrof@kernel.org \
--cc=deller@gmx.de \
--cc=ebiederm@xmission.com \
--cc=gregkh@linuxfoundation.org \
--cc=hch@lst.de \
--cc=keescook@chromium.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-next@vger.kernel.org \
--cc=linux-parisc@vger.kernel.org \
--cc=nixiaoming@huawei.com \
--cc=sfr@canb.auug.org.au \
--cc=viro@ZenIV.linux.org.uk \
--cc=yzaikin@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).