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=McJVH1c945oCSNbn7qycy8pX4w4mkUqm0tOdBEdR7z25g@mail.gmail.com>
Date:   Mon, 18 Dec 2017 11:18:23 +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 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...

The character device doesn't deal with strings so it will be much faster anyway.

Best regards,
Bartosz Golaszewski