Arnd, x86 folks, your review for the second patch would be greatly appreciated.
Now that Joel has cleaned up and removed one of the routines which we wanted
to deprecate, remove two major arrays from kernel/sysctl.c which are empty or
almost empty. One of them, the debug one just needs moving to its source, so
do that.
The move for the signal sysctl costs us 23 bytes but we have already saved
1465 bytes with the other recent cleanup Joel made. The next step is to
depreecate one more call and then we can simplify the registration to only
use ARRAY_SIZE() completely and remove the extra empty entries all over.
That should save us tons of bytes all around in the kernel and we'd then
later kill for good all recursion possible sysctl registration calls.
These patches apply on top of sysctl-next [0] which already carry Joel's patches.
[0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/log/?h=sysctl-next
Luis Chamberlain (2):
sysctl: remove empty dev table
signal: move show_unhandled_signals sysctl to its own file
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 | 19 -------------------
7 files changed, 28 insertions(+), 19 deletions(-)
--
2.39.2
Now that all the dev sysctls have been moved out we can remove the
dev sysctl base directory. We don't need to create base directories,
they are created for you as if using 'mkdir -p' with register_syctl()
and register_sysctl_init(). For details refer to sysctl_mkdir_p()
usage.
We save 90 bytes with this changes:
./scripts/bloat-o-meter vmlinux.2.remove-sysctl-table vmlinux.3-remove-dev-table
add/remove: 0/1 grow/shrink: 0/1 up/down: 0/-90 (-90)
Function old new delta
sysctl_init_bases 111 85 -26
dev_table 64 - -64
Total: Before=21257057, After=21256967, chg -0.00%
Signed-off-by: Luis Chamberlain <[email protected]>
---
kernel/sysctl.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index fa2aa8bd32b6..a7fdb828afb6 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2344,16 +2344,11 @@ static struct ctl_table debug_table[] = {
{ }
};
-static struct ctl_table dev_table[] = {
- { }
-};
-
int __init sysctl_init_bases(void)
{
register_sysctl_init("kernel", kern_table);
register_sysctl_init("vm", vm_table);
register_sysctl_init("debug", debug_table);
- register_sysctl_init("dev", dev_table);
return 0;
}
--
2.39.2
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 <[email protected]>
---
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(-)
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;
}
--
2.39.2
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 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 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 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