lkml.org 
[lkml]   [2020]   [Feb]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2] proc: faster open/read/close with "permanent" files
    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.

    \
     
     \ /
      Last update: 2020-02-20 08:13    [W:2.830 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site