Messages in this thread | | | From | Nilay Vaish <> | Date | Wed, 1 Jun 2016 09:24:33 -0500 | Subject | Re: [PATCH 1/4] perf stat: Basic support for TopDown in perf stat |
| |
On 24 May 2016 at 14:52, Andi Kleen <andi@firstfloor.org> wrote: > From: Andi Kleen <ak@linux.intel.com> > > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c > index 715a1128daeb..dab184d86816 100644 > --- a/tools/perf/builtin-stat.c > +++ b/tools/perf/builtin-stat.c > @@ -59,10 +59,13 @@ > #include "util/thread.h" > #include "util/thread_map.h" > #include "util/counts.h" > +#include "util/group.h" > #include "util/session.h" > #include "util/tool.h" > +#include "util/group.h" > #include "asm/bug.h"
You have included util/group.h twice. Is this intentional?
> +static int topdown_filter_events(const char **attr, char **str, bool use_group) > +{ > + int off = 0; > + int i; > + int len = 0; > + char *s; > + > + for (i = 0; attr[i]; i++) { > + if (pmu_have_event("cpu", attr[i])) { > + len += strlen(attr[i]) + 1; > + attr[i - off] = attr[i]; > + } else > + off++; > + } > + attr[i - off] = NULL; > + > + *str = malloc(len + 1 + 2); > + if (!*str) > + return -1; > + s = *str; > + if (i - off == 0) { > + *s = 0; > + return 0; > + }
I think we are leaking some memory here. If i == off, then we set attr[0] = NULL and do not free the memory allocated to str.
> @@ -1909,6 +1982,46 @@ static int add_default_attributes(void) > return 0; > } > > + if (topdown_run) { > + char *str = NULL; > + bool warn = false; > + > + if (stat_config.aggr_mode != AGGR_GLOBAL && > + stat_config.aggr_mode != AGGR_CORE) { > + pr_err("top down event configuration requires --per-core mode\n"); > + return -1; > + } > + stat_config.aggr_mode = AGGR_CORE; > + if (nr_cgroups || !target__has_cpu(&target)) { > + pr_err("top down event configuration requires system-wide mode (-a)\n"); > + return -1; > + } > + > + if (!force_metric_only) > + metric_only = true; > + if (topdown_filter_events(topdown_attrs, &str, > + arch_topdown_check_group(&warn)) < 0) { > + pr_err("Out of memory\n"); > + return -1; > + } > + if (topdown_attrs[0] && str) { > + if (warn) > + arch_topdown_group_warn(); > + err = parse_events(evsel_list, str, NULL); > + if (err) { > + fprintf(stderr, > + "Cannot set up top down events %s: %d\n", > + str, err); > + free(str); > + return -1; > + } > + } else { > + fprintf(stderr, "System does not support topdown\n"); > + return -1; > + } > + free(str); > + } > +
Continuing with my comment from above memory leak, if i == off, topdown_attrs[0] will be NULL. So we would enter the else portion here and return -1. But we never free the string we allocated in the function topdown_filter_events(). I think we are leaking some memory though seems only about 3 bytes.
-- Nilay
| |