Re: [PATCH 08/19] perf tools: Add mem2node object

[Arnaldo Carvalho de Melo]

Em Wed, Mar 07, 2018 at 04:50:09PM +0100, Jiri Olsa escreveu:
> Adding mem2node object to allow the easy lookup
> of the node for the physical address.
> 
> It has following interface:
> 
>   int  mem2node__init(struct mem2node *map, struct perf_env *env);
>   void mem2node__exit(struct mem2node *map);
>   int  mem2node__node(struct mem2node *map, u64 addr);
> 
> The mem2node__init initialize object from the perf data
> file MEM_TOPOLOGY feature data. Following calls to
> mem2node__node will return node number for given
> physical address. The mem2node__exit function frees
> the object.
> 
> Link: http://lkml.kernel.org/n/tip-qq7sohu774wxq154n3my037z@xxxxxxxxxxxxxx
> Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> ---
>  tools/perf/util/Build      |   1 +
>  tools/perf/util/mem2node.c | 129 +++++++++++++++++++++++++++++++++++++++++++++
>  tools/perf/util/mem2node.h |  19 +++++++
>  3 files changed, 149 insertions(+)
>  create mode 100644 tools/perf/util/mem2node.c
>  create mode 100644 tools/perf/util/mem2node.h
> 
> diff --git a/tools/perf/util/Build b/tools/perf/util/Build
> index ea0a452550b0..8052373bcd6a 100644
> --- a/tools/perf/util/Build
> +++ b/tools/perf/util/Build
> @@ -106,6 +106,7 @@ libperf-y += units.o
>  libperf-y += time-utils.o
>  libperf-y += expr-bison.o
>  libperf-y += branch.o
> +libperf-y += mem2node.o
>  
>  libperf-$(CONFIG_LIBBPF) += bpf-loader.o
>  libperf-$(CONFIG_BPF_PROLOGUE) += bpf-prologue.o
> diff --git a/tools/perf/util/mem2node.c b/tools/perf/util/mem2node.c
> new file mode 100644
> index 000000000000..6da8ddbb1182
> --- /dev/null
> +++ b/tools/perf/util/mem2node.c
> @@ -0,0 +1,129 @@
> +#include <errno.h>
> +#include <inttypes.h>
> +#include <linux/bitmap.h>
> +#include "mem2node.h"
> +#include "util.h"
> +
> +struct entry {
> +	struct rb_node	rb_node;
> +	u64	start;
> +	u64	end;
> +	u64	node;
> +};

Hey, this name is way too generic, please rename it to something more
descriptive

> +
> +static void entry__insert(struct entry *entry, struct rb_root *root)
> +{
> +	struct rb_node **p = &root->rb_node;
> +	struct rb_node *parent = NULL;
> +	struct entry *e;
> +
> +	while (*p != NULL) {
> +		parent = *p;
> +		e = rb_entry(parent, struct entry, rb_node);
> +
> +		if (entry->start < e->start)
> +			p = &(*p)->rb_left;
> +		else
> +			p = &(*p)->rb_right;
> +	}
> +
> +	rb_link_node(&entry->rb_node, parent, p);
> +	rb_insert_color(&entry->rb_node, root);
> +}
> +
> +int mem2node__init(struct mem2node *map, struct perf_env *env)
> +{
> +	struct memory_node *n, *nodes = &env->memory_nodes[0];
> +	u64 bsize = env->memory_bsize;
> +	struct entry *entry;
> +	int i, j = 0, max = 0;
> +
> +	memset(map, 0x0, sizeof(*map));
> +	map->root = RB_ROOT;
> +
> +	for (i = 0; i < env->nr_memory_nodes; i++) {
> +		n = &nodes[i];
> +		max += bitmap_weight(n->set, n->size);
> +	}
> +
> +	entry = zalloc(sizeof(*entry) * max);
> +	if (!entry)
> +		return -ENOMEM;

Also this is not an 'entry', but max ones, please rename this variable
to 'entries'.

> +
> +	for (i = 0; i < env->nr_memory_nodes; i++) {
> +		u64 bit;
> +
> +		n = &nodes[i];
> +
> +		for (bit = 0; bit < n->size; bit++) {
> +			u64 start;
> +
> +			if (!test_bit(bit, n->set))
> +				continue;
> +
> +			start = bit * bsize;
> +
> +			/*
> +			 * Merge nearby areas, we walk in order
> +			 * through the bitmap, so no need to sort.
> +			 */
> +			if (j > 0) {
> +				struct entry *prev = &entry[j - 1];
> +
> +				if ((prev->end == start) &&
> +				    (prev->node == n->node)) {
> +					prev->end += bsize;
> +					continue;
> +				}
> +			}
> +
> +			entry[j].start = start;
> +			entry[j].end   = start + bsize;
> +			entry[j].node  = n->node;
> +			RB_CLEAR_NODE(&entry[j].rb_node);

The previous four line could be done via the usual:

			krava_entry__init(&entries[j], start, bsize, n->node);

> +			j++;
> +		}
> +	}
> +
> +	/* Cut unused entries, due to merging. */
> +	entry = realloc(entry, sizeof(*entry) * j);
> +	if (!entry)
> +		return -ENOMEM;


Humm, so you lose it when not able to cut it short? Why not:

	nentries = realloc(entries, sizeof(entries[0) * j);
	if (nentries)
		entries = nentries;

And if you couldn't cut it (however unlikely that is) just go on and use
what you have already.

> +
> +	for (i = 0; i < j; i++) {
> +		pr_debug("mem2node %03" PRIu64 " [0x%016" PRIx64 "-0x%016" PRIx64 "]\n",
> +			 entry[i].node, entry[i].start, entry[i].end);
> +
> +		entry__insert(&entry[i], &map->root);
> +	}
> +
> +	map->entry = entry;
> +	return 0;
> +}
> +
> +void mem2node__exit(struct mem2node *map)
> +{
> +	zfree(&map->entry);
> +}
> +
> +int mem2node__node(struct mem2node *map, u64 addr)
> +{
> +	struct rb_node **p, *parent = NULL;
> +	struct entry *entry;
> +
> +	p = &map->root.rb_node;
> +	while (*p != NULL) {
> +		parent = *p;
> +		entry = rb_entry(parent, struct entry, rb_node);
> +		if (addr < entry->start)
> +			p = &(*p)->rb_left;
> +		else if (addr >= entry->end)
> +			p = &(*p)->rb_right;
> +		else
> +			goto out;
> +	}
> +
> +	entry = NULL;
> +out:
> +	return entry ? (int) entry->node : -1;
> +}
> diff --git a/tools/perf/util/mem2node.h b/tools/perf/util/mem2node.h
> new file mode 100644
> index 000000000000..e27333755a2c
> --- /dev/null
> +++ b/tools/perf/util/mem2node.h
> @@ -0,0 +1,19 @@
> +#ifndef __MEM2NODE_H
> +#define __MEM2NODE_H
> +
> +#include <linux/rbtree.h>
> +#include "env.h"
> +
> +struct entry;
> +
> +struct mem2node {
> +	struct rb_root	 root;
> +	struct entry	*entry;
> +	int		 cnt;
> +};
> +
> +int  mem2node__init(struct mem2node *map, struct perf_env *env);
> +void mem2node__exit(struct mem2node *map);
> +int  mem2node__node(struct mem2node *map, u64 addr);
> +
> +#endif /* __MEM2NODE_H */
> -- 
> 2.13.6