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

[Bartosz Golaszewski]

2017-12-18 16:07 GMT+01:00 Christophe LEROY <christophe.leroy@xxxxxx>:
>
>
> Le 18/12/2017 Ã 11:18, Bartosz Golaszewski a Ãcrit :
>>
>> 2017-12-18 11:08 GMT+01:00 Christophe Leroy <christophe.leroy@xxxxxx>:
>>>
>>> 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@xxxxxx>
>>> ---
>>>   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@xxxxxxxxxxxxxxx
>>> 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
>>
>