[2/2] signal: move show_unhandled_signals sysctl to its own file
Commit Message
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(-)
Comments
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
@@ -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>
@@ -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>
@@ -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>
@@ -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
@@ -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 */
@@ -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();
@@ -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;
}