Re: linux-next: manual merge of the vfs tree with the parisc-hd tree
From: Xiaoming Ni
Date: Thu May 14 2020 - 12:18:26 EST
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 <mcgrof@xxxxxxxxxx> writes:
On Tue, May 12, 2020 at 06:52:35AM -0500, Eric W. Biederman wrote:
Luis Chamberlain <mcgrof@xxxxxxxxxx> 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)
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 *)&sysctl_vals[0])
#define SYSCTL_ONE ((void *)&sysctl_vals[1])
#define SYSCTL_INT_MAX ((void *)&sysctl_vals[2])
+#define SYSCTL_NEG_ONE ((void *)&sysctl_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 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
.
Thank you for your guidance, I will send the patch later
Xiaoming Ni