From: Alexey Dobriyan <adobriyan@gmail.com>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v2] proc: faster open/read/close with "permanent" files
Date: Thu, 20 Feb 2020 10:13:00 +0300 [thread overview]
Message-ID: <20200220071300.GA2188@avx2> (raw)
In-Reply-To: <20200219130600.3cb5cd65fbd696fe43fb7adc@linux-foundation.org>
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@gmail.com> 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.
prev parent reply other threads:[~2020-02-20 7:13 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-19 19:11 [PATCH v2] proc: faster open/read/close with "permanent" files Alexey Dobriyan
2020-02-19 21:06 ` Andrew Morton
2020-02-20 7:13 ` Alexey Dobriyan [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200220071300.GA2188@avx2 \
--to=adobriyan@gmail.com \
--cc=akpm@linux-foundation.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).