| 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
| ||
|
Date: Thu, 23 May 2024 16:25:25 -0700
From: Namhyung Kim <namhyung@...nel.org>
To: Ian Rogers <irogers@...gle.com>
Cc: Arnaldo Carvalho de Melo <acme@...hat.com>, linux-kernel@...r.kernel.org,
linux-perf-users@...r.kernel.org
Subject: Re: [PATCH v1] tools api io: Move filling the io buffer to its own function
On Sun, May 19, 2024 at 11:17 AM Ian Rogers <irogers@...gle.com> wrote:
>
> In general a read fills 4kb so filling the buffer is a 1 in 4096
> operation, move it out of the io__get_char function to avoid some
> checking overhead and to better hint the function is good to inline.
>
> For perf's IO intensive internal (non-rigorous) benchmarks there's a
> near 8% improvement to kallsyms-parsing with a default build.
Oh, is it just from removing the io->eof check? Otherwise I don't
see any difference.
Thanks,
Namhyung
>
> Before:
> ```
> $ perf bench internals all
> Computing performance of single threaded perf event synthesis by
> synthesizing events on the perf process itself:
> Average synthesis took: 146.322 usec (+- 0.305 usec)
> Average num. events: 61.000 (+- 0.000)
> Average time per event 2.399 usec
> Average data synthesis took: 145.056 usec (+- 0.155 usec)
> Average num. events: 329.000 (+- 0.000)
> Average time per event 0.441 usec
>
> Average kallsyms__parse took: 162.313 ms (+- 0.599 ms)
> ...
> Computing performance of sysfs PMU event scan for 100 times
> Average core PMU scanning took: 53.720 usec (+- 7.823 usec)
> Average PMU scanning took: 375.145 usec (+- 23.974 usec)
> ```
> After:
> ```
> $ perf bench internals all
> Computing performance of single threaded perf event synthesis by
> synthesizing events on the perf process itself:
> Average synthesis took: 127.829 usec (+- 0.079 usec)
> Average num. events: 61.000 (+- 0.000)
> Average time per event 2.096 usec
> Average data synthesis took: 133.652 usec (+- 0.101 usec)
> Average num. events: 327.000 (+- 0.000)
> Average time per event 0.409 usec
>
> Average kallsyms__parse took: 150.415 ms (+- 0.313 ms)
> ...
> Computing performance of sysfs PMU event scan for 100 times
> Average core PMU scanning took: 47.790 usec (+- 1.178 usec)
> Average PMU scanning took: 376.945 usec (+- 23.683 usec)
> ```
>
> Signed-off-by: Ian Rogers <irogers@...gle.com>
> ---
> tools/lib/api/io.h | 69 +++++++++++++++++++++++++---------------------
> 1 file changed, 38 insertions(+), 31 deletions(-)
>
> diff --git a/tools/lib/api/io.h b/tools/lib/api/io.h
> index 84adf8102018..d3eb04d1bc89 100644
> --- a/tools/lib/api/io.h
> +++ b/tools/lib/api/io.h
> @@ -43,48 +43,55 @@ static inline void io__init(struct io *io, int fd,
> io->eof = false;
> }
>
> -/* Reads one character from the "io" file with similar semantics to fgetc. */
> -static inline int io__get_char(struct io *io)
> +/* Read from fd filling the buffer. Called when io->data == io->end. */
> +static inline int io__fill_buffer(struct io *io)
> {
> - char *ptr = io->data;
> + ssize_t n;
>
> if (io->eof)
> return -1;
>
> - if (ptr == io->end) {
> - ssize_t n;
> -
> - if (io->timeout_ms != 0) {
> - struct pollfd pfds[] = {
> - {
> - .fd = io->fd,
> - .events = POLLIN,
> - },
> - };
> -
> - n = poll(pfds, 1, io->timeout_ms);
> - if (n == 0)
> - errno = ETIMEDOUT;
> - if (n > 0 && !(pfds[0].revents & POLLIN)) {
> - errno = EIO;
> - n = -1;
> - }
> - if (n <= 0) {
> - io->eof = true;
> - return -1;
> - }
> + if (io->timeout_ms != 0) {
> + struct pollfd pfds[] = {
> + {
> + .fd = io->fd,
> + .events = POLLIN,
> + },
> + };
> +
> + n = poll(pfds, 1, io->timeout_ms);
> + if (n == 0)
> + errno = ETIMEDOUT;
> + if (n > 0 && !(pfds[0].revents & POLLIN)) {
> + errno = EIO;
> + n = -1;
> }
> - n = read(io->fd, io->buf, io->buf_len);
> -
> if (n <= 0) {
> io->eof = true;
> return -1;
> }
> - ptr = &io->buf[0];
> - io->end = &io->buf[n];
> }
> - io->data = ptr + 1;
> - return *ptr;
> + n = read(io->fd, io->buf, io->buf_len);
> +
> + if (n <= 0) {
> + io->eof = true;
> + return -1;
> + }
> + io->data = &io->buf[0];
> + io->end = &io->buf[n];
> + return 0;
> +}
> +
> +/* Reads one character from the "io" file with similar semantics to fgetc. */
> +static inline int io__get_char(struct io *io)
> +{
> + if (io->data == io->end) {
> + int ret = io__fill_buffer(io);
> +
> + if (ret)
> + return ret;
> + }
> + return *io->data++;
> }
>
> /* Read a hexadecimal value with no 0x prefix into the out argument hex. If the
> --
> 2.45.0.rc1.225.g2a3ae87e7f-goog
>
Powered by blists - more mailing lists