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

[Jiri Olsa <jolsa@redhat.com>]

On Wed, Mar 07, 2018 at 04:27:36PM -0300, Arnaldo Carvalho de Melo wrote:

SNIP

> > 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

ok, will change

> 
> > +
> > +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'.

ok


SNIP

> > +
> > +			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);

ook

> 
> > +			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:

just shortening the memory, because some entries merge together


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

I don't think we need nentries.. AFAIK realloc works ok over single variable

jirka