Re: [PATCH] proc: allow to mark /proc files permanent outside of fs/proc/

[Andrew Morton]

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:

	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?

> +void impl_proc_make_permanent(struct proc_dir_entry *pde)
> +{
> +	if (pde) {
> +		pde_make_permanent(pde);
> +	}

Please let's be running checkpatch more often?

> +}
> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> index 96122e91c645..885b1cd38020 100644
> --- 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
> +}
> +
>  #endif /* _LINUX_PROC_FS_H */
> -- 
> 2.47.0