On Wed, Apr 09, 2025 at 02:35:46PM -0700, Andrew Morton wrote:
> On Wed, 9 Apr 2025 22:20:13 +0300 Alexey Dobriyan <adobriyan@xxxxxxxxx> wrote:
>
> > >From 06e2ff406942fef65b9c397a7f44478dd4b61451 Mon Sep 17 00:00:00 2001
> > From: Alexey Dobriyan <adobriyan@xxxxxxxxx>
> > Date: Sat, 5 Apr 2025 14:50:10 +0300
> > Subject: [PATCH 1/1] proc: allow to mark /proc files permanent outside of
> > fs/proc/
> >
> > From: Mateusz Guzik <mjguzik@xxxxxxxxx>
> >
> > Add proc_make_permanent() function to mark PDE as permanent to speed up
> > open/read/close (one alloc/free and lock/unlock less).
>
> When proposing a speedup it is preferable to provide some benchmarking
> results to help others understand the magnitude of that speedup.
>
> > ...
> >
> > index 58b9067b2391..81dcd0ddadb6 100644
> > --- a/fs/filesystems.c
> > +++ b/fs/filesystems.c
> > @@ -252,7 +252,9 @@ static int filesystems_proc_show(struct seq_file *m, void *v)
> >
> > static int __init proc_filesystems_init(void)
> > {
> > - proc_create_single("filesystems", 0, NULL, filesystems_proc_show);
> > + struct proc_dir_entry *pde =
> > + proc_create_single("filesystems", 0, NULL, filesystems_proc_show);
>
> To avoid the 80-column nasties, this is more pleasing:
It is inferior style, see below. BTW, how the kernel is still on 80 columns?
> struct proc_dir_entry *pde;
>
> pde = proc_create_single("filesystems", 0, NULL, filesystems_proc_show);
>
>
> > + proc_make_permanent(pde);
> > return 0;
> > }
> > module_init(proc_filesystems_init);
> > diff --git a/fs/proc/generic.c b/fs/proc/generic.c
> > index a3e22803cddf..0342600c0172 100644
> > --- a/fs/proc/generic.c
> > +++ b/fs/proc/generic.c
> > @@ -826,3 +826,15 @@ ssize_t proc_simple_write(struct file *f, const char __user *ubuf, size_t size,
> > kfree(buf);
> > return ret == 0 ? size : ret;
> > }
> > +
> > +/*
> > + * Not exported to modules:
> > + * modules' /proc files aren't permanent because modules aren't permanent.
> > + */
> > +void impl_proc_make_permanent(struct proc_dir_entry *pde);
>
> This declaration is unneeded, isn't it?
It is necessary, but I need to make a comment, yes.
> > +void impl_proc_make_permanent(struct proc_dir_entry *pde)
> > +{
> > + if (pde) {
> > + pde_make_permanent(pde);
> > + }
>
> Please let's be running checkpatch more often?
No! I'd rather change kernel coding style.
> > --- a/fs/proc/internal.h
> > +++ b/fs/proc/internal.h
> > @@ -80,8 +80,11 @@ static inline bool pde_is_permanent(const struct proc_dir_entry *pde)
> > return pde->flags & PROC_ENTRY_PERMANENT;
> > }
> >
> > +/* This is for builtin code, not even for modules which are compiled in. */
> > static inline void pde_make_permanent(struct proc_dir_entry *pde)
> > {
> > + /* Ensure magic flag does something. */
> > + static_assert(PROC_ENTRY_PERMANENT != 0);
>
> Looks odd. What is this doing? The comment does a poor job of
> explaining this!
>
> > pde->flags |= PROC_ENTRY_PERMANENT;
> > }
> >
> > diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
> > index ea62201c74c4..2d59f29b49eb 100644
> > --- a/include/linux/proc_fs.h
> > +++ b/include/linux/proc_fs.h
> > @@ -247,4 +247,14 @@ static inline struct pid_namespace *proc_pid_ns(struct super_block *sb)
> >
> > bool proc_ns_file(const struct file *file);
> >
> > +static inline void proc_make_permanent(struct proc_dir_entry *pde)
> > +{
> > + /* Don't give matches to modules. */
>
> This comment is also mysterious (to me). Please expand upon it.
>
> > +#if defined CONFIG_PROC_FS && !defined MODULE
> > + /* This mess is created by defining "struct proc_dir_entry" elsewhere. */
>
> Also mysterious.
>
> > + void impl_proc_make_permanent(struct proc_dir_entry *pde);
>
> Forward-declaring a function within a function in this manner is quite
> unusual. Let's be conventional, please.
>
> > + impl_proc_make_permanent(pde);
> > +#endif
> > +}
This patch tries to create semi-exported interface which does nothing
if module built as module is using it it but does something it built-in or
module built-in does. This is why all complications and prorotypes.
But let me change coding style first.
