sysfs_kf_seq_show() spends 87% of time in memset()

sysfs_kf_seq_show() spends 87% of time in memset()

[Christophe LEROY]

Hello Tejun,

Doing a 'perf record' on an application using GPIOs a lot, I discovered 
that most of the time spent in the read() system call of the 'value' 
sysfs file of that GPIO (which returns "0\n" or "1\n") is indeed spent 
in memset() zeroing a buffer of size PAGE_SIZE for a 2 bytes read:


  --1.95%--ret_from_syscall
            sys_read
            |
             --1.93%--vfs_read
                       |
                        --1.89%--__vfs_read
                                  |
                                   --1.86%--seq_read
                                             |
                                              --1.68%--sysfs_kf_seq_show
                                                        |
                                                         --1.46%--memset

As far as I can see, that memset() was introduced by your commit 
f5c16f29bf5e5 ("sysfs: make sure read buffer is zeroed")

Is that really necessary, taking into account that the ->show will 
overwrite it ?

Thanks
Christophe

Re: sysfs_kf_seq_show() spends 87% of time in memset()

[Tejun Heo]

Hello,

On Fri, Dec 15, 2017 at 01:59:55PM +0100, Christophe LEROY wrote:
> Doing a 'perf record' on an application using GPIOs a lot, I
> discovered that most of the time spent in the read() system call of
> the 'value' sysfs file of that GPIO (which returns "0\n" or "1\n")
> is indeed spent in memset() zeroing a buffer of size PAGE_SIZE for a
> 2 bytes read:
...
> As far as I can see, that memset() was introduced by your commit
> f5c16f29bf5e5 ("sysfs: make sure read buffer is zeroed")
> 
> Is that really necessary, taking into account that the ->show will
> overwrite it ?

We were doing get_zeroed_pages() before, so the commit is restoring
the previous behavior, and, yeah, I think there's some risk of leaking
uninitialized data.  The way the ->show() interface is defined doesn't
give us a lot of leeway.  The right thing to do would be switching the
said driver to prealloc read or ->bin_read.

Thanks.

-- 
tejun