Re: [PATCH v2 2/3] lib kallsyms: parse using io api

[Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>]

Em Fri, May 01, 2020 at 03:13:14PM -0700, Ian Rogers escreveu:
> Perf record will call kallsyms__parse 4 times during startup and process
> megabytes of data. This changes kallsyms__parse to use the io library
> rather than fgets to improve performance of the user code by over 8%.
> 
> Before:
>   Running 'internals/kallsyms-parse' benchmark:
>   Average kallsyms__parse took: 103.988 ms (+- 0.203 ms)
> After:
>   Running 'internals/kallsyms-parse' benchmark:
>   Average kallsyms__parse took: 95.571 ms (+- 0.006 ms)
> 
> For a workload like:
> $ perf record /bin/true
> Run under 'perf record -e cycles:u -g' the time goes from:
> Before
> 30.10%     1.67%  perf     perf                [.] kallsyms__parse
> After
> 25.55%    20.04%  perf     perf                [.] kallsyms__parse
> So a little under 5% of the start-up time is removed. A lot of what
> remains is on the kernel side, but caching kallsyms within perf would
> at least impact memory footprint.

Applied and added this to the commit log:

Committer notes:

The internal/kallsyms-parse bench is run using:

  [root@five ~]# perf bench internals kallsyms-parse
  # Running 'internals/kallsyms-parse' benchmark:
    Average kallsyms__parse took: 80.381 ms (+- 0.115 ms)
  [root@five ~]#

And this pre-existing test uses these routines to parse kallsyms and
then compare with the info obtained from the matching ELF symtab:

  [root@five ~]# perf test vmlinux
   1: vmlinux symtab matches kallsyms                       : Ok
  [root@five ~]#

Signed-off-by: Ian Rogers <irogers@google.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/lib/api/io.h          |  3 ++
>  tools/lib/symbol/kallsyms.c | 86 ++++++++++++++++---------------------
>  2 files changed, 41 insertions(+), 48 deletions(-)
> 
> diff --git a/tools/lib/api/io.h b/tools/lib/api/io.h
> index b7e55b5f8a4a..777c20f6b604 100644
> --- a/tools/lib/api/io.h
> +++ b/tools/lib/api/io.h
> @@ -7,6 +7,9 @@
>  #ifndef __API_IO__
>  #define __API_IO__
>  
> +#include <stdlib.h>
> +#include <unistd.h>
> +
>  struct io {
>  	/* File descriptor being read/ */
>  	int fd;
> diff --git a/tools/lib/symbol/kallsyms.c b/tools/lib/symbol/kallsyms.c
> index 1a7a9f877095..e335ac2b9e19 100644
> --- a/tools/lib/symbol/kallsyms.c
> +++ b/tools/lib/symbol/kallsyms.c
> @@ -1,7 +1,9 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include "symbol/kallsyms.h"
> +#include "api/io.h"
>  #include <stdio.h>
> -#include <stdlib.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
>  
>  u8 kallsyms2elf_type(char type)
>  {
> @@ -15,74 +17,62 @@ bool kallsyms__is_function(char symbol_type)
>  	return symbol_type == 'T' || symbol_type == 'W';
>  }
>  
> -/*
> - * While we find nice hex chars, build a long_val.
> - * Return number of chars processed.
> - */
> -int hex2u64(const char *ptr, u64 *long_val)
> +static void read_to_eol(struct io *io)
>  {
> -	char *p;
> +	int ch;
>  
> -	*long_val = strtoull(ptr, &p, 16);
> -
> -	return p - ptr;
> +	for (;;) {
> +		ch = io__get_char(io);
> +		if (ch < 0 || ch == '\n')
> +			return;
> +	}
>  }
>  
>  int kallsyms__parse(const char *filename, void *arg,
>  		    int (*process_symbol)(void *arg, const char *name,
>  					  char type, u64 start))
>  {
> -	char *line = NULL;
> -	size_t n;
> -	int err = -1;
> -	FILE *file = fopen(filename, "r");
> -
> -	if (file == NULL)
> -		goto out_failure;
> -
> -	err = 0;
> +	struct io io;
> +	char bf[BUFSIZ];
> +	int err;
>  
> -	while (!feof(file)) {
> -		u64 start;
> -		int line_len, len;
> -		char symbol_type;
> -		char *symbol_name;
> +	io.fd = open(filename, O_RDONLY, 0);
>  
> -		line_len = getline(&line, &n, file);
> -		if (line_len < 0 || !line)
> -			break;
> +	if (io.fd < 0)
> +		return -1;
>  
> -		line[--line_len] = '\0'; /* \n */
> +	io__init(&io, io.fd, bf, sizeof(bf));
>  
> -		len = hex2u64(line, &start);
> +	err = 0;
> +	while (!io.eof) {
> +		__u64 start;
> +		int ch;
> +		size_t i;
> +		char symbol_type;
> +		char symbol_name[KSYM_NAME_LEN + 1];
>  
> -		/* Skip the line if we failed to parse the address. */
> -		if (!len)
> +		if (io__get_hex(&io, &start) != ' ') {
> +			read_to_eol(&io);
>  			continue;
> -
> -		len++;
> -		if (len + 2 >= line_len)
> +		}
> +		symbol_type = io__get_char(&io);
> +		if (io__get_char(&io) != ' ') {
> +			read_to_eol(&io);
>  			continue;
> -
> -		symbol_type = line[len];
> -		len += 2;
> -		symbol_name = line + len;
> -		len = line_len - len;
> -
> -		if (len >= KSYM_NAME_LEN) {
> -			err = -1;
> -			break;
>  		}
> +		for (i = 0; i < sizeof(symbol_name); i++) {
> +			ch = io__get_char(&io);
> +			if (ch < 0 || ch == '\n')
> +				break;
> +			symbol_name[i]  = ch;
> +		}
> +		symbol_name[i]  = '\0';
>  
>  		err = process_symbol(arg, symbol_name, symbol_type, start);
>  		if (err)
>  			break;
>  	}
>  
> -	free(line);
> -	fclose(file);
> +	close(io.fd);
>  	return err;
> -
> -out_failure:
> -	return -1;
>  }
> -- 
> 2.26.2.526.g744177e7f7-goog
> 

-- 

- Arnaldo