linux-kernel - Re: [PATCH 1/4] gpio: sysfs: change 'value' attribute to prealloc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMRc=McjSUEMxZmaU+FZVzXivQydkMyF_cW1_d4e7mfX=KM3qg@mail.gmail.com>
Date:   Mon, 18 Dec 2017 18:40:14 +0100
From:   Bartosz Golaszewski <brgl@...ev.pl>
To:     Christophe LEROY <christophe.leroy@....fr>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-gpio@...r.kernel.org
Subject: Re: [PATCH 1/4] gpio: sysfs: change 'value' attribute to prealloc

2017-12-18 16:07 GMT+01:00 Christophe LEROY <christophe.leroy@....fr>:
>
>
> Le 18/12/2017 à 11:18, Bartosz Golaszewski a écrit :
>>
>> 2017-12-18 11:08 GMT+01:00 Christophe Leroy <christophe.leroy@....fr>:
>>>
>>> The GPIO 'value' attribute is time critical. A small bench with
>>> 'perf record' on the app below shows that 80% of the time spent in
>>> sysfs_kf_seq_show() is spent in memset() for zeroising the buffer.
>>>
>>> |--67.48%--sysfs_kf_seq_show
>>> |          |
>>> |          |--54.40%--memset
>>> |          |
>>> |          |--11.49%--dev_attr_show
>>> |          |          |
>>> |          |          |--10.06%--value_show
>>> |          |          |          |
>>> |          |          |          |--4.75%--sprintf
>>> |          |          |          |          |
>>>
>>> This patch changes the attribute type to prealloc, eliminating the
>>> need to zeroise the buffer at each read. 'perf record' gives the
>>> following result.
>>>
>>> |--42.41%--sysfs_kf_read
>>> |          |
>>> |          |--39.73%--dev_attr_show
>>> |          |          |
>>> |          |          |--38.23%--value_show
>>> |          |          |          |
>>> |          |          |          |--29.22%--sprintf
>>> |          |          |          |          |
>>>
>>> Test done with the following small app:
>>>
>>> int main(int argc, char **argv)
>>> {
>>>          int fd = open(argv[1], O_RDONLY);
>>>
>>>          for (;;) {
>>>                  int buf[512];
>>>
>>>                  read(fd, buf, 512);
>>>                  lseek(fd, 0, SEEK_SET);
>>>          }
>>>          exit(0);
>>> }
>>>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@....fr>
>>> ---
>>>   drivers/gpio/gpiolib-sysfs.c | 2 +-
>>>   include/linux/device.h       | 3 +++
>>>   2 files changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
>>> index 3f454eaf2101..7a3f4271393b 100644
>>> --- a/drivers/gpio/gpiolib-sysfs.c
>>> +++ b/drivers/gpio/gpiolib-sysfs.c
>>> @@ -138,7 +138,7 @@ static ssize_t value_store(struct device *dev,
>>>
>>>          return status;
>>>   }
>>> -static DEVICE_ATTR_RW(value);
>>> +static DEVICE_ATTR_PREALLOC(value, S_IWUSR | S_IRUGO, value_show,
>>> value_store);
>>>
>>>   static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
>>>   {
>>> diff --git a/include/linux/device.h b/include/linux/device.h
>>> index 9d32000725da..46ac622e5c6f 100644
>>> --- a/include/linux/device.h
>>> +++ b/include/linux/device.h
>>> @@ -575,6 +575,9 @@ ssize_t device_store_bool(struct device *dev, struct
>>> device_attribute *attr,
>>>
>>>   #define DEVICE_ATTR(_name, _mode, _show, _store) \
>>>          struct device_attribute dev_attr_##_name = __ATTR(_name, _mode,
>>> _show, _store)
>>> +#define DEVICE_ATTR_PREALLOC(_name, _mode, _show, _store) \
>>> +       struct device_attribute dev_attr_##_name = \
>>> +               __ATTR_PREALLOC(_name, _mode, _show, _store)
>>>   #define DEVICE_ATTR_RW(_name) \
>>>          struct device_attribute dev_attr_##_name = __ATTR_RW(_name)
>>>   #define DEVICE_ATTR_RO(_name) \
>>> --
>>> 2.13.3
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
>>> the body of a message to majordomo@...r.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>> I'm not sure we should be fixing performance issues in an interface
>> that's deprecated anyway...
>
>
> Many applications use that interface. I agree not to make deep changes for
> performance, but here the few changes are quite tiny, yet for a significant
> improvement, so aren't they worth it anyway ?
>
> Christophe

Well, TBH if anything, I'd prefer to worsen the performance to
actually discourage people from using sysfs for GPIOs. :)

It's not up to me though, so let's wait for Linus' opinion.

Thanks,
Bartosz

>
>
>>
>> The character device doesn't deal with strings so it will be much faster
>> anyway.
>>
>> Best regards,
>> Bartosz Golaszewski
>>
>