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
