Hi Johannes,
On Tue, Oct 22, 2024 at 3:19 PM Johannes Berg <johannes@xxxxxxxxxxxxxxxx> wrote:
> From: Johannes Berg <johannes.berg@xxxxxxxxx>
>
> As struct file_operations is really big, but (most) debugfs
> files only use simple_open, read, write and perhaps seek, and
> don't need anything else, this wastes a lot of space for NULL
> pointers.
>
> Add a struct debugfs_short_fops and some bookkeeping code in
> debugfs so that users can use that with debugfs_create_file()
> using _Generic to figure out which function to use.
>
> Converting mac80211 to use it where possible saves quite a
> bit of space:
>
> 1010127 205064 1220 1216411 128f9b net/mac80211/mac80211.ko (before)
> 981199 205064 1220 1187483 121e9b net/mac80211/mac80211.ko (after)
> -------
> -28928 = ~28KiB
>
> With a marginal space cost in debugfs:
>
> 8701 550 16 9267 2433 fs/debugfs/inode.o (before)
> 25233 325 32 25590 63f6 fs/debugfs/file.o (before)
> 8914 558 16 9488 2510 fs/debugfs/inode.o (after)
> 25380 325 32 25737 6489 fs/debugfs/file.o (after)
> ---------------
> +360 +8
>
> (All on x86-64)
>
> A simple spatch suggests there are more than 300 instances,
> not even counting the ones hidden in macros like in mac80211,
> that could be trivially converted, for additional savings of
> about 240 bytes for each.
>
> Signed-off-by: Johannes Berg <johannes.berg@xxxxxxxxx>
Thanks for your patch, which is now commit 8dc6d81c6b2acc43 ("debugfs:
add small file operations for most files") upstream.
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> -struct dentry *debugfs_create_file(const char *name, umode_t mode,
> - struct dentry *parent, void *data,
> - const struct file_operations *fops)
> +struct dentry *debugfs_create_file_full(const char *name, umode_t mode,
> + struct dentry *parent, void *data,
> + const struct file_operations *fops)
> {
> + if (WARN_ON((unsigned long)fops &
> + (DEBUGFS_FSDATA_IS_SHORT_FOPS_BIT |
> + DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)))
> + return ERR_PTR(-EINVAL);
>
This warning is triggered during boot on m68k:
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at fs/debugfs/inode.c:457
debugfs_create_file_full+0x36/0x70
Modules linked in:
CPU: 0 UID: 0 PID: 1 Comm: swapper Not tainted
6.12.0-atari-11072-gdeea1aa0a8f2 #1787
Stack from 0102fe78:
0102fe78 0051acfd 0051acfd 00000000 00200188 005321b8 0042af36 0051acfd
0002459c 00000000 005321b8 000001c9 00000009 00000000 005aff3c 0060f4d8
00024644 005321b8 000001c9 00200188 00000009 00000000 00000000 000000b8
00000008 01020c00 00000000 0000209c 005f13e0 00000000 00000000 0102ff88
00200188 005321b8 000001c9 00000009 00000000 005f13fa 005139e6 00000124
00000000 00000000 004370c2 000020f6 000000b8 00000008 01020c00 0003b03e
Call Trace: [<00200188>] debugfs_create_file_full+0x36/0x70
[<0042af36>] dump_stack+0xc/0x10
[<0002459c>] __warn+0x82/0xbc
[<00024644>] warn_slowpath_fmt+0x6e/0xc4
[<00200188>] debugfs_create_file_full+0x36/0x70
[<0000209c>] do_one_initcall+0x0/0x198
[<005f13e0>] tk_debug_sleep_time_init+0x0/0x22
[<00200188>] debugfs_create_file_full+0x36/0x70
[<005f13fa>] tk_debug_sleep_time_init+0x1a/0x22
[<000020f6>] do_one_initcall+0x5a/0x198
[<0003b03e>] parse_args+0x0/0x202
[<0000209c>] do_one_initcall+0x0/0x198
[<0041c948>] strcpy+0x0/0x1c
[<00070007>] alarmtimer_suspend+0x6f/0x1fc
[<005e9628>] kernel_init_freeable+0x140/0x198
[<0041c948>] strcpy+0x0/0x1c
[<005e9676>] kernel_init_freeable+0x18e/0x198
[<005f13e0>] tk_debug_sleep_time_init+0x0/0x22
[<0042b2d2>] kernel_init+0x0/0xf2
[<0042b2e6>] kernel_init+0x14/0xf2
[<0042b2d2>] kernel_init+0x0/0xf2
[<00002524>] ret_from_kernel_thread+0xc/0x14
---[ end trace 0000000000000000 ]---
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at fs/debugfs/inode.c:457
debugfs_create_file_full+0x36/0x70
Modules linked in:
CPU: 0 UID: 0 PID: 1 Comm: swapper Tainted: G W
6.12.0-atari-11072-gdeea1aa0a8f2 #1787
Tainted: [W]=WARN
Stack from 0102fe70:
0102fe70 0051acfd 0051acfd 00000000 00200188 005321b8 0042af36 0051acfd
0002459c 00000000 005321b8 000001c9 00000009 00000000 005aff3c 0060f53c
00024644 005321b8 000001c9 00200188 00000009 00000000 00000000 000000b8
00000008 01020c00 00000000 0000209c 002a44f0 0029c032 005b8a1a 0102ff88
00200188 005321b8 000001c9 00000009 00000000 002a450e 00539121 00000124
00000000 00000000 00462a86 002a44f0 0060f53c 000020f6 000000b8 00000008
Call Trace: [<00200188>] debugfs_create_file_full+0x36/0x70
[<0042af36>] dump_stack+0xc/0x10
[<0002459c>] __warn+0x82/0xbc
[<00024644>] warn_slowpath_fmt+0x6e/0xc4
[<00200188>] debugfs_create_file_full+0x36/0x70
[<0000209c>] do_one_initcall+0x0/0x198
[<002a44f0>] deferred_probe_initcall+0x0/0x8a
[<0029c032>] _mix_pool_bytes+0x14/0x1a
[<00200188>] debugfs_create_file_full+0x36/0x70
[<002a450e>] deferred_probe_initcall+0x1e/0x8a
[<002a44f0>] deferred_probe_initcall+0x0/0x8a
[<000020f6>] do_one_initcall+0x5a/0x198
[<0003b03e>] parse_args+0x0/0x202
[<0000209c>] do_one_initcall+0x0/0x198
[<0041c948>] strcpy+0x0/0x1c
[<00070007>] alarmtimer_suspend+0x6f/0x1fc
[<005e9628>] kernel_init_freeable+0x140/0x198
[<0041c948>] strcpy+0x0/0x1c
[<005e9676>] kernel_init_freeable+0x18e/0x198
[<002a44f0>] deferred_probe_initcall+0x0/0x8a
[<0042b2d2>] kernel_init+0x0/0xf2
[<0042b2e6>] kernel_init+0x14/0xf2
[<0042b2d2>] kernel_init+0x0/0xf2
[<00002524>] ret_from_kernel_thread+0xc/0x14
---[ end trace 0000000000000000 ]---
> --- a/fs/debugfs/internal.h
> +++ b/fs/debugfs/internal.h
> @@ -18,6 +18,7 @@ extern const struct file_operations debugfs_full_proxy_file_operations;
>
> struct debugfs_fsdata {
> const struct file_operations *real_fops;
> + const struct debugfs_short_fops *short_fops;
> union {
> /* automount_fn is used when real_fops is NULL */
> debugfs_automount_t automount;
> @@ -39,6 +40,11 @@ struct debugfs_fsdata {
> * pointer gets its lowest bit set.
> */
> #define DEBUGFS_FSDATA_IS_REAL_FOPS_BIT BIT(0)
> +/*
> + * A dentry's ->d_fsdata, when pointing to real fops, is with
> + * short fops instead of full fops.
> + */
> +#define DEBUGFS_FSDATA_IS_SHORT_FOPS_BIT BIT(1)
As the minimum alignment is 2 on m68k, you cannot use bit 1 in pointers
for your own private use. See also the comment at
https://elixir.bootlin.com/linux/v6.12.1/source/include/linux/maple_tree.h#L44
Reverting commit 8dc6d81c6b2acc43 fixes the issue, and reduces the
atari_defconfig kernel size by 447 bytes, according to bloat-o-meter.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
