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)
+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
.
