Hi Johannes,
On Mon, Nov 25, 2024 at 1:37 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> 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
> > --- 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@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
