Re: [PATCH 5/5] perf tools: Transform nodes string info to struct

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Tue, Jun 28, 2016 at 01:29:05PM +0200, Jiri Olsa escreveu:
> Storing NUMA info within struct numa_node instead
> of strings. This way it's usable in future patches.
> 
> Also it turned out it's slightly less code involved
> than using strings.
> 
> Link: http://lkml.kernel.org/n/tip-ka37sax3gfaxwvytfxi0ycy1@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/env.c    |  5 +++-
>  tools/perf/util/env.h    | 12 ++++++--
>  tools/perf/util/header.c | 78 +++++++++++++++++-------------------------------
>  3 files changed, 41 insertions(+), 54 deletions(-)
> 
> diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
> index 49a11d9d8b8f..153985977475 100644
> --- a/tools/perf/util/env.c
> +++ b/tools/perf/util/env.c
> @@ -18,10 +18,13 @@ void perf_env__exit(struct perf_env *env)
>  	zfree(&env->cmdline_argv);
>  	zfree(&env->sibling_cores);
>  	zfree(&env->sibling_threads);
> -	zfree(&env->numa_nodes);
>  	zfree(&env->pmu_mappings);
>  	zfree(&env->cpu);
>  
> +	for (i = 0; i < env->numa_nodes_cnt; i++)
> +		cpu_map__put(env->numa_nodes[i].map);
> +	zfree(&env->numa_nodes);
> +
>  	for (i = 0; i < env->caches_cnt; i++)
>  		cpu_cache_level__free(&env->caches[i]);
>  	zfree(&env->caches);
> diff --git a/tools/perf/util/env.h b/tools/perf/util/env.h
> index 56cffb60a0b4..027d0f4e6bff 100644
> --- a/tools/perf/util/env.h
> +++ b/tools/perf/util/env.h
> @@ -2,6 +2,7 @@
>  #define __PERF_ENV_H
>  
>  #include <linux/types.h>
> +#include "cpumap.h"
>  
>  struct cpu_topology_map {
>  	int	socket_id;
> @@ -18,6 +19,13 @@ struct cpu_cache_level {
>  	char	*map;
>  };
>  
> +struct numa_node {
> +	u32		 node;
> +	u64		 mem_total;
> +	u64		 mem_free;
> +	struct cpu_map	*map;
> +};
> +
>  struct perf_env {
>  	char			*hostname;
>  	char			*os_release;
> @@ -33,18 +41,18 @@ struct perf_env {
>  	int			nr_cmdline;
>  	int			nr_sibling_cores;
>  	int			nr_sibling_threads;
> -	int			nr_numa_nodes;

why rename it from nr_numa_nodes to numa_nodes_cnt? Seems gratuitous and
potentially introduces up to three 4 byte holes into 'struct perf_env'
:-\

Applied the other patches in this series,

Thanks,

- Arnaldo

>  	int			nr_pmu_mappings;
>  	int			nr_groups;
>  	char			*cmdline;
>  	const char		**cmdline_argv;
>  	char			*sibling_cores;
>  	char			*sibling_threads;
> -	char			*numa_nodes;
>  	char			*pmu_mappings;
>  	struct cpu_topology_map	*cpu;
>  	struct cpu_cache_level	*caches;
>  	int			 caches_cnt;
> +	struct numa_node	*numa_nodes;
> +	int			 numa_nodes_cnt;
>  };
>  
>  extern struct perf_env perf_env;
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index c5cd2698281f..af5f275462b3 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -1306,42 +1306,19 @@ static void print_total_mem(struct perf_header *ph, int fd __maybe_unused,
>  static void print_numa_topology(struct perf_header *ph, int fd __maybe_unused,
>  				FILE *fp)
>  {
> -	u32 nr, c, i;
> -	char *str, *tmp;
> -	uint64_t mem_total, mem_free;
> -
> -	/* nr nodes */
> -	nr = ph->env.nr_numa_nodes;
> -	str = ph->env.numa_nodes;
> -
> -	for (i = 0; i < nr; i++) {
> -		/* node number */
> -		c = strtoul(str, &tmp, 0);
> -		if (*tmp != ':')
> -			goto error;
> -
> -		str = tmp + 1;
> -		mem_total = strtoull(str, &tmp, 0);
> -		if (*tmp != ':')
> -			goto error;
> +	int i;
> +	struct numa_node *n;
>  
> -		str = tmp + 1;
> -		mem_free = strtoull(str, &tmp, 0);
> -		if (*tmp != ':')
> -			goto error;
> +	for (i = 0; i < ph->env.numa_nodes_cnt; i++) {
> +		n = &ph->env.numa_nodes[i];
>  
>  		fprintf(fp, "# node%u meminfo  : total = %"PRIu64" kB,"
>  			    " free = %"PRIu64" kB\n",
> -			c, mem_total, mem_free);
> +			n->node, n->mem_total, n->mem_free);
>  
> -		str = tmp + 1;
> -		fprintf(fp, "# node%u cpu list : %s\n", c, str);
> -
> -		str += strlen(str) + 1;
> +		fprintf(fp, "# node%u cpu list : ", n->node);
> +		cpu_map__fprintf(n->map, fp);
>  	}
> -	return;
> -error:
> -	fprintf(fp, "# numa topology : not available\n");
>  }
>  
>  static void print_cpuid(struct perf_header *ph, int fd __maybe_unused, FILE *fp)
> @@ -1906,11 +1883,10 @@ static int process_numa_topology(struct perf_file_section *section __maybe_unuse
>  				 struct perf_header *ph, int fd,
>  				 void *data __maybe_unused)
>  {
> +	struct numa_node *nodes, *n;
>  	ssize_t ret;
> -	u32 nr, node, i;
> +	u32 nr, i;
>  	char *str;
> -	uint64_t mem_total, mem_free;
> -	struct strbuf sb;
>  
>  	/* nr nodes */
>  	ret = readn(fd, &nr, sizeof(nr));
> @@ -1920,48 +1896,48 @@ static int process_numa_topology(struct perf_file_section *section __maybe_unuse
>  	if (ph->needs_swap)
>  		nr = bswap_32(nr);
>  
> -	ph->env.nr_numa_nodes = nr;
> -	if (strbuf_init(&sb, 256) < 0)
> -		return -1;
> +	ph->env.numa_nodes_cnt = nr;
> +	nodes = zalloc(sizeof(*nodes) * nr);
> +	if (!nodes)
> +		return -ENOMEM;
>  
>  	for (i = 0; i < nr; i++) {
> +		n = &nodes[i];
> +
>  		/* node number */
> -		ret = readn(fd, &node, sizeof(node));
> -		if (ret != sizeof(node))
> +		ret = readn(fd, &n->node, sizeof(u32));
> +		if (ret != sizeof(n->node))
>  			goto error;
>  
> -		ret = readn(fd, &mem_total, sizeof(u64));
> +		ret = readn(fd, &n->mem_total, sizeof(u64));
>  		if (ret != sizeof(u64))
>  			goto error;
>  
> -		ret = readn(fd, &mem_free, sizeof(u64));
> +		ret = readn(fd, &n->mem_free, sizeof(u64));
>  		if (ret != sizeof(u64))
>  			goto error;
>  
>  		if (ph->needs_swap) {
> -			node = bswap_32(node);
> -			mem_total = bswap_64(mem_total);
> -			mem_free = bswap_64(mem_free);
> +			n->node      = bswap_32(n->node);
> +			n->mem_total = bswap_64(n->mem_total);
> +			n->mem_free  = bswap_64(n->mem_free);
>  		}
>  
> -		if (strbuf_addf(&sb, "%u:%"PRIu64":%"PRIu64":",
> -				node, mem_total, mem_free) < 0)
> -			goto error;
> -
>  		str = do_read_string(fd, ph);
>  		if (!str)
>  			goto error;
>  
> -		/* include a NULL character at the end */
> -		if (strbuf_add(&sb, str, strlen(str) + 1) < 0)
> +		n->map = cpu_map__new(str);
> +		if (!n->map)
>  			goto error;
> +
>  		free(str);
>  	}
> -	ph->env.numa_nodes = strbuf_detach(&sb, NULL);
> +	ph->env.numa_nodes = nodes;
>  	return 0;
>  
>  error:
> -	strbuf_release(&sb);
> +	free(nodes);
>  	return -1;
>  }
>  
> -- 
> 2.4.11