| Message ID | 20241022151838.26f9925fb959.Ia80b55e934bbfc45ce0df42a3233d34b35508046@changeid |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] debugfs: add small file operations for most files | expand |
Hi Johannes, On Tue, Oct 22, 2024 at 3:19 PM Johannes Berg <johannes@sipsolutions.net> wrote: > From: Johannes Berg <johannes.berg@intel.com> > > 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@intel.com> 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
Hi Johannes, On Mon, Nov 25, 2024 at 1:37 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Tue, Oct 22, 2024 at 3:19 PM Johannes Berg <johannes@sipsolutions.net> wrote: > > From: Johannes Berg <johannes.berg@intel.com> > > > > 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@intel.com> > > 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 > > --- 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 If you want to support truncated debugfs_fops structures, I am afraid there is no other option than storing a flag, structure type, or structure size inside the structure. The latter would allow us to support not just full and short structures, but also arbitrary truncation, given even more opportunities for size reduction. E.g. something like #define get_method(_ops, _op, _op_fallback) \ ((_ops->size > offsetof(struct debugfs_fops, _op)) ? _ops->_op : _op_fallback) Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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
On Tue, 2024-11-26 at 09:38 +0100, Geert Uytterhoeven wrote: > > > > Thanks for your patch, which is now commit 8dc6d81c6b2acc43 ("debugfs: > > add small file operations for most files") upstream. Or rather "no thanks" ;-) > > > +#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. D'oh. Sorry about that. Though honestly even if I _had_ seen such a comment deep in maple tree code, on that struct I'd probably have assumed it's because there's no pointer in it and thus no alignment anyway... But sounds like you're pointers don't need to be naturally aligned, and so 2-byte alignment is sufficient. I guess we _could_ solve that by __aligned(4) on the fops structs, but ... I'm not sure that makes sense. > Reverting commit 8dc6d81c6b2acc43 fixes the issue, Clearly. Though have to also revert the related patch in wireless :) > and reduces the > atari_defconfig kernel size by 447 bytes, according to bloat-o-meter. Well, fair point, but if we care about size then we can win back more than this by converting about a handful of debugfs files to short ops, and if there are no debugfs files then we wouldn't need debugfs either, I'd think. So I think in a way the size argument goes the other way (with a little bit of extra work), if that was meant to be an argument at all? :-) Anyway, that doesn't help now ... > If you want to support truncated debugfs_fops structures, I am > afraid there is no other option than storing a flag, structure type, > or structure size inside the structure. The latter would allow us > to support not just full and short structures, but also arbitrary > truncation, given even more opportunities for size reduction. > > E.g. something like > > #define get_method(_ops, _op, _op_fallback) \ > ((_ops->size > offsetof(struct debugfs_fops, _op)) ? > _ops->_op : _op_fallback) Yeah I thought about something like that originally, but first of all I'd have to reorder the fields in struct file_operations I think (which may or may not have a design to it), but also that doesn't really help here since I'd still need a way to know the kind of struct that it was for sizeof? And store that somewhere along the way. But we can fix this differently, by just having different ops. We really need to only have this bit of information for the very first open, after that we've already allocated the structures. So duplicating the full proxy struct (as as big as that may be, need to convert one more debugfs file to win it back), we should be able to remove the need for the pointer tag. Below changes should fix the issue, but is only lightly tested so far (with regular and short ops files). Perhaps you'd like to give it a spin? johannes From: Johannes Berg <johannes.berg@intel.com> Date: Tue, 26 Nov 2024 10:29:23 +0100 Subject: [PATCH] fs: debugfs: differentiate short fops with proxy ops Geert reported that my previous short fops debugfs changes broke m68k, because it only has mandatory alignement of two, so we can't stash the "is it short" information into the pointer (as we already did with the "is it real" bit.) Instead, exploit the fact that debugfs_file_get() called on an already open file will already find that the fsdata is no longer the real fops but rather the allocated data that already distinguishes full/short ops, so only open() needs to be able to distinguish. We can achieve that by using two different open functions. Unfortunately this requires another set of full file ops, increasing the size by 536 bytes (x86-64), but that's still a reasonable trade-off given that only converting some of the wireless stack gained over 28k. This brings the total cost of this to around 1k, for wins of 28k (all x86-64). Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> Link: https://lore.kernel.org/CAMuHMdWu_9-L2Te101w8hU7H_2yobJFPXSwwUmGHSJfaPWDKiQ@mail.gmail.com Signed-off-by: Johannes Berg <johannes.berg@intel.com> --- fs/debugfs/file.c | 72 ++++++++++++++++++++++++++++++------------- fs/debugfs/inode.c | 11 +++---- fs/debugfs/internal.h | 6 +--- 3 files changed, 55 insertions(+), 34 deletions(-) diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c index 47dc96dfe386..bdb4f2ca0506 100644 --- a/fs/debugfs/file.c +++ b/fs/debugfs/file.c @@ -64,22 +64,13 @@ const struct file_operations *debugfs_real_fops(const struct file *filp) } EXPORT_SYMBOL_GPL(debugfs_real_fops); -/** - * debugfs_file_get - mark the beginning of file data access - * @dentry: the dentry object whose data is being accessed. - * - * Up to a matching call to debugfs_file_put(), any successive call - * into the file removing functions debugfs_remove() and - * debugfs_remove_recursive() will block. Since associated private - * file data may only get freed after a successful return of any of - * the removal functions, you may safely access it after a successful - * call to debugfs_file_get() without worrying about lifetime issues. - * - * If -%EIO is returned, the file has already been removed and thus, - * it is not safe to access any of its data. If, on the other hand, - * it is allowed to access the file data, zero is returned. - */ -int debugfs_file_get(struct dentry *dentry) +enum dbgfs_get_mode { + DBGFS_GET_ALREADY, + DBGFS_GET_REGULAR, + DBGFS_GET_SHORT, +}; + +static int __debugfs_file_get(struct dentry *dentry, enum dbgfs_get_mode mode) { struct debugfs_fsdata *fsd; void *d_fsd; @@ -96,15 +87,17 @@ int debugfs_file_get(struct dentry *dentry) if (!((unsigned long)d_fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)) { fsd = d_fsd; } else { + if (WARN_ON(mode == DBGFS_GET_ALREADY)) + return -EINVAL; + fsd = kmalloc(sizeof(*fsd), GFP_KERNEL); if (!fsd) return -ENOMEM; - if ((unsigned long)d_fsd & DEBUGFS_FSDATA_IS_SHORT_FOPS_BIT) { + if (mode == DBGFS_GET_SHORT) { fsd->real_fops = NULL; fsd->short_fops = (void *)((unsigned long)d_fsd & - ~(DEBUGFS_FSDATA_IS_REAL_FOPS_BIT | - DEBUGFS_FSDATA_IS_SHORT_FOPS_BIT)); + ~DEBUGFS_FSDATA_IS_REAL_FOPS_BIT); } else { fsd->real_fops = (void *)((unsigned long)d_fsd & ~DEBUGFS_FSDATA_IS_REAL_FOPS_BIT); @@ -138,6 +131,26 @@ int debugfs_file_get(struct dentry *dentry) return 0; } + +/** + * debugfs_file_get - mark the beginning of file data access + * @dentry: the dentry object whose data is being accessed. + * + * Up to a matching call to debugfs_file_put(), any successive call + * into the file removing functions debugfs_remove() and + * debugfs_remove_recursive() will block. Since associated private + * file data may only get freed after a successful return of any of + * the removal functions, you may safely access it after a successful + * call to debugfs_file_get() without worrying about lifetime issues. + * + * If -%EIO is returned, the file has already been removed and thus, + * it is not safe to access any of its data. If, on the other hand, + * it is allowed to access the file data, zero is returned. + */ +int debugfs_file_get(struct dentry *dentry) +{ + return __debugfs_file_get(dentry, DBGFS_GET_ALREADY); +} EXPORT_SYMBOL_GPL(debugfs_file_get); /** @@ -424,7 +437,8 @@ static void __full_proxy_fops_init(struct file_operations *proxy_fops, proxy_fops->unlocked_ioctl = full_proxy_unlocked_ioctl; } -static int full_proxy_open(struct inode *inode, struct file *filp) +static int full_proxy_open(struct inode *inode, struct file *filp, + enum dbgfs_get_mode mode) { struct dentry *dentry = F_DENTRY(filp); const struct file_operations *real_fops; @@ -432,7 +446,7 @@ static int full_proxy_open(struct inode *inode, struct file *filp) struct debugfs_fsdata *fsd; int r; - r = debugfs_file_get(dentry); + r = __debugfs_file_get(dentry, mode); if (r) return r == -EIO ? -ENOENT : r; @@ -491,8 +505,22 @@ static int full_proxy_open(struct inode *inode, struct file *filp) return r; } +static int full_proxy_open_regular(struct inode *inode, struct file *filp) +{ + return full_proxy_open(inode, filp, DBGFS_GET_REGULAR); +} + const struct file_operations debugfs_full_proxy_file_operations = { - .open = full_proxy_open, + .open = full_proxy_open_regular, +}; + +static int full_proxy_open_short(struct inode *inode, struct file *filp) +{ + return full_proxy_open(inode, filp, DBGFS_GET_SHORT); +} + +const struct file_operations debugfs_full_short_proxy_file_operations = { + .open = full_proxy_open_short, }; ssize_t debugfs_attr_read(struct file *file, char __user *buf, diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c index 38a9c7eb97e6..65e46c7b6bf1 100644 --- a/fs/debugfs/inode.c +++ b/fs/debugfs/inode.c @@ -455,8 +455,7 @@ struct dentry *debugfs_create_file_full(const char *name, umode_t mode, const struct file_operations *fops) { if (WARN_ON((unsigned long)fops & - (DEBUGFS_FSDATA_IS_SHORT_FOPS_BIT | - DEBUGFS_FSDATA_IS_REAL_FOPS_BIT))) + DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)) return ERR_PTR(-EINVAL); return __debugfs_create_file(name, mode, parent, data, @@ -471,15 +470,13 @@ struct dentry *debugfs_create_file_short(const char *name, umode_t mode, const struct debugfs_short_fops *fops) { if (WARN_ON((unsigned long)fops & - (DEBUGFS_FSDATA_IS_SHORT_FOPS_BIT | - DEBUGFS_FSDATA_IS_REAL_FOPS_BIT))) + DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)) return ERR_PTR(-EINVAL); return __debugfs_create_file(name, mode, parent, data, - fops ? &debugfs_full_proxy_file_operations : + fops ? &debugfs_full_short_proxy_file_operations : &debugfs_noop_file_operations, - (const void *)((unsigned long)fops | - DEBUGFS_FSDATA_IS_SHORT_FOPS_BIT)); + fops); } EXPORT_SYMBOL_GPL(debugfs_create_file_short); diff --git a/fs/debugfs/internal.h b/fs/debugfs/internal.h index a3edfa4f0d8e..bbae4a228ef4 100644 --- a/fs/debugfs/internal.h +++ b/fs/debugfs/internal.h @@ -15,6 +15,7 @@ struct file_operations; extern const struct file_operations debugfs_noop_file_operations; extern const struct file_operations debugfs_open_proxy_file_operations; extern const struct file_operations debugfs_full_proxy_file_operations; +extern const struct file_operations debugfs_full_short_proxy_file_operations; struct debugfs_fsdata { const struct file_operations *real_fops; @@ -40,11 +41,6 @@ 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) /* Access BITS */ #define DEBUGFS_ALLOW_API BIT(0)
Hi Johannes, On Tue, Nov 26, 2024 at 10:37 AM Johannes Berg <johannes@sipsolutions.net> wrote: > On Tue, 2024-11-26 at 09:38 +0100, Geert Uytterhoeven wrote: > > > Thanks for your patch, which is now commit 8dc6d81c6b2acc43 ("debugfs: > > > add small file operations for most files") upstream. > > Or rather "no thanks" ;-) > > > > > +#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. > > D'oh. Sorry about that. Though honestly even if I _had_ seen such a > comment deep in maple tree code, on that struct I'd probably have > assumed it's because there's no pointer in it and thus no alignment > anyway... > > But sounds like you're pointers don't need to be naturally aligned, and > so 2-byte alignment is sufficient. > > I guess we _could_ solve that by __aligned(4) on the fops structs, but > ... I'm not sure that makes sense. > > > Reverting commit 8dc6d81c6b2acc43 fixes the issue, > > Clearly. Though have to also revert the related patch in wireless :) > > > and reduces the > > atari_defconfig kernel size by 447 bytes, according to bloat-o-meter. > > Well, fair point, but if we care about size then we can win back more > than this by converting about a handful of debugfs files to short ops, > and if there are no debugfs files then we wouldn't need debugfs either, > I'd think. > > So I think in a way the size argument goes the other way (with a little > bit of extra work), if that was meant to be an argument at all? :-) Assuming non-wireless drivers can be converted, too? As none of the classical m68k machines support wireless, so far nothing is gained... > From: Johannes Berg <johannes.berg@intel.com> > Date: Tue, 26 Nov 2024 10:29:23 +0100 > Subject: [PATCH] fs: debugfs: differentiate short fops with proxy ops > > Geert reported that my previous short fops debugfs changes > broke m68k, because it only has mandatory alignement of two, > so we can't stash the "is it short" information into the > pointer (as we already did with the "is it real" bit.) > > Instead, exploit the fact that debugfs_file_get() called on > an already open file will already find that the fsdata is > no longer the real fops but rather the allocated data that > already distinguishes full/short ops, so only open() needs > to be able to distinguish. We can achieve that by using two > different open functions. > > Unfortunately this requires another set of full file ops, > increasing the size by 536 bytes (x86-64), but that's still 226 on m68k. > a reasonable trade-off given that only converting some of > the wireless stack gained over 28k. This brings the total > cost of this to around 1k, for wins of 28k (all x86-64). > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > Link: https://lore.kernel.org/CAMuHMdWu_9-L2Te101w8hU7H_2yobJFPXSwwUmGHSJfaPWDKiQ@mail.gmail.com > Signed-off-by: Johannes Berg <johannes.berg@intel.com> Thanks, that fixed the issue! Tested-by: Geert Uytterhoeven <geert@linux-m68k.org> Gr{oetje,eeting}s, Geert
On Tue, 2024-11-26 at 15:16 +0100, Geert Uytterhoeven wrote: > > Assuming non-wireless drivers can be converted, too? As none of the > classical m68k machines support wireless, so far nothing is gained... Oh, most debugfs files can be converted, since most don't do anything beyond simple_open/read/write/seek and those are covered. > > Unfortunately this requires another set of full file ops, > > increasing the size by 536 bytes (x86-64), but that's still > > 226 on m68k. Not that bad, I guess. So if that's 226 bytes and before you had 447 we'd probably only need to convert 4 instances to have a net win :) > Thanks, that fixed the issue! > Tested-by: Geert Uytterhoeven <geert@linux-m68k.org> Great, thanks for testing! I'll do some more testing of my own and then figure out how to get it into the tree. Greg, any thoughts on that? johannes
diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c index 67299e8b734e..47dc96dfe386 100644 --- a/fs/debugfs/file.c +++ b/fs/debugfs/file.c @@ -100,8 +100,16 @@ int debugfs_file_get(struct dentry *dentry) if (!fsd) return -ENOMEM; - fsd->real_fops = (void *)((unsigned long)d_fsd & - ~DEBUGFS_FSDATA_IS_REAL_FOPS_BIT); + if ((unsigned long)d_fsd & DEBUGFS_FSDATA_IS_SHORT_FOPS_BIT) { + fsd->real_fops = NULL; + fsd->short_fops = (void *)((unsigned long)d_fsd & + ~(DEBUGFS_FSDATA_IS_REAL_FOPS_BIT | + DEBUGFS_FSDATA_IS_SHORT_FOPS_BIT)); + } else { + fsd->real_fops = (void *)((unsigned long)d_fsd & + ~DEBUGFS_FSDATA_IS_REAL_FOPS_BIT); + fsd->short_fops = NULL; + } refcount_set(&fsd->active_users, 1); init_completion(&fsd->active_users_drained); INIT_LIST_HEAD(&fsd->cancellations); @@ -241,9 +249,10 @@ static int debugfs_locked_down(struct inode *inode, { if ((inode->i_mode & 07777 & ~0444) == 0 && !(filp->f_mode & FMODE_WRITE) && - !real_fops->unlocked_ioctl && - !real_fops->compat_ioctl && - !real_fops->mmap) + (!real_fops || + (!real_fops->unlocked_ioctl && + !real_fops->compat_ioctl && + !real_fops->mmap))) return 0; if (security_locked_down(LOCKDOWN_DEBUGFS)) @@ -316,19 +325,38 @@ static ret_type full_proxy_ ## name(proto) \ return r; \ } -FULL_PROXY_FUNC(llseek, loff_t, filp, - PROTO(struct file *filp, loff_t offset, int whence), - ARGS(filp, offset, whence)); +#define FULL_PROXY_FUNC_BOTH(name, ret_type, filp, proto, args) \ +static ret_type full_proxy_ ## name(proto) \ +{ \ + struct dentry *dentry = F_DENTRY(filp); \ + struct debugfs_fsdata *fsd; \ + ret_type r; \ + \ + r = debugfs_file_get(dentry); \ + if (unlikely(r)) \ + return r; \ + fsd = dentry->d_fsdata; \ + if (fsd->real_fops) \ + r = fsd->real_fops->name(args); \ + else \ + r = fsd->short_fops->name(args); \ + debugfs_file_put(dentry); \ + return r; \ +} -FULL_PROXY_FUNC(read, ssize_t, filp, - PROTO(struct file *filp, char __user *buf, size_t size, - loff_t *ppos), - ARGS(filp, buf, size, ppos)); +FULL_PROXY_FUNC_BOTH(llseek, loff_t, filp, + PROTO(struct file *filp, loff_t offset, int whence), + ARGS(filp, offset, whence)); -FULL_PROXY_FUNC(write, ssize_t, filp, - PROTO(struct file *filp, const char __user *buf, size_t size, - loff_t *ppos), - ARGS(filp, buf, size, ppos)); +FULL_PROXY_FUNC_BOTH(read, ssize_t, filp, + PROTO(struct file *filp, char __user *buf, size_t size, + loff_t *ppos), + ARGS(filp, buf, size, ppos)); + +FULL_PROXY_FUNC_BOTH(write, ssize_t, filp, + PROTO(struct file *filp, const char __user *buf, + size_t size, loff_t *ppos), + ARGS(filp, buf, size, ppos)); FULL_PROXY_FUNC(unlocked_ioctl, long, filp, PROTO(struct file *filp, unsigned int cmd, unsigned long arg), @@ -363,7 +391,7 @@ static int full_proxy_release(struct inode *inode, struct file *filp) * not to leak any resources. Releasers must not assume that * ->i_private is still being meaningful here. */ - if (real_fops->release) + if (real_fops && real_fops->release) r = real_fops->release(inode, filp); replace_fops(filp, d_inode(dentry)->i_fop); @@ -373,39 +401,48 @@ static int full_proxy_release(struct inode *inode, struct file *filp) } static void __full_proxy_fops_init(struct file_operations *proxy_fops, - const struct file_operations *real_fops) + struct debugfs_fsdata *fsd) { proxy_fops->release = full_proxy_release; - if (real_fops->llseek) + + if ((fsd->real_fops && fsd->real_fops->llseek) || + (fsd->short_fops && fsd->short_fops->llseek)) proxy_fops->llseek = full_proxy_llseek; - if (real_fops->read) + + if ((fsd->real_fops && fsd->real_fops->read) || + (fsd->short_fops && fsd->short_fops->read)) proxy_fops->read = full_proxy_read; - if (real_fops->write) + + if ((fsd->real_fops && fsd->real_fops->write) || + (fsd->short_fops && fsd->short_fops->write)) proxy_fops->write = full_proxy_write; - if (real_fops->poll) + + if (fsd->real_fops && fsd->real_fops->poll) proxy_fops->poll = full_proxy_poll; - if (real_fops->unlocked_ioctl) + + if (fsd->real_fops && fsd->real_fops->unlocked_ioctl) proxy_fops->unlocked_ioctl = full_proxy_unlocked_ioctl; } static int full_proxy_open(struct inode *inode, struct file *filp) { struct dentry *dentry = F_DENTRY(filp); - const struct file_operations *real_fops = NULL; + const struct file_operations *real_fops; struct file_operations *proxy_fops = NULL; + struct debugfs_fsdata *fsd; int r; r = debugfs_file_get(dentry); if (r) return r == -EIO ? -ENOENT : r; - real_fops = debugfs_real_fops(filp); - + fsd = dentry->d_fsdata; + real_fops = fsd->real_fops; r = debugfs_locked_down(inode, filp, real_fops); if (r) goto out; - if (!fops_get(real_fops)) { + if (real_fops && !fops_get(real_fops)) { #ifdef CONFIG_MODULES if (real_fops->owner && real_fops->owner->state == MODULE_STATE_GOING) { @@ -426,11 +463,14 @@ static int full_proxy_open(struct inode *inode, struct file *filp) r = -ENOMEM; goto free_proxy; } - __full_proxy_fops_init(proxy_fops, real_fops); + __full_proxy_fops_init(proxy_fops, fsd); replace_fops(filp, proxy_fops); - if (real_fops->open) { - r = real_fops->open(inode, filp); + if (!real_fops || real_fops->open) { + if (real_fops) + r = real_fops->open(inode, filp); + else + r = simple_open(inode, filp); if (r) { replace_fops(filp, d_inode(dentry)->i_fop); goto free_proxy; diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c index 66d9b3b4c588..38a9c7eb97e6 100644 --- a/fs/debugfs/inode.c +++ b/fs/debugfs/inode.c @@ -412,7 +412,7 @@ static struct dentry *end_creating(struct dentry *dentry) static struct dentry *__debugfs_create_file(const char *name, umode_t mode, struct dentry *parent, void *data, const struct file_operations *proxy_fops, - const struct file_operations *real_fops) + const void *real_fops) { struct dentry *dentry; struct inode *inode; @@ -450,49 +450,38 @@ static struct dentry *__debugfs_create_file(const char *name, umode_t mode, return end_creating(dentry); } -/** - * debugfs_create_file - create a file in the debugfs filesystem - * @name: a pointer to a string containing the name of the file to create. - * @mode: the permission that the file should have. - * @parent: a pointer to the parent dentry for this file. This should be a - * directory dentry if set. If this parameter is NULL, then the - * file will be created in the root of the debugfs filesystem. - * @data: a pointer to something that the caller will want to get to later - * on. The inode.i_private pointer will point to this value on - * the open() call. - * @fops: a pointer to a struct file_operations that should be used for - * this file. - * - * This is the basic "create a file" function for debugfs. It allows for a - * wide range of flexibility in creating a file, or a directory (if you want - * to create a directory, the debugfs_create_dir() function is - * recommended to be used instead.) - * - * This function will return a pointer to a dentry if it succeeds. This - * pointer must be passed to the debugfs_remove() function when the file is - * to be removed (no automatic cleanup happens if your module is unloaded, - * you are responsible here.) If an error occurs, ERR_PTR(-ERROR) will be - * returned. - * - * If debugfs is not enabled in the kernel, the value -%ENODEV will be - * returned. - * - * NOTE: it's expected that most callers should _ignore_ the errors returned - * by this function. Other debugfs functions handle the fact that the "dentry" - * passed to them could be an error and they don't crash in that case. - * Drivers should generally work fine even if debugfs fails to init anyway. - */ -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); return __debugfs_create_file(name, mode, parent, data, fops ? &debugfs_full_proxy_file_operations : &debugfs_noop_file_operations, fops); } -EXPORT_SYMBOL_GPL(debugfs_create_file); +EXPORT_SYMBOL_GPL(debugfs_create_file_full); + +struct dentry *debugfs_create_file_short(const char *name, umode_t mode, + struct dentry *parent, void *data, + const struct debugfs_short_fops *fops) +{ + if (WARN_ON((unsigned long)fops & + (DEBUGFS_FSDATA_IS_SHORT_FOPS_BIT | + DEBUGFS_FSDATA_IS_REAL_FOPS_BIT))) + return ERR_PTR(-EINVAL); + + return __debugfs_create_file(name, mode, parent, data, + fops ? &debugfs_full_proxy_file_operations : + &debugfs_noop_file_operations, + (const void *)((unsigned long)fops | + DEBUGFS_FSDATA_IS_SHORT_FOPS_BIT)); +} +EXPORT_SYMBOL_GPL(debugfs_create_file_short); /** * debugfs_create_file_unsafe - create a file in the debugfs filesystem diff --git a/fs/debugfs/internal.h b/fs/debugfs/internal.h index dae80c2a469e..a3edfa4f0d8e 100644 --- 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) /* Access BITS */ #define DEBUGFS_ALLOW_API BIT(0) diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h index 0928a6c8ae1e..59444b495d49 100644 --- a/include/linux/debugfs.h +++ b/include/linux/debugfs.h @@ -71,9 +71,63 @@ typedef struct vfsmount *(*debugfs_automount_t)(struct dentry *, void *); struct dentry *debugfs_lookup(const char *name, struct dentry *parent); -struct dentry *debugfs_create_file(const char *name, umode_t mode, - struct dentry *parent, void *data, - const struct file_operations *fops); +struct debugfs_short_fops { + ssize_t (*read)(struct file *, char __user *, size_t, loff_t *); + ssize_t (*write)(struct file *, const char __user *, size_t, loff_t *); + loff_t (*llseek) (struct file *, loff_t, int); +}; + +struct dentry *debugfs_create_file_full(const char *name, umode_t mode, + struct dentry *parent, void *data, + const struct file_operations *fops); +struct dentry *debugfs_create_file_short(const char *name, umode_t mode, + struct dentry *parent, void *data, + const struct debugfs_short_fops *fops); + +/** + * debugfs_create_file - create a file in the debugfs filesystem + * @name: a pointer to a string containing the name of the file to create. + * @mode: the permission that the file should have. + * @parent: a pointer to the parent dentry for this file. This should be a + * directory dentry if set. If this parameter is NULL, then the + * file will be created in the root of the debugfs filesystem. + * @data: a pointer to something that the caller will want to get to later + * on. The inode.i_private pointer will point to this value on + * the open() call. + * @fops: a pointer to a struct file_operations or struct debugfs_short_fops that + * should be used for this file. + * + * This is the basic "create a file" function for debugfs. It allows for a + * wide range of flexibility in creating a file, or a directory (if you want + * to create a directory, the debugfs_create_dir() function is + * recommended to be used instead.) + * + * This function will return a pointer to a dentry if it succeeds. This + * pointer must be passed to the debugfs_remove() function when the file is + * to be removed (no automatic cleanup happens if your module is unloaded, + * you are responsible here.) If an error occurs, ERR_PTR(-ERROR) will be + * returned. + * + * If debugfs is not enabled in the kernel, the value -%ENODEV will be + * returned. + * + * If fops points to a struct debugfs_short_fops, then simple_open() will be + * used for the open, and only read/write/llseek are supported and are proxied, + * so no module reference or release are needed. + * + * NOTE: it's expected that most callers should _ignore_ the errors returned + * by this function. Other debugfs functions handle the fact that the "dentry" + * passed to them could be an error and they don't crash in that case. + * Drivers should generally work fine even if debugfs fails to init anyway. + */ +#define debugfs_create_file(name, mode, parent, data, fops) \ + _Generic(fops, \ + const struct file_operations *: debugfs_create_file_full, \ + const struct debugfs_short_fops *: debugfs_create_file_short, \ + struct file_operations *: debugfs_create_file_full, \ + struct debugfs_short_fops *: debugfs_create_file_short) \ + (name, mode, parent, data, fops) + struct dentry *debugfs_create_file_unsafe(const char *name, umode_t mode, struct dentry *parent, void *data, const struct file_operations *fops); @@ -207,7 +261,7 @@ static inline struct dentry *debugfs_lookup(const char *name, static inline struct dentry *debugfs_create_file(const char *name, umode_t mode, struct dentry *parent, void *data, - const struct file_operations *fops) + const void *fops) { return ERR_PTR(-ENODEV); }