Re: [PATCH 08/19] perf tools: Add mem2node object
On Thu, Mar 08, 2018 at 09:58:49AM -0300, Arnaldo Carvalho de Melo wrote: SNIP > > I don't think we need nentries.. AFAIK realloc works ok over single variable > > So: > > 1) you alloc entries with a max number of entries > > 2) you go on populating it > > 3) there are some left, lets shrink it: > > entries = realloc(entries, nr_entries * sizeof(entries[0]); > > Here it will probably not fail, but you check it anyway, and that is > right, what happens if this returns NULL? entries gets set to NULL, > we lose the reference to the allocated memory and you return -ENOMEM, > right? > > We end up leaking entries when what I'm suggesting you to do is to > not clobber entries with the return of realloc() (doing it this way most > of the time leads to bugs), but instead store it to a temp var > (nentries), and if it succeeds, then you know that you can > set nentries to entries and go ahead with your nicely shrunk block of > memory. > > If it fails, then you continue with the original block of memory, that > continues to have what you just set up, etc. ah that ;-) ok, will fix thanks, jirka
Re: [PATCH 08/19] perf tools: Add mem2node object
Em Thu, Mar 08, 2018 at 09:58:49AM -0300, Arnaldo Carvalho de Melo escreveu: > We end up leaking entries when what I'm suggesting you to do is to > not clobber entries with the return of realloc() (doing it this way most > of the time leads to bugs), but instead store it to a temp var > (nentries), and if it succeeds, then you know that you can > set nentries to entries and go ahead with your nicely shrunk block of > memory. > If it fails, then you continue with the original block of memory, that > continues to have what you just set up, etc. > Lemme look a third time to your original patch, I must be missing > something... I don't think I'm missing anything ;-) - Arnaldo
Re: [PATCH 08/19] perf tools: Add mem2node object
Em Thu, Mar 08, 2018 at 12:03:07PM +0100, Jiri Olsa escreveu:
> 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 ..6da8ddbb1182
> > > --- /dev/null
> > > +++ b/tools/perf/util/mem2node.c
> > > @@ -0,0 +1,129 @@
> > > +#include
> > > +#include
> > > +#include
> > > +#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
So:
1) you alloc entries with a max number of entries
2) you go on populating it
3) there are some left, lets shrink it:
entries = realloc(entries, nr_entries * sizeof(entries[0]);
Here it will probably not fail, but you check it anyway, and that is
right, what happens if this returns NULL? entries gets set to NULL,
we lose the reference to the allocated memory and you return -ENOMEM,
right?
We end up leaking entries when what I'm suggesting you to do is to
not clobber entries with the return of realloc() (doing it this way most
of the time leads to bugs), but instead store it to a temp var
(nentries), and if it succeeds, then you know that you can
set nentries to entries and go ahead with your nicely shrunk block of
memory.
If it fails, then you continue with the original block of memory, that
continues to have what you just set up, etc.
Lemme look a third time to your original patch, I must be missing
something...
- Arnaldo
Re: [PATCH 08/19] perf tools: Add mem2node object
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 ..6da8ddbb1182
> > --- /dev/null
> > +++ b/tools/perf/util/mem2node.c
> > @@ -0,0 +1,129 @@
> > +#include
> > +#include
> > +#include
> > +#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
Re: [PATCH 08/19] perf tools: Add mem2node object
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-qq7sohu774wxq154n3my0...@git.kernel.org
> Signed-off-by: Jiri Olsa
> ---
> 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 ..6da8ddbb1182
> --- /dev/null
> +++ b/tools/perf/util/mem2node.c
> @@ -0,0 +1,129 @@
> +#include
> +#include
> +#include
> +#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++) {
