| Message ID | 20230522210814.1919325-3-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | kernel/sysctl.c: remove to major base directories | expand |
On 5/22/23 14:08, Luis Chamberlain wrote: > --- a/arch/x86/kernel/umip.c > +++ b/arch/x86/kernel/umip.c > @@ -12,6 +12,7 @@ > #include <asm/insn.h> > #include <asm/insn-eval.h> > #include <linux/ratelimit.h> > +#include <linux/signal.h> Oh, so this is actually fixing a bug: umip.c uses 'show_unhandled_signals' but it doesn't explicitly include linux/signal.h where 'show_unhandled_signals' is declared. It doesn't actually have anything to do with moving the show_unhandled_signals sysctl, right? If that's the case, it would be nice to have this in its own patch.
On Tue, May 23, 2023 at 07:16:55AM -0700, Dave Hansen wrote: > On 5/22/23 14:08, Luis Chamberlain wrote: > > --- a/arch/x86/kernel/umip.c > > +++ b/arch/x86/kernel/umip.c > > @@ -12,6 +12,7 @@ > > #include <asm/insn.h> > > #include <asm/insn-eval.h> > > #include <linux/ratelimit.h> > > +#include <linux/signal.h> > > Oh, so this is actually fixing a bug: umip.c uses > 'show_unhandled_signals' but it doesn't explicitly include > linux/signal.h where 'show_unhandled_signals' is declared. Fixes a non-critical bug perhaps, but I doubt it would be fixing a functional bug as otherwise folks would have reported a build bug, no? What or how it ends up including that file today to avoid build failures is beyond me. > It doesn't actually have anything to do with moving the > show_unhandled_signals sysctl, right? Well in my case it is making sure the sysctl variable used is declared as well. > If that's the case, it would be nice to have this in its own patch. If its not really fixing any build bugs or functional bugs I don't see the need. But if you really want it, I can do it. Let me know! Luis
On Wed, May 24, 2023 at 12:30:37AM -0700, Luis Chamberlain wrote:
> Let me know!
Re-poke. I know, it's just been a day :P
Luis
On 5/24/23 00:30, Luis Chamberlain wrote: >> It doesn't actually have anything to do with moving the >> show_unhandled_signals sysctl, right? > Well in my case it is making sure the sysctl variable used is declared > as well. But what does this have to do with _this_ patch? This: > --- a/arch/x86/kernel/umip.c > +++ b/arch/x86/kernel/umip.c > @@ -12,6 +12,7 @@ > #include <asm/insn.h> > #include <asm/insn-eval.h> > #include <linux/ratelimit.h> > +#include <linux/signal.h> For instance. You don't move things to another header or make *ANY* change to the compilation of umip.c. So why patch it? It looks to me like a _fundamentally_ superfluous change. That hunk literally *can't* be related to the rest of the patch. >> If that's the case, it would be nice to have this in its own patch. > If its not really fixing any build bugs or functional bugs I don't see > the need. But if you really want it, I can do it. > > Let me know! Yes, I really want it. Please remove all the x86 bits from _this_ patch. If x86 has a separate, preexisting problem, please send that patch separately with a separate changelog and justification. We'll take a look.
On Thu, May 25, 2023 at 11:52:58AM -0700, Dave Hansen wrote: > On 5/24/23 00:30, Luis Chamberlain wrote: > >> It doesn't actually have anything to do with moving the > >> show_unhandled_signals sysctl, right? > > Well in my case it is making sure the sysctl variable used is declared > > as well. > > But what does this have to do with _this_ patch? This: Because to create consistency for the users. > > --- a/arch/x86/kernel/umip.c > > +++ b/arch/x86/kernel/umip.c > > @@ -12,6 +12,7 @@ > > #include <asm/insn.h> > > #include <asm/insn-eval.h> > > #include <linux/ratelimit.h> > > +#include <linux/signal.h> > > For instance. You don't move things to another header or make *ANY* > change to the compilation of umip.c. So why patch it? > > It looks to me like a _fundamentally_ superfluous change. That hunk > literally *can't* be related to the rest of the patch. I suspect it is not needed as otherwise compilation would have failed. So I'll just drop it. > >> If that's the case, it would be nice to have this in its own patch. > > If its not really fixing any build bugs or functional bugs I don't see > > the need. But if you really want it, I can do it. > > > > Let me know! > > Yes, I really want it. > > Please remove all the x86 bits from _this_ patch. If x86 has a > separate, preexisting problem, please send that patch separately with a > separate changelog and justification. > > We'll take a look. Sounds good. Luis
diff --git a/arch/parisc/kernel/traps.c b/arch/parisc/kernel/traps.c index f9696fbf646c..e15f7e201962 100644 --- a/arch/parisc/kernel/traps.c +++ b/arch/parisc/kernel/traps.c @@ -23,6 +23,7 @@ #include <linux/module.h> #include <linux/smp.h> #include <linux/spinlock.h> +#include <linux/signal.h> #include <linux/init.h> #include <linux/interrupt.h> #include <linux/console.h> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c index 004cb30b7419..91905377d708 100644 --- a/arch/x86/kernel/signal.c +++ b/arch/x86/kernel/signal.c @@ -12,6 +12,7 @@ #include <linux/sched.h> #include <linux/sched/task_stack.h> +#include <linux/signal.h> #include <linux/mm.h> #include <linux/smp.h> #include <linux/kernel.h> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c index 58b1f208eff5..180d770f8817 100644 --- a/arch/x86/kernel/traps.c +++ b/arch/x86/kernel/traps.c @@ -31,6 +31,7 @@ #include <linux/kexec.h> #include <linux/sched.h> #include <linux/sched/task_stack.h> +#include <linux/signal.h> #include <linux/timer.h> #include <linux/init.h> #include <linux/bug.h> diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c index 5a4b21389b1d..cef5240dcd92 100644 --- a/arch/x86/kernel/umip.c +++ b/arch/x86/kernel/umip.c @@ -12,6 +12,7 @@ #include <asm/insn.h> #include <asm/insn-eval.h> #include <linux/ratelimit.h> +#include <linux/signal.h> #undef pr_fmt #define pr_fmt(fmt) "umip: " fmt diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c index e4399983c50c..e5f13250e68c 100644 --- a/arch/x86/mm/fault.c +++ b/arch/x86/mm/fault.c @@ -6,6 +6,7 @@ */ #include <linux/sched.h> /* test_thread_flag(), ... */ #include <linux/sched/task_stack.h> /* task_stack_*(), ... */ +#include <linux/sched/signal.h> /* show_unhandled_signals */ #include <linux/kdebug.h> /* oops_begin/end, ... */ #include <linux/extable.h> /* search_exception_tables */ #include <linux/memblock.h> /* max_low_pfn */ diff --git a/kernel/signal.c b/kernel/signal.c index 8f6330f0e9ca..5ba4150c01a7 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -45,6 +45,7 @@ #include <linux/posix-timers.h> #include <linux/cgroup.h> #include <linux/audit.h> +#include <linux/sysctl.h> #define CREATE_TRACE_POINTS #include <trace/events/signal.h> @@ -4771,6 +4772,28 @@ static inline void siginfo_buildtime_checks(void) #endif } +#if defined(CONFIG_SYSCTL) +static struct ctl_table signal_debug_table[] = { +#ifdef CONFIG_SYSCTL_EXCEPTION_TRACE + { + .procname = "exception-trace", + .data = &show_unhandled_signals, + .maxlen = sizeof(int), + .mode = 0644, + .proc_handler = proc_dointvec + }, +#endif + { } +}; + +static int __init init_signal_sysctls(void) +{ + register_sysctl_init("debug", signal_debug_table); + return 0; +} +early_initcall(init_signal_sysctls); +#endif /* CONFIG_SYSCTL */ + void __init signals_init(void) { siginfo_buildtime_checks(); diff --git a/kernel/sysctl.c b/kernel/sysctl.c index a7fdb828afb6..43240955dcad 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -2331,24 +2331,10 @@ static struct ctl_table vm_table[] = { { } }; -static struct ctl_table debug_table[] = { -#ifdef CONFIG_SYSCTL_EXCEPTION_TRACE - { - .procname = "exception-trace", - .data = &show_unhandled_signals, - .maxlen = sizeof(int), - .mode = 0644, - .proc_handler = proc_dointvec - }, -#endif - { } -}; - int __init sysctl_init_bases(void) { register_sysctl_init("kernel", kern_table); register_sysctl_init("vm", vm_table); - register_sysctl_init("debug", debug_table); return 0; }
The show_unhandled_signals sysctl is the only sysctl for debug left on kernel/sysctl.c. We've been moving the syctls out from kernel/sysctl.c so to help avoid merge conflicts as the shared array gets out of hand. This change incurs simplifies sysctl registration by localizing it where it should go for a penalty in size of increasing the kernel by 23 bytes, we accept this given recent cleanups have actually already saved us 1465 bytes in the prior commits. ./scripts/bloat-o-meter vmlinux.3-remove-dev-table vmlinux.4-remove-debug-table add/remove: 3/1 grow/shrink: 0/1 up/down: 177/-154 (23) Function old new delta signal_debug_table - 128 +128 init_signal_sysctls - 33 +33 __pfx_init_signal_sysctls - 16 +16 sysctl_init_bases 85 59 -26 debug_table 128 - -128 Total: Before=21256967, After=21256990, chg +0.00% Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- arch/parisc/kernel/traps.c | 1 + arch/x86/kernel/signal.c | 1 + arch/x86/kernel/traps.c | 1 + arch/x86/kernel/umip.c | 1 + arch/x86/mm/fault.c | 1 + kernel/signal.c | 23 +++++++++++++++++++++++ kernel/sysctl.c | 14 -------------- 7 files changed, 28 insertions(+), 14 deletions(-)