Re: [PATCH v2] proc: faster open/read/close with "permanent" files

[Alexey Dobriyan]

On Wed, Feb 19, 2020 at 01:06:00PM -0800, Andrew Morton wrote:
> On Wed, 19 Feb 2020 22:11:27 +0300 Alexey Dobriyan <adobriyan@xxxxxxxxx> wrote:
> 
> > Now that "struct proc_ops" exist we can start putting there stuff which
> > could not fly with VFS "struct file_operations"...
> > 
> > Most of fs/proc/inode.c file is dedicated to make open/read/.../close reliable
> > in the event of disappearing /proc entries which usually happens if module is
> > getting removed. Files like /proc/cpuinfo which never disappear simply do not
> > need such protection.
> > 
> > Save 2 atomic ops, 1 allocation, 1 free per open/read/close sequence for such
> > "permanent" files.
> > 
> > Enable "permanent" flag for
> > 
> > 	/proc/cpuinfo
> > 	/proc/kmsg
> > 	/proc/modules
> > 	/proc/slabinfo
> > 	/proc/stat
> > 	/proc/sysvipc/*
> > 	/proc/swaps
> > 
> > More will come once I figure out foolproof way to prevent out module
> > authors from marking their stuff "permanent" for performance reasons
> > when it is not.
> > 
> > This should help with scalability: benchmark is "read /proc/cpuinfo R times
> > by N threads scattered over the system".
> > 
> > 	N	R	t, s (before)	t, s (after)
> > 	-----------------------------------------------------
> > 	64	4096	1.582458	1.530502	-3.2%
> > 	256	4096	6.371926	6.125168	-3.9%
> > 	1024	4096	25.64888	24.47528	-4.6%
> 
> I guess that's significant.
> 
> > --- a/fs/proc/internal.h
> > +++ b/fs/proc/internal.h
> > @@ -61,6 +61,7 @@ struct proc_dir_entry {
> >  	struct rb_node subdir_node;
> >  	char *name;
> >  	umode_t mode;
> > +	u8 flags;
> 
> Add a comment describing what this is?
> 
> >  	u8 namelen;
> >  	char inline_name[];
> >  } __randomize_layout;
> >
> > ...
> >
> > @@ -12,7 +13,21 @@ struct proc_dir_entry;
> >  struct seq_file;
> >  struct seq_operations;
> >  
> > +enum {
> > +	/*
> > +	 * All /proc entries using this ->proc_ops instance are never removed.
> > +	 *
> > +	 * If in doubt, ignore this flag.
> > +	 */
> > +#ifdef MODULE
> > +	PROC_ENTRY_PERMANENT = 0U,
> > +#else
> > +	PROC_ENTRY_PERMANENT = 1U << 0,
> > +#endif
> > +};
> 
> That feels quite hacky.  Is it really needed?  Any module which uses
> this is simply buggy?

Without "#ifdef MODULE" -- yes, buggy.
> 
> Can we just leave this undefined if MODULE and break the build?

It is for the case when module is built-in, so module removal won't
happen.

This flag requires discipline. It says that all code working for proc
entry will never be unloaded and /proc entry itself will stay as well.

> >  struct proc_ops {
> > +	unsigned int proc_flags;
> >  	int	(*proc_open)(struct inode *, struct file *);
> >  	ssize_t	(*proc_read)(struct file *, char __user *, size_t, loff_t *);
> >  	ssize_t	(*proc_write)(struct file *, const char __user *, size_t, loff_t *);
> > @@ -25,7 +40,7 @@ struct proc_ops {
> >  #endif
> >  	int	(*proc_mmap)(struct file *, struct vm_area_struct *);
> >  	unsigned long (*proc_get_unmapped_area)(struct file *, unsigned long, unsigned long, unsigned long, unsigned long);
> > -};
> > +} __randomize_layout;
> 
> Unchangelogged, unrelated?

No! Randomization kicks in if all members are pointers to functions,
so once a integer is added it is not randomised anymore.

Or so I've heard...

I'll resend with more comments.