Re: linux-next: manual merge of the vfs tree with the parisc-hd tree
On Fri, May 15, 2020 at 12:17:52AM +0800, Xiaoming Ni wrote:
> On 2020/5/14 14:05, Xiaoming Ni wrote:
> > On 2020/5/13 20:50, Luis Chamberlain wrote:
> > > 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 writes:
> > > > > >
> > > > > > > On Tue, May 12, 2020 at 06:52:35AM -0500, Eric W. Biederman wrote:
> > > > > > > > Luis Chamberlain 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
> > > > #include
> > > > #include
> > > > +#include
> > > > #include
> > > >
> > > > #include
> > > > -
> > > > /*
> > > > * 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)
>
> When I made the patch, I found that only sysctl_writes_strict and
> hung_task_warnings use the neg_one
Re: linux-next: manual merge of the vfs tree with the parisc-hd tree
On 2020/5/14 14:05, Xiaoming Ni wrote:
On 2020/5/13 20:50, Luis Chamberlain wrote:
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 writes:
On Tue, May 12, 2020 at 06:52:35AM -0500, Eric W. Biederman wrote:
Luis Chamberlain 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
#include
#include
+#include
#include
#include
-
/*
* 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)
When I made the patch, I found that only sysctl_writes_strict and
hung_task_warnings use the neg_one variable, so is it necessary to merge
and generate the SYSCTL_NEG_ONE variable?
In addition, the SYSCTL symbol namespace has not been created yet. Do I
just need to add a new member -1 to the sysctl_vals array?
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index b6f5d45..acae1fa 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -23,7 +23,7 @@
static const struct inode_operations proc_sys_dir_operations;
/* shared constants to be used in various sysctls */
-const int sysctl_vals[] = { 0, 1, INT_MAX };
+const int sysctl_vals[] = { 0, 1, INT_MAX, -1 };
EXPORT_SYMBOL(sysctl_vals);
/* Support for permanently empty directories */
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 02fa844..6d741d6 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -41,6 +41,7 @@
#define SYSCTL_ZERO((void *)_vals[0])
#define SYSCTL_ONE ((void *)_vals[1])
#define SYSCTL_INT_MAX ((void *)_vals[2])
+#define SYSCTL_NEG_ONE ((void *)_vals[3])
extern const int sysctl_vals[];
Thanks
Xiaoming Ni
+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
Re: linux-next: manual merge of the vfs tree with the parisc-hd tree
On 2020/5/13 20:50, Luis Chamberlain wrote:
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 writes:
On Tue, May 12, 2020 at 06:52:35AM -0500, Eric W. Biederman wrote:
Luis Chamberlain 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
#include
#include
+#include
#include
#include
-
/*
* 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);
Re: linux-next: manual merge of the vfs tree with the parisc-hd tree
On Wed, May 13, 2020 at 09:44:40AM -0500, Eric W. Biederman wrote:
> Luis Chamberlain writes:
>
> > On Wed, May 13, 2020 at 08:42:30AM -0500, Eric W. Biederman wrote:
> >> Luis Chamberlain writes:
> >>
> >> > On Tue, May 12, 2020 at 12:40:55PM -0500, Eric W. Biederman wrote:
> >> >> Luis Chamberlain writes:
> >> >>
> >> >> > On Tue, May 12, 2020 at 06:52:35AM -0500, Eric W. Biederman wrote:
> >> >> >> Luis Chamberlain 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.
> >>
> >> Just to be clear moving the tables out of kernel/sysctl.c is a related
> >> but slightly different problem.
> >
> > Sure, but also before we go on this crusade, how about we add a few
> > helpers:
> >
> > register_sysctl_kernel()
> > register_sysctl_vm()
> > register_sysctl_fs()
> > register_sysctl_debug()
> > register_sysctl_dev()
>
> Hmm.
>
> register_sysctl("kernel")
>
> > That should make it easier to look for these, and shorter. We *know*
> > this is a common path, given the size of the existing table.
>
> I don't really care but one character shorter doesn't look like it
> really helps. Not really for grepping and not maintenance as we get a
> bunch of trivial one line implementations.
Alright, let's skip the helpers for now.
Luis
Re: linux-next: manual merge of the vfs tree with the parisc-hd tree
Luis Chamberlain writes:
> On Wed, May 13, 2020 at 08:42:30AM -0500, Eric W. Biederman wrote:
>> Luis Chamberlain writes:
>>
>> > On Tue, May 12, 2020 at 12:40:55PM -0500, Eric W. Biederman wrote:
>> >> Luis Chamberlain writes:
>> >>
>> >> > On Tue, May 12, 2020 at 06:52:35AM -0500, Eric W. Biederman wrote:
>> >> >> Luis Chamberlain 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.
>>
>> Just to be clear moving the tables out of kernel/sysctl.c is a related
>> but slightly different problem.
>
> Sure, but also before we go on this crusade, how about we add a few
> helpers:
>
> register_sysctl_kernel()
> register_sysctl_vm()
> register_sysctl_fs()
> register_sysctl_debug()
> register_sysctl_dev()
Hmm.
register_sysctl("kernel")
> That should make it easier to look for these, and shorter. We *know*
> this is a common path, given the size of the existing table.
I don't really care but one character shorter doesn't look like it
really helps. Not really for grepping and not maintenance as we get a
bunch of trivial one line implementations.
Eric
Re: linux-next: manual merge of the vfs tree with the parisc-hd tree
On Wed, May 13, 2020 at 08:42:30AM -0500, Eric W. Biederman wrote:
> Luis Chamberlain writes:
>
> > On Tue, May 12, 2020 at 12:40:55PM -0500, Eric W. Biederman wrote:
> >> Luis Chamberlain writes:
> >>
> >> > On Tue, May 12, 2020 at 06:52:35AM -0500, Eric W. Biederman wrote:
> >> >> Luis Chamberlain 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.
>
> Just to be clear moving the tables out of kernel/sysctl.c is a related
> but slightly different problem.
Sure, but also before we go on this crusade, how about we add a few
helpers:
register_sysctl_kernel()
register_sysctl_vm()
register_sysctl_fs()
register_sysctl_debug()
register_sysctl_dev()
That should make it easier to look for these, and shorter. We *know*
this is a common path, given the size of the existing table.
> Today it looks like there are 35 calls of register_sysctl_table
> and 9 calls of register_sysctl_paths.
>
> Among them is lib/sysctl_test.c and check-sysctl-docs.
>
> Meanwhile I can only find 5 calls to register_sysctl in the tree
> so it looks like I didn't get very far converting things over.
While we're on the spring cleaning topic, I've tried to put what I can
think of for TODO items here, anything else? Feel free to edit, its a
wiki after all.
https://kernelnewbies.org/KernelProjects/proc
Feel free to add wishlist items.
Luis
Re: linux-next: manual merge of the vfs tree with the parisc-hd tree
Luis Chamberlain writes:
> On Tue, May 12, 2020 at 12:40:55PM -0500, Eric W. Biederman wrote:
>> Luis Chamberlain writes:
>>
>> > On Tue, May 12, 2020 at 06:52:35AM -0500, Eric W. Biederman wrote:
>> >> Luis Chamberlain 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.
Just to be clear moving the tables out of kernel/sysctl.c is a related
but slightly different problem.
Today it looks like there are 35 calls of register_sysctl_table
and 9 calls of register_sysctl_paths.
Among them is lib/sysctl_test.c and check-sysctl-docs.
Meanwhile I can only find 5 calls to register_sysctl in the tree
so it looks like I didn't get very far converting things over.
Eric
Re: linux-next: manual merge of the vfs tree with the parisc-hd tree
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 writes:
> > >
> > > > On Tue, May 12, 2020 at 06:52:35AM -0500, Eric W. Biederman wrote:
> > > > > Luis Chamberlain 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 intsysctl_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
> #include
> #include
> +#include
> #include
>
> #include
> -
> /*
> * 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,
> + },
> + {}
>
Re: linux-next: manual merge of the vfs tree with the parisc-hd tree
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 writes:
On Tue, May 12, 2020 at 06:52:35AM -0500, Eric W. Biederman wrote:
Luis Chamberlain 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
#include
#include
+#include
#include
#include
-
/*
* 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);
+static int __maybe_unused neg_one = -1;
+static struct ctl_table hung_task_sysctls[] = {
+ {
+ .procname = "hung_task_panic",
+ .data = _hung_task_panic,
+ .maxlen = sizeof(int),
+ .mode = 0644,
+ .proc_handler = proc_dointvec_minmax,
+ .extra1 = SYSCTL_ZERO,
+ .extra2 = SYSCTL_ONE,
+ },
+ {
+ .procname = "hung_task_check_count",
+ .data = _hung_task_check_count,
+ .maxlen = sizeof(int),
+ .mode = 0644,
+ .proc_handler = proc_dointvec_minmax,
+ .extra1 = SYSCTL_ZERO,
+ },
+ {
+ .procname = "hung_task_timeout_secs",
+ .data = _hung_task_timeout_secs,
+ .maxlen = sizeof(unsigned long),
+ .mode = 0644,
+ .proc_handler = proc_dohung_task_timeout_secs,
+ .extra2 = _task_timeout_max,
+ },
+ {
+ .procname = "hung_task_check_interval_secs",
+ .data = _hung_task_check_interval_secs,
+ .maxlen = sizeof(unsigned long),
+ .mode = 0644,
+ .proc_handler = proc_dohung_task_timeout_secs,
+ .extra2 = _task_timeout_max,
+ },
+ {
+ .procname = "hung_task_warnings",
+ .data = _hung_task_warnings,
+ .maxlen = sizeof(int),
+ .mode = 0644,
+ .proc_handler = proc_dointvec_minmax,
+ .extra1
Re: linux-next: manual merge of the vfs tree with the parisc-hd tree
On Tue, May 12, 2020 at 12:40:55PM -0500, Eric W. Biederman wrote:
> Luis Chamberlain writes:
>
> > On Tue, May 12, 2020 at 06:52:35AM -0500, Eric W. Biederman wrote:
> >> Luis Chamberlain 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
Re: linux-next: manual merge of the vfs tree with the parisc-hd tree
Luis Chamberlain writes:
> On Tue, May 12, 2020 at 06:52:35AM -0500, Eric W. Biederman wrote:
>> Luis Chamberlain 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.
Eric
Re: linux-next: manual merge of the vfs tree with the parisc-hd tree
On Tue, May 12, 2020 at 06:52:35AM -0500, Eric W. Biederman wrote:
> Luis Chamberlain writes:
>
> > On Mon, May 11, 2020 at 09:55:16AM +0800, Xiaoming Ni wrote:
> >> On 2020/5/11 9:11, Stephen Rothwell wrote:
> >> > Hi all,
> >> >
> >> > Today's linux-next merge of the vfs tree got a conflict in:
> >> >
> >> >kernel/sysctl.c
> >> >
> >> > between commit:
> >> >
> >> >b6522fa409cf ("parisc: add sysctl file interface
> >> > panic_on_stackoverflow")
> >> >
> >> > from the parisc-hd tree and commit:
> >> >
> >> >f461d2dcd511 ("sysctl: avoid forward declarations")
> >> >
> >> > from the vfs tree.
> >> >
> >> > I fixed it up (see below) and can carry the fix as necessary. This
> >> > is now fixed as far as linux-next is concerned, but any non trivial
> >> > conflicts should be mentioned to your upstream maintainer when your tree
> >> > is submitted for merging. You may also want to consider cooperating
> >> > with the maintainer of the conflicting tree to minimise any particularly
> >> > complex conflicts.
> >> >
> >>
> >>
> >> Kernel/sysctl.c contains more than 190 interface files, and there are a
> >> large number of config macro controls. When modifying the sysctl interface
> >> directly in kernel/sysctl.c , conflicts are very easy to occur.
> >>
> >> At the same time, the register_sysctl_table() provided by the system can
> >> easily add the sysctl interface, and there is no conflict of
> >> kernel/sysctl.c
> >> .
> >>
> >> Should we add instructions in the patch guide (coding-style.rst
> >> submitting-patches.rst):
> >> Preferentially use register_sysctl_table() to add a new sysctl interface,
> >> centralize feature codes, and avoid directly modifying kernel/sysctl.c ?
> >
> > Yes, however I don't think folks know how to do this well. So I think we
> > just have to do at least start ourselves, and then reflect some of this
> > in the docs. The reason that this can be not easy is that we need to
> > ensure that at an init level we haven't busted dependencies on setting
> > this. We also just don't have docs on how to do this well.
> >
> >> In addition, is it necessary to transfer the architecture-related sysctl
> >> interface to arch/xxx/kernel/sysctl.c ?
>
>
> >
> > Well here's an initial attempt to start with fs stuff in a very
> > conservative way. What do folks think?
>
> I don't see how any of that deals with the current conflict in -next.
The point is to cleanup the kitchen sink full of knobs everyone from
different subsystem has put in place for random things so to reduce
the amount of edits on the file, so to then avoid the possibility
of merge conflicts.
> You are putting the fs sysctls in the wrong place. The should live
> in fs/ not in fs/proc/.
That's an easy fix, sure, I'll do that.
> Otherwise you are pretty much repeating
> the problem the problem of poorly located code in another location.
Sure, alright, well I'll chug on with trying to clean up the kitchen
sink. We can decide where we put items during review.
> > fs/proc/Makefile | 1 +
> > fs/proc/fs_sysctl_table.c | 97 +++
> > kernel/sysctl.c | 48 ---
> > 3 files changed, 98 insertions(+), 48 deletions(-)
> > create mode 100644 fs/proc/fs_sysctl_table.c
> >
> > diff --git a/fs/proc/Makefile b/fs/proc/Makefile
> > index bd08616ed8ba..8bf419b2ac7d 100644
> > --- a/fs/proc/Makefile
> > +++ b/fs/proc/Makefile
> > @@ -28,6 +28,7 @@ proc-y+= namespaces.o
> > proc-y += self.o
> > proc-y += thread_self.o
> > proc-$(CONFIG_PROC_SYSCTL) += proc_sysctl.o
> > +proc-$(CONFIG_SYSCTL) += fs_sysctl_table.o
> > proc-$(CONFIG_NET) += proc_net.o
> > proc-$(CONFIG_PROC_KCORE) += kcore.o
> > proc-$(CONFIG_PROC_VMCORE) += vmcore.o
> > diff --git a/fs/proc/fs_sysctl_table.c b/fs/proc/fs_sysctl_table.c
> > new file mode 100644
> > index ..f56a49989872
> > --- /dev/null
> > +++ b/fs/proc/fs_sysctl_table.c
> > @@ -0,0 +1,97 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * /proc/sys/fs sysctl table
> > + */
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +static unsigned long zero_ul;
> > +static unsigned long long_max = LONG_MAX;
> > +
> > +static struct ctl_table fs_table[] = {
> > + {
> > + .procname = "inode-nr",
> > + .data = _stat,
> > + .maxlen = 2*sizeof(long),
> > + .mode = 0444,
> > + .proc_handler = proc_nr_inodes,
> > + },
> > + {
> > + .procname = "inode-state",
> > + .data = _stat,
> > + .maxlen = 7*sizeof(long),
> > + .mode = 0444,
> > + .proc_handler =
Re: linux-next: manual merge of the vfs tree with the parisc-hd tree
Luis Chamberlain writes:
> On Mon, May 11, 2020 at 09:55:16AM +0800, Xiaoming Ni wrote:
>> On 2020/5/11 9:11, Stephen Rothwell wrote:
>> > Hi all,
>> >
>> > Today's linux-next merge of the vfs tree got a conflict in:
>> >
>> >kernel/sysctl.c
>> >
>> > between commit:
>> >
>> >b6522fa409cf ("parisc: add sysctl file interface
>> > panic_on_stackoverflow")
>> >
>> > from the parisc-hd tree and commit:
>> >
>> >f461d2dcd511 ("sysctl: avoid forward declarations")
>> >
>> > from the vfs tree.
>> >
>> > I fixed it up (see below) and can carry the fix as necessary. This
>> > is now fixed as far as linux-next is concerned, but any non trivial
>> > conflicts should be mentioned to your upstream maintainer when your tree
>> > is submitted for merging. You may also want to consider cooperating
>> > with the maintainer of the conflicting tree to minimise any particularly
>> > complex conflicts.
>> >
>>
>>
>> Kernel/sysctl.c contains more than 190 interface files, and there are a
>> large number of config macro controls. When modifying the sysctl interface
>> directly in kernel/sysctl.c , conflicts are very easy to occur.
>>
>> At the same time, the register_sysctl_table() provided by the system can
>> easily add the sysctl interface, and there is no conflict of kernel/sysctl.c
>> .
>>
>> Should we add instructions in the patch guide (coding-style.rst
>> submitting-patches.rst):
>> Preferentially use register_sysctl_table() to add a new sysctl interface,
>> centralize feature codes, and avoid directly modifying kernel/sysctl.c ?
>
> Yes, however I don't think folks know how to do this well. So I think we
> just have to do at least start ourselves, and then reflect some of this
> in the docs. The reason that this can be not easy is that we need to
> ensure that at an init level we haven't busted dependencies on setting
> this. We also just don't have docs on how to do this well.
>
>> In addition, is it necessary to transfer the architecture-related sysctl
>> interface to arch/xxx/kernel/sysctl.c ?
>
> Well here's an initial attempt to start with fs stuff in a very
> conservative way. What do folks think?
I don't see how any of that deals with the current conflict in -next.
You are putting the fs sysctls in the wrong place. The should live
in fs/ not in fs/proc/. Otherwise you are pretty much repeating
the problem the problem of poorly located code in another location.
> fs/proc/Makefile | 1 +
> fs/proc/fs_sysctl_table.c | 97 +++
> kernel/sysctl.c | 48 ---
> 3 files changed, 98 insertions(+), 48 deletions(-)
> create mode 100644 fs/proc/fs_sysctl_table.c
>
> diff --git a/fs/proc/Makefile b/fs/proc/Makefile
> index bd08616ed8ba..8bf419b2ac7d 100644
> --- a/fs/proc/Makefile
> +++ b/fs/proc/Makefile
> @@ -28,6 +28,7 @@ proc-y += namespaces.o
> proc-y += self.o
> proc-y += thread_self.o
> proc-$(CONFIG_PROC_SYSCTL) += proc_sysctl.o
> +proc-$(CONFIG_SYSCTL)+= fs_sysctl_table.o
> proc-$(CONFIG_NET) += proc_net.o
> proc-$(CONFIG_PROC_KCORE)+= kcore.o
> proc-$(CONFIG_PROC_VMCORE) += vmcore.o
> diff --git a/fs/proc/fs_sysctl_table.c b/fs/proc/fs_sysctl_table.c
> new file mode 100644
> index ..f56a49989872
> --- /dev/null
> +++ b/fs/proc/fs_sysctl_table.c
> @@ -0,0 +1,97 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * /proc/sys/fs sysctl table
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +static unsigned long zero_ul;
> +static unsigned long long_max = LONG_MAX;
> +
> +static struct ctl_table fs_table[] = {
> + {
> + .procname = "inode-nr",
> + .data = _stat,
> + .maxlen = 2*sizeof(long),
> + .mode = 0444,
> + .proc_handler = proc_nr_inodes,
> + },
> + {
> + .procname = "inode-state",
> + .data = _stat,
> + .maxlen = 7*sizeof(long),
> + .mode = 0444,
> + .proc_handler = proc_nr_inodes,
> + },
> + {
> + .procname = "file-nr",
> + .data = _stat,
> + .maxlen = sizeof(files_stat),
> + .mode = 0444,
> + .proc_handler = proc_nr_files,
> + },
> + {
> + .procname = "file-max",
> + .data = _stat.max_files,
> + .maxlen = sizeof(files_stat.max_files),
> + .mode = 0644,
> + .proc_handler = proc_doulongvec_minmax,
> + .extra1 = _ul,
> + .extra2 = _max,
> + },
> + {
> + .procname
Re: linux-next: manual merge of the vfs tree with the parisc-hd tree
On Mon, May 11, 2020 at 10:22:04PM -0700, Kees Cook wrote:
> On Tue, May 12, 2020 at 12:33:05AM +, Luis Chamberlain wrote:
> > On Mon, May 11, 2020 at 09:55:16AM +0800, Xiaoming Ni wrote:
> > > On 2020/5/11 9:11, Stephen Rothwell wrote:
> > > > Hi all,
> > > >
> > > > Today's linux-next merge of the vfs tree got a conflict in:
> > > >
> > > >kernel/sysctl.c
> > > >
> > > > between commit:
> > > >
> > > >b6522fa409cf ("parisc: add sysctl file interface
> > > > panic_on_stackoverflow")
> > > >
> > > > from the parisc-hd tree and commit:
> > > >
> > > >f461d2dcd511 ("sysctl: avoid forward declarations")
> > > >
> > > > from the vfs tree.
> > > >
> > > > I fixed it up (see below) and can carry the fix as necessary. This
> > > > is now fixed as far as linux-next is concerned, but any non trivial
> > > > conflicts should be mentioned to your upstream maintainer when your tree
> > > > is submitted for merging. You may also want to consider cooperating
> > > > with the maintainer of the conflicting tree to minimise any particularly
> > > > complex conflicts.
> > > >
> > >
> > >
> > > Kernel/sysctl.c contains more than 190 interface files, and there are a
> > > large number of config macro controls. When modifying the sysctl interface
> > > directly in kernel/sysctl.c , conflicts are very easy to occur.
> > >
> > > At the same time, the register_sysctl_table() provided by the system can
> > > easily add the sysctl interface, and there is no conflict of
> > > kernel/sysctl.c
> > > .
> > >
> > > Should we add instructions in the patch guide (coding-style.rst
> > > submitting-patches.rst):
> > > Preferentially use register_sysctl_table() to add a new sysctl interface,
> > > centralize feature codes, and avoid directly modifying kernel/sysctl.c ?
> >
> > Yes, however I don't think folks know how to do this well. So I think we
> > just have to do at least start ourselves, and then reflect some of this
> > in the docs. The reason that this can be not easy is that we need to
> > ensure that at an init level we haven't busted dependencies on setting
> > this. We also just don't have docs on how to do this well.
> >
> > > In addition, is it necessary to transfer the architecture-related sysctl
> > > interface to arch/xxx/kernel/sysctl.c ?
> >
> > Well here's an initial attempt to start with fs stuff in a very
> > conservative way. What do folks think?
> >
> > [...]
> > +static unsigned long zero_ul;
> > +static unsigned long long_max = LONG_MAX;
>
> I think it'd be nice to keep these in one place for others to reuse,
> though that means making them non-static. (And now that I look at them,
> I thought they were supposed to be const?)
So much spring cleaning to do. I can add the const and share it.
It seems odd to stuff this into a sysctl.h, types.h doesn't seem
right... I can't think of something proper, so I'll just move them
to sysctl.h for now.
Any thought on the approach though? I mean, I realize that this will
require more of the subsystem specific folks to look at the code and
review, but if this seems fair, I'll get the ball rolling.
Luis
Re: linux-next: manual merge of the vfs tree with the parisc-hd tree
On Tue, May 12, 2020 at 12:33:05AM +, Luis Chamberlain wrote:
> On Mon, May 11, 2020 at 09:55:16AM +0800, Xiaoming Ni wrote:
> > On 2020/5/11 9:11, Stephen Rothwell wrote:
> > > Hi all,
> > >
> > > Today's linux-next merge of the vfs tree got a conflict in:
> > >
> > >kernel/sysctl.c
> > >
> > > between commit:
> > >
> > >b6522fa409cf ("parisc: add sysctl file interface
> > > panic_on_stackoverflow")
> > >
> > > from the parisc-hd tree and commit:
> > >
> > >f461d2dcd511 ("sysctl: avoid forward declarations")
> > >
> > > from the vfs tree.
> > >
> > > I fixed it up (see below) and can carry the fix as necessary. This
> > > is now fixed as far as linux-next is concerned, but any non trivial
> > > conflicts should be mentioned to your upstream maintainer when your tree
> > > is submitted for merging. You may also want to consider cooperating
> > > with the maintainer of the conflicting tree to minimise any particularly
> > > complex conflicts.
> > >
> >
> >
> > Kernel/sysctl.c contains more than 190 interface files, and there are a
> > large number of config macro controls. When modifying the sysctl interface
> > directly in kernel/sysctl.c , conflicts are very easy to occur.
> >
> > At the same time, the register_sysctl_table() provided by the system can
> > easily add the sysctl interface, and there is no conflict of kernel/sysctl.c
> > .
> >
> > Should we add instructions in the patch guide (coding-style.rst
> > submitting-patches.rst):
> > Preferentially use register_sysctl_table() to add a new sysctl interface,
> > centralize feature codes, and avoid directly modifying kernel/sysctl.c ?
>
> Yes, however I don't think folks know how to do this well. So I think we
> just have to do at least start ourselves, and then reflect some of this
> in the docs. The reason that this can be not easy is that we need to
> ensure that at an init level we haven't busted dependencies on setting
> this. We also just don't have docs on how to do this well.
>
> > In addition, is it necessary to transfer the architecture-related sysctl
> > interface to arch/xxx/kernel/sysctl.c ?
>
> Well here's an initial attempt to start with fs stuff in a very
> conservative way. What do folks think?
>
> [...]
> +static unsigned long zero_ul;
> +static unsigned long long_max = LONG_MAX;
I think it'd be nice to keep these in one place for others to reuse,
though that means making them non-static. (And now that I look at them,
I thought they were supposed to be const?)
--
Kees Cook
Re: linux-next: manual merge of the vfs tree with the parisc-hd tree
On Mon, May 11, 2020 at 09:55:16AM +0800, Xiaoming Ni wrote:
> On 2020/5/11 9:11, Stephen Rothwell wrote:
> > Hi all,
> >
> > Today's linux-next merge of the vfs tree got a conflict in:
> >
> >kernel/sysctl.c
> >
> > between commit:
> >
> >b6522fa409cf ("parisc: add sysctl file interface panic_on_stackoverflow")
> >
> > from the parisc-hd tree and commit:
> >
> >f461d2dcd511 ("sysctl: avoid forward declarations")
> >
> > from the vfs tree.
> >
> > I fixed it up (see below) and can carry the fix as necessary. This
> > is now fixed as far as linux-next is concerned, but any non trivial
> > conflicts should be mentioned to your upstream maintainer when your tree
> > is submitted for merging. You may also want to consider cooperating
> > with the maintainer of the conflicting tree to minimise any particularly
> > complex conflicts.
> >
>
>
> Kernel/sysctl.c contains more than 190 interface files, and there are a
> large number of config macro controls. When modifying the sysctl interface
> directly in kernel/sysctl.c , conflicts are very easy to occur.
>
> At the same time, the register_sysctl_table() provided by the system can
> easily add the sysctl interface, and there is no conflict of kernel/sysctl.c
> .
>
> Should we add instructions in the patch guide (coding-style.rst
> submitting-patches.rst):
> Preferentially use register_sysctl_table() to add a new sysctl interface,
> centralize feature codes, and avoid directly modifying kernel/sysctl.c ?
Yes, however I don't think folks know how to do this well. So I think we
just have to do at least start ourselves, and then reflect some of this
in the docs. The reason that this can be not easy is that we need to
ensure that at an init level we haven't busted dependencies on setting
this. We also just don't have docs on how to do this well.
> In addition, is it necessary to transfer the architecture-related sysctl
> interface to arch/xxx/kernel/sysctl.c ?
Well here's an initial attempt to start with fs stuff in a very
conservative way. What do folks think?
fs/proc/Makefile | 1 +
fs/proc/fs_sysctl_table.c | 97 +++
kernel/sysctl.c | 48 ---
3 files changed, 98 insertions(+), 48 deletions(-)
create mode 100644 fs/proc/fs_sysctl_table.c
diff --git a/fs/proc/Makefile b/fs/proc/Makefile
index bd08616ed8ba..8bf419b2ac7d 100644
--- a/fs/proc/Makefile
+++ b/fs/proc/Makefile
@@ -28,6 +28,7 @@ proc-y+= namespaces.o
proc-y += self.o
proc-y += thread_self.o
proc-$(CONFIG_PROC_SYSCTL) += proc_sysctl.o
+proc-$(CONFIG_SYSCTL) += fs_sysctl_table.o
proc-$(CONFIG_NET) += proc_net.o
proc-$(CONFIG_PROC_KCORE) += kcore.o
proc-$(CONFIG_PROC_VMCORE) += vmcore.o
diff --git a/fs/proc/fs_sysctl_table.c b/fs/proc/fs_sysctl_table.c
new file mode 100644
index ..f56a49989872
--- /dev/null
+++ b/fs/proc/fs_sysctl_table.c
@@ -0,0 +1,97 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * /proc/sys/fs sysctl table
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+static unsigned long zero_ul;
+static unsigned long long_max = LONG_MAX;
+
+static struct ctl_table fs_table[] = {
+ {
+ .procname = "inode-nr",
+ .data = _stat,
+ .maxlen = 2*sizeof(long),
+ .mode = 0444,
+ .proc_handler = proc_nr_inodes,
+ },
+ {
+ .procname = "inode-state",
+ .data = _stat,
+ .maxlen = 7*sizeof(long),
+ .mode = 0444,
+ .proc_handler = proc_nr_inodes,
+ },
+ {
+ .procname = "file-nr",
+ .data = _stat,
+ .maxlen = sizeof(files_stat),
+ .mode = 0444,
+ .proc_handler = proc_nr_files,
+ },
+ {
+ .procname = "file-max",
+ .data = _stat.max_files,
+ .maxlen = sizeof(files_stat.max_files),
+ .mode = 0644,
+ .proc_handler = proc_doulongvec_minmax,
+ .extra1 = _ul,
+ .extra2 = _max,
+ },
+ {
+ .procname = "nr_open",
+ .data = _nr_open,
+ .maxlen = sizeof(unsigned int),
+ .mode = 0644,
+ .proc_handler = proc_dointvec_minmax,
+ .extra1 = _nr_open_min,
+ .extra2 = _nr_open_max,
+ },
+ {
+ .procname = "dentry-state",
+ .data = _stat,
+ .maxlen = 6*sizeof(long),
+
Re: linux-next: manual merge of the vfs tree with the parisc-hd tree
On 2020/5/11 9:11, Stephen Rothwell wrote:
Hi all,
Today's linux-next merge of the vfs tree got a conflict in:
kernel/sysctl.c
between commit:
b6522fa409cf ("parisc: add sysctl file interface panic_on_stackoverflow")
from the parisc-hd tree and commit:
f461d2dcd511 ("sysctl: avoid forward declarations")
from the vfs tree.
I fixed it up (see below) and can carry the fix as necessary. This
is now fixed as far as linux-next is concerned, but any non trivial
conflicts should be mentioned to your upstream maintainer when your tree
is submitted for merging. You may also want to consider cooperating
with the maintainer of the conflicting tree to minimise any particularly
complex conflicts.
Kernel/sysctl.c contains more than 190 interface files, and there are a
large number of config macro controls. When modifying the sysctl
interface directly in kernel/sysctl.c , conflicts are very easy to occur.
At the same time, the register_sysctl_table() provided by the system can
easily add the sysctl interface, and there is no conflict of
kernel/sysctl.c .
Should we add instructions in the patch guide (coding-style.rst
submitting-patches.rst):
Preferentially use register_sysctl_table() to add a new sysctl
interface, centralize feature codes, and avoid directly modifying
kernel/sysctl.c ?
In addition, is it necessary to transfer the architecture-related sysctl
interface to arch/xxx/kernel/sysctl.c ?
Thanks
Xiaoming Ni
linux-next: manual merge of the vfs tree with the parisc-hd tree
Hi all,
Today's linux-next merge of the vfs tree got a conflict in:
kernel/sysctl.c
between commit:
b6522fa409cf ("parisc: add sysctl file interface panic_on_stackoverflow")
from the parisc-hd tree and commit:
f461d2dcd511 ("sysctl: avoid forward declarations")
from the vfs tree.
I fixed it up (see below) and can carry the fix as necessary. This
is now fixed as far as linux-next is concerned, but any non trivial
conflicts should be mentioned to your upstream maintainer when your tree
is submitted for merging. You may also want to consider cooperating
with the maintainer of the conflicting tree to minimise any particularly
complex conflicts.
--
Cheers,
Stephen Rothwell
diff --cc kernel/sysctl.c
index b9ff323e1d26,e961286d0e14..
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@@ -3372,71 -1576,1732 +1576,1734 @@@ int proc_do_large_bitmap(struct ctl_tab
return -ENOSYS;
}
- int proc_dointvec_userhz_jiffies(struct ctl_table *table, int write,
- void __user *buffer, size_t *lenp, loff_t *ppos)
- {
- return -ENOSYS;
- }
+ #endif /* CONFIG_PROC_SYSCTL */
+
+ #if defined(CONFIG_SYSCTL)
+ int proc_do_static_key(struct ctl_table *table, int write,
+ void *buffer, size_t *lenp, loff_t *ppos)
+ {
+ struct static_key *key = (struct static_key *)table->data;
+ static DEFINE_MUTEX(static_key_mutex);
+ int val, ret;
+ struct ctl_table tmp = {
+ .data = ,
+ .maxlen = sizeof(val),
+ .mode = table->mode,
+ .extra1 = SYSCTL_ZERO,
+ .extra2 = SYSCTL_ONE,
+ };
+
+ if (write && !capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
+ mutex_lock(_key_mutex);
+ val = static_key_enabled(key);
+ ret = proc_dointvec_minmax(, write, buffer, lenp, ppos);
+ if (write && !ret) {
+ if (val)
+ static_key_enable(key);
+ else
+ static_key_disable(key);
+ }
+ mutex_unlock(_key_mutex);
+ return ret;
+ }
+
+ static struct ctl_table kern_table[] = {
+ {
+ .procname = "sched_child_runs_first",
+ .data = _sched_child_runs_first,
+ .maxlen = sizeof(unsigned int),
+ .mode = 0644,
+ .proc_handler = proc_dointvec,
+ },
+ #ifdef CONFIG_SCHED_DEBUG
+ {
+ .procname = "sched_min_granularity_ns",
+ .data = _sched_min_granularity,
+ .maxlen = sizeof(unsigned int),
+ .mode = 0644,
+ .proc_handler = sched_proc_update_handler,
+ .extra1 = _sched_granularity_ns,
+ .extra2 = _sched_granularity_ns,
+ },
+ {
+ .procname = "sched_latency_ns",
+ .data = _sched_latency,
+ .maxlen = sizeof(unsigned int),
+ .mode = 0644,
+ .proc_handler = sched_proc_update_handler,
+ .extra1 = _sched_granularity_ns,
+ .extra2 = _sched_granularity_ns,
+ },
+ {
+ .procname = "sched_wakeup_granularity_ns",
+ .data = _sched_wakeup_granularity,
+ .maxlen = sizeof(unsigned int),
+ .mode = 0644,
+ .proc_handler = sched_proc_update_handler,
+ .extra1 = _wakeup_granularity_ns,
+ .extra2 = _wakeup_granularity_ns,
+ },
+ #ifdef CONFIG_SMP
+ {
+ .procname = "sched_tunable_scaling",
+ .data = _sched_tunable_scaling,
+ .maxlen = sizeof(enum sched_tunable_scaling),
+ .mode = 0644,
+ .proc_handler = sched_proc_update_handler,
+ .extra1 = _sched_tunable_scaling,
+ .extra2 = _sched_tunable_scaling,
+ },
+ {
+ .procname = "sched_migration_cost_ns",
+ .data = _sched_migration_cost,
+ .maxlen = sizeof(unsigned int),
+ .mode = 0644,
+ .proc_handler = proc_dointvec,
+ },
+ {
+ .procname = "sched_nr_migrate",
+ .data = _sched_nr_migrate,
+ .maxlen = sizeof(unsigned int),
+ .mode = 0644,
+ .proc_handler = proc_dointvec,
+ },
+ #ifdef CONFIG_SCHEDSTATS
+ {
+ .procname = "sched_schedstats",
+ .data = NULL,
+ .maxlen = sizeof(unsigned int),
+ .mode = 0644,
+ .proc_handler = sysctl_schedstats,
+
