From: Andi Kleen <[email protected]>
I'm not fully sure if this is the correct fix, but without this
I get crashes on more complex perf stat metric usages. The problem
is that part of the state gets freed when a weak group fails,
but then is later still used. Just don't free the ids, we're
going to reuse them anyways on the weak group retry.
For example:
% perf stat -M IpB,IpCall,IpTB,IPC,Retiring_SMT,Frontend_Bound_SMT,Kernel_Utilization,CPU_Utilization --metric-only -a -I 1000 sleep 2
crashes and gives in valgrind:
=21527== Invalid write of size 8
==21527== at 0x4EE582: hlist_add_head (list.h:644)
==21527== by 0x4EFD3C: perf_evlist__id_hash (evlist.c:477)
==21527== by 0x4EFD99: perf_evlist__id_add (evlist.c:483)
==21527== by 0x4EFF15: perf_evlist__id_add_fd (evlist.c:524)
==21527== by 0x4FC693: store_evsel_ids (evsel.c:2969)
==21527== by 0x4FC76C: perf_evsel__store_ids (evsel.c:2986)
==21527== by 0x450DA7: __run_perf_stat (builtin-stat.c:519)
==21527== by 0x451285: run_perf_stat (builtin-stat.c:636)
==21527== by 0x454619: cmd_stat (builtin-stat.c:1966)
==21527== by 0x4D557D: run_builtin (perf.c:310)
==21527== by 0x4D57EA: handle_internal_command (perf.c:362)
==21527== by 0x4D5931: run_argv (perf.c:406)
==21527== Address 0x12e3f008 is 104 bytes inside a block of size 2,056 free'd
==21527== at 0x4839A0C: free (vg_replace_malloc.c:540)
==21527== by 0x627139: xyarray__delete (xyarray.c:32)
==21527== by 0x4F6BE4: perf_evsel__free_id (evsel.c:1253)
==21527== by 0x4FA11F: evsel__close (evsel.c:1994)
==21527== by 0x4F30A3: perf_evlist__reset_weak_group (evlist.c:1783)
==21527== by 0x450B47: __run_perf_stat (builtin-stat.c:466)
==21527== by 0x451285: run_perf_stat (builtin-stat.c:636)
==21527== by 0x454619: cmd_stat (builtin-stat.c:1966)
==21527== by 0x4D557D: run_builtin (perf.c:310)
==21527== by 0x4D57EA: handle_internal_command (perf.c:362)
==21527== by 0x4D5931: run_argv (perf.c:406)
==21527== by 0x4D5CAE: main (perf.c:531)
==21527== Block was alloc'd at
==21527== at 0x483AB1A: calloc (vg_replace_malloc.c:762)
==21527== by 0x627024: zalloc (zalloc.c:8)
==21527== by 0x627088: xyarray__new (xyarray.c:10)
==21527== by 0x4F6B20: perf_evsel__alloc_id (evsel.c:1237)
==21527== by 0x4FC74E: perf_evsel__store_ids (evsel.c:2983)
==21527== by 0x450DA7: __run_perf_stat (builtin-stat.c:519)
==21527== by 0x451285: run_perf_stat (builtin-stat.c:636)
==21527== by 0x454619: cmd_stat (builtin-stat.c:1966)
==21527== by 0x4D557D: run_builtin (perf.c:310)
==21527== by 0x4D57EA: handle_internal_command (perf.c:362)
==21527== by 0x4D5931: run_argv (perf.c:406)
==21527== by 0x4D5CAE: main (perf.c:531)
Signed-off-by: Andi Kleen <[email protected]>
---
tools/perf/util/evlist.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 095924aa186b..765303553041 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -1780,7 +1780,7 @@ struct evsel *perf_evlist__reset_weak_group(struct evlist *evsel_list,
is_open = false;
if (c2->leader == leader) {
if (is_open)
- evsel__close(c2);
+ perf_evsel__close(&evsel->core);
c2->leader = c2;
c2->core.nr_members = 0;
}
--
2.21.0
From: Andi Kleen <[email protected]>
My "compile perf statically" setup doesn't like this assert
for unknown reasons. Replace it with a standard BUG_ON
Signed-off-by: Andi Kleen <[email protected]>
---
tools/perf/util/expr.y | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
index f9a20a39b64a..5086a941295a 100644
--- a/tools/perf/util/expr.y
+++ b/tools/perf/util/expr.y
@@ -6,7 +6,6 @@
#define IN_EXPR_Y 1
#include "expr.h"
#include "smt.h"
-#include <assert.h>
#include <string.h>
#define MAXIDLEN 256
@@ -172,7 +171,8 @@ static int expr__lex(YYSTYPE *res, const char **pp)
void expr__add_id(struct parse_ctx *ctx, const char *name, double val)
{
int idx;
- assert(ctx->num_ids < MAX_PARSE_ID);
+
+ BUG_ON(ctx->num_ids >= MAX_PARSE_ID);
idx = ctx->num_ids++;
ctx->ids[idx].name = name;
ctx->ids[idx].val = val;
--
2.21.0
From: Andi Kleen <[email protected]>
Make sure to not free the name passed in by the caller, but free
all the allocated ids when parsing expressions.
The loop at the end knows that the first entry shouldn't be freed,
so make sure the caller name is the first entry.
Fixes
% perf stat -M IpB,IpCall,IpTB,IPC,Retiring_SMT,Frontend_Bound_SMT,Kernel_Utilization,CPU_Utilization --metric-only -a -I 1000 sleep 2
valgrind:
1.009943231 ==21527== Invalid read of size 1
==21527== at 0x483CB74: strcmp (vg_replace_strmem.c:849)
==21527== by 0x582CF8: collect_all_aliases (stat-display.c:554)
==21527== by 0x582EB3: collect_data (stat-display.c:577)
==21527== by 0x583A32: print_counter_aggr (stat-display.c:806)
==21527== by 0x584FAD: perf_evlist__print_counters (stat-display.c:1200)
==21527== by 0x45133A: print_counters (builtin-stat.c:655)
==21527== by 0x450629: process_interval (builtin-stat.c:353)
==21527== by 0x450FBD: __run_perf_stat (builtin-stat.c:564)
==21527== by 0x451285: run_perf_stat (builtin-stat.c:636)
==21527== by 0x454619: cmd_stat (builtin-stat.c:1966)
==21527== by 0x4D557D: run_builtin (perf.c:310)
==21527== by 0x4D57EA: handle_internal_command (perf.c:362)
==21527== Address 0x12826cd0 is 0 bytes inside a block of size 25 free'd
==21527== at 0x4839A0C: free (vg_replace_malloc.c:540)
==21527== by 0x627041: __zfree (zalloc.c:13)
==21527== by 0x57F66A: generic_metric (stat-shadow.c:814)
==21527== by 0x580B21: perf_stat__print_shadow_stats (stat-shadow.c:1057)
==21527== by 0x58418E: print_metric_headers (stat-display.c:943)
==21527== by 0x5844BC: print_interval (stat-display.c:1004)
==21527== by 0x584DEB: perf_evlist__print_counters (stat-display.c:1172)
==21527== by 0x45133A: print_counters (builtin-stat.c:655)
==21527== by 0x450629: process_interval (builtin-stat.c:353)
==21527== by 0x450FBD: __run_perf_stat (builtin-stat.c:564)
==21527== by 0x451285: run_perf_stat (builtin-stat.c:636)
==21527== by 0x454619: cmd_stat (builtin-stat.c:1966)
==21527== Block was alloc'd at
==21527== at 0x483880B: malloc (vg_replace_malloc.c:309)
==21527== by 0x51677DE: strdup (in /usr/lib64/libc-2.29.so)
==21527== by 0x506457: parse_events_name (parse-events.c:1754)
==21527== by 0x5550BB: parse_events_parse (parse-events.y:214)
==21527== by 0x50694D: parse_events__scanner (parse-events.c:1887)
==21527== by 0x506AEF: parse_events (parse-events.c:1927)
==21527== by 0x521D8B: metricgroup__parse_groups (metricgroup.c:527)
==21527== by 0x45156F: parse_metric_groups (builtin-stat.c:721)
==21527== by 0x6228A9: get_value (parse-options.c:243)
==21527== by 0x62363F: parse_short_opt (parse-options.c:348)
==21527== by 0x62363F: parse_options_step (parse-options.c:536)
==21527== by 0x62363F: parse_options_subcommand (parse-options.c:651)
==21527== by 0x453C1D: cmd_stat (builtin-stat.c:1718)
==21527== by 0x4D557D: run_builtin (perf.c:310)
and also a leak report.
Signed-off-by: Andi Kleen <[email protected]>
---
tools/perf/util/stat-shadow.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
index 70c87fdb2a43..2c41d47f6f83 100644
--- a/tools/perf/util/stat-shadow.c
+++ b/tools/perf/util/stat-shadow.c
@@ -738,6 +738,8 @@ static void generic_metric(struct perf_stat_config *config,
char *n, *pn;
expr__ctx_init(&pctx);
+ /* Must be first id entry */
+ expr__add_id(&pctx, name, avg);
for (i = 0; metric_events[i]; i++) {
struct saved_value *v;
struct stats *stats;
@@ -776,8 +778,6 @@ static void generic_metric(struct perf_stat_config *config,
expr__add_id(&pctx, n, avg_stats(stats)*scale);
}
- expr__add_id(&pctx, name, avg);
-
if (!metric_events[i]) {
const char *p = metric_expr;
--
2.21.0
On Mon, Sep 23, 2019 at 04:33:37PM -0700, Andi Kleen wrote:
> From: Andi Kleen <[email protected]>
>
> I'm not fully sure if this is the correct fix, but without this
> I get crashes on more complex perf stat metric usages. The problem
> is that part of the state gets freed when a weak group fails,
> but then is later still used. Just don't free the ids, we're
> going to reuse them anyways on the weak group retry.
>
> For example:
>
> % perf stat -M IpB,IpCall,IpTB,IPC,Retiring_SMT,Frontend_Bound_SMT,Kernel_Utilization,CPU_Utilization --metric-only -a -I 1000 sleep 2
>
> crashes and gives in valgrind:
>
> =21527== Invalid write of size 8
> ==21527== at 0x4EE582: hlist_add_head (list.h:644)
> ==21527== by 0x4EFD3C: perf_evlist__id_hash (evlist.c:477)
> ==21527== by 0x4EFD99: perf_evlist__id_add (evlist.c:483)
> ==21527== by 0x4EFF15: perf_evlist__id_add_fd (evlist.c:524)
> ==21527== by 0x4FC693: store_evsel_ids (evsel.c:2969)
> ==21527== by 0x4FC76C: perf_evsel__store_ids (evsel.c:2986)
> ==21527== by 0x450DA7: __run_perf_stat (builtin-stat.c:519)
> ==21527== by 0x451285: run_perf_stat (builtin-stat.c:636)
> ==21527== by 0x454619: cmd_stat (builtin-stat.c:1966)
> ==21527== by 0x4D557D: run_builtin (perf.c:310)
> ==21527== by 0x4D57EA: handle_internal_command (perf.c:362)
> ==21527== by 0x4D5931: run_argv (perf.c:406)
> ==21527== Address 0x12e3f008 is 104 bytes inside a block of size 2,056 free'd
> ==21527== at 0x4839A0C: free (vg_replace_malloc.c:540)
> ==21527== by 0x627139: xyarray__delete (xyarray.c:32)
> ==21527== by 0x4F6BE4: perf_evsel__free_id (evsel.c:1253)
> ==21527== by 0x4FA11F: evsel__close (evsel.c:1994)
> ==21527== by 0x4F30A3: perf_evlist__reset_weak_group (evlist.c:1783)
> ==21527== by 0x450B47: __run_perf_stat (builtin-stat.c:466)
> ==21527== by 0x451285: run_perf_stat (builtin-stat.c:636)
> ==21527== by 0x454619: cmd_stat (builtin-stat.c:1966)
> ==21527== by 0x4D557D: run_builtin (perf.c:310)
> ==21527== by 0x4D57EA: handle_internal_command (perf.c:362)
> ==21527== by 0x4D5931: run_argv (perf.c:406)
> ==21527== by 0x4D5CAE: main (perf.c:531)
> ==21527== Block was alloc'd at
> ==21527== at 0x483AB1A: calloc (vg_replace_malloc.c:762)
> ==21527== by 0x627024: zalloc (zalloc.c:8)
> ==21527== by 0x627088: xyarray__new (xyarray.c:10)
> ==21527== by 0x4F6B20: perf_evsel__alloc_id (evsel.c:1237)
> ==21527== by 0x4FC74E: perf_evsel__store_ids (evsel.c:2983)
> ==21527== by 0x450DA7: __run_perf_stat (builtin-stat.c:519)
> ==21527== by 0x451285: run_perf_stat (builtin-stat.c:636)
> ==21527== by 0x454619: cmd_stat (builtin-stat.c:1966)
> ==21527== by 0x4D557D: run_builtin (perf.c:310)
> ==21527== by 0x4D57EA: handle_internal_command (perf.c:362)
> ==21527== by 0x4D5931: run_argv (perf.c:406)
> ==21527== by 0x4D5CAE: main (perf.c:531)
>
> Signed-off-by: Andi Kleen <[email protected]>
> ---
> tools/perf/util/evlist.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 095924aa186b..765303553041 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -1780,7 +1780,7 @@ struct evsel *perf_evlist__reset_weak_group(struct evlist *evsel_list,
> is_open = false;
> if (c2->leader == leader) {
> if (is_open)
> - evsel__close(c2);
> + perf_evsel__close(&evsel->core);
id/sample_id arrays are not created when evsel is open but
we free it at close
for now this fix seems correct to me.. we are moving id/sample_id
arrays under libperf, I'll make a note to check on close and reopen
of evsel and add some tests for that
Acked-by: Jiri Olsa <[email protected]>
thanks,
jirka
> c2->leader = c2;
> c2->core.nr_members = 0;
> }
> --
> 2.21.0
>
On Mon, Sep 23, 2019 at 04:33:38PM -0700, Andi Kleen wrote:
> From: Andi Kleen <[email protected]>
>
> My "compile perf statically" setup doesn't like this assert
> for unknown reasons. Replace it with a standard BUG_ON
>
> Signed-off-by: Andi Kleen <[email protected]>
> ---
> tools/perf/util/expr.y | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
> index f9a20a39b64a..5086a941295a 100644
> --- a/tools/perf/util/expr.y
> +++ b/tools/perf/util/expr.y
> @@ -6,7 +6,6 @@
> #define IN_EXPR_Y 1
> #include "expr.h"
> #include "smt.h"
> -#include <assert.h>
> #include <string.h>
>
> #define MAXIDLEN 256
> @@ -172,7 +171,8 @@ static int expr__lex(YYSTYPE *res, const char **pp)
> void expr__add_id(struct parse_ctx *ctx, const char *name, double val)
> {
> int idx;
> - assert(ctx->num_ids < MAX_PARSE_ID);
> +
> + BUG_ON(ctx->num_ids >= MAX_PARSE_ID);
hi,
getting compilation fail
LINK perf
/usr/bin/ld: perf-in.o: in function `expr__add_id':
/home/jolsa/kernel/linux-perf/tools/perf/util/expr.y:175: undefined reference to `BUG_ON'
collect2: error: ld returned 1 exit status
make[2]: *** [Makefile.perf:609: perf] Error 1
make[1]: *** [Makefile.perf:221: sub-make] Error 2
make: *** [Makefile:70: all] Error 2
jirka
> idx = ctx->num_ids++;
> ctx->ids[idx].name = name;
> ctx->ids[idx].val = val;
> --
> 2.21.0
>
On Mon, Sep 23, 2019 at 04:33:39PM -0700, Andi Kleen wrote:
> From: Andi Kleen <[email protected]>
>
> Make sure to not free the name passed in by the caller, but free
> all the allocated ids when parsing expressions.
>
> The loop at the end knows that the first entry shouldn't be freed,
> so make sure the caller name is the first entry.
>
> Fixes
>
> % perf stat -M IpB,IpCall,IpTB,IPC,Retiring_SMT,Frontend_Bound_SMT,Kernel_Utilization,CPU_Utilization --metric-only -a -I 1000 sleep 2
>
> valgrind:
> 1.009943231 ==21527== Invalid read of size 1
> ==21527== at 0x483CB74: strcmp (vg_replace_strmem.c:849)
> ==21527== by 0x582CF8: collect_all_aliases (stat-display.c:554)
> ==21527== by 0x582EB3: collect_data (stat-display.c:577)
> ==21527== by 0x583A32: print_counter_aggr (stat-display.c:806)
> ==21527== by 0x584FAD: perf_evlist__print_counters (stat-display.c:1200)
> ==21527== by 0x45133A: print_counters (builtin-stat.c:655)
> ==21527== by 0x450629: process_interval (builtin-stat.c:353)
> ==21527== by 0x450FBD: __run_perf_stat (builtin-stat.c:564)
> ==21527== by 0x451285: run_perf_stat (builtin-stat.c:636)
> ==21527== by 0x454619: cmd_stat (builtin-stat.c:1966)
> ==21527== by 0x4D557D: run_builtin (perf.c:310)
> ==21527== by 0x4D57EA: handle_internal_command (perf.c:362)
> ==21527== Address 0x12826cd0 is 0 bytes inside a block of size 25 free'd
> ==21527== at 0x4839A0C: free (vg_replace_malloc.c:540)
> ==21527== by 0x627041: __zfree (zalloc.c:13)
> ==21527== by 0x57F66A: generic_metric (stat-shadow.c:814)
> ==21527== by 0x580B21: perf_stat__print_shadow_stats (stat-shadow.c:1057)
> ==21527== by 0x58418E: print_metric_headers (stat-display.c:943)
> ==21527== by 0x5844BC: print_interval (stat-display.c:1004)
> ==21527== by 0x584DEB: perf_evlist__print_counters (stat-display.c:1172)
> ==21527== by 0x45133A: print_counters (builtin-stat.c:655)
> ==21527== by 0x450629: process_interval (builtin-stat.c:353)
> ==21527== by 0x450FBD: __run_perf_stat (builtin-stat.c:564)
> ==21527== by 0x451285: run_perf_stat (builtin-stat.c:636)
> ==21527== by 0x454619: cmd_stat (builtin-stat.c:1966)
> ==21527== Block was alloc'd at
> ==21527== at 0x483880B: malloc (vg_replace_malloc.c:309)
> ==21527== by 0x51677DE: strdup (in /usr/lib64/libc-2.29.so)
> ==21527== by 0x506457: parse_events_name (parse-events.c:1754)
> ==21527== by 0x5550BB: parse_events_parse (parse-events.y:214)
> ==21527== by 0x50694D: parse_events__scanner (parse-events.c:1887)
> ==21527== by 0x506AEF: parse_events (parse-events.c:1927)
> ==21527== by 0x521D8B: metricgroup__parse_groups (metricgroup.c:527)
> ==21527== by 0x45156F: parse_metric_groups (builtin-stat.c:721)
> ==21527== by 0x6228A9: get_value (parse-options.c:243)
> ==21527== by 0x62363F: parse_short_opt (parse-options.c:348)
> ==21527== by 0x62363F: parse_options_step (parse-options.c:536)
> ==21527== by 0x62363F: parse_options_subcommand (parse-options.c:651)
> ==21527== by 0x453C1D: cmd_stat (builtin-stat.c:1718)
> ==21527== by 0x4D557D: run_builtin (perf.c:310)
>
> and also a leak report.
>
> Signed-off-by: Andi Kleen <[email protected]>
> ---
> tools/perf/util/stat-shadow.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
> index 70c87fdb2a43..2c41d47f6f83 100644
> --- a/tools/perf/util/stat-shadow.c
> +++ b/tools/perf/util/stat-shadow.c
> @@ -738,6 +738,8 @@ static void generic_metric(struct perf_stat_config *config,
> char *n, *pn;
>
> expr__ctx_init(&pctx);
> + /* Must be first id entry */
> + expr__add_id(&pctx, name, avg);
hum, shouldn't u instead use strdup(name) instead of name?
jirka
> for (i = 0; metric_events[i]; i++) {
> struct saved_value *v;
> struct stats *stats;
> @@ -776,8 +778,6 @@ static void generic_metric(struct perf_stat_config *config,
> expr__add_id(&pctx, n, avg_stats(stats)*scale);
> }
>
> - expr__add_id(&pctx, name, avg);
> -
> if (!metric_events[i]) {
> const char *p = metric_expr;
>
> --
> 2.21.0
>
> > expr__ctx_init(&pctx);
> > + /* Must be first id entry */
> > + expr__add_id(&pctx, name, avg);
>
> hum, shouldn't u instead use strdup(name) instead of name?
The cleanup loop later skips freeing the first entry.
-Andi
On Tue, Sep 24, 2019 at 07:08:56AM -0700, Andi Kleen wrote:
> > > expr__ctx_init(&pctx);
> > > + /* Must be first id entry */
> > > + expr__add_id(&pctx, name, avg);
> >
> > hum, shouldn't u instead use strdup(name) instead of name?
>
> The cleanup loop later skips freeing the first entry.
aaah, nice ;-)
Acked-by: Jiri Olsa <[email protected]>
thanks,
jirka
> id/sample_id arrays are not created when evsel is open but
> we free it at close
>
> for now this fix seems correct to me.. we are moving id/sample_id
> arrays under libperf, I'll make a note to check on close and reopen
> of evsel and add some tests for that
>
> Acked-by: Jiri Olsa <[email protected]>
It looks like there are still some bogus closes in such a case, at least valgrind
complains about some close(-1). But these should be harmless, so I guess
we can leave them for now.
-Andi
Em Tue, Sep 24, 2019 at 04:44:18PM +0200, Jiri Olsa escreveu:
> On Tue, Sep 24, 2019 at 07:08:56AM -0700, Andi Kleen wrote:
> > > > expr__ctx_init(&pctx);
> > > > + /* Must be first id entry */
> > > > + expr__add_id(&pctx, name, avg);
> > >
> > > hum, shouldn't u instead use strdup(name) instead of name?
> >
> > The cleanup loop later skips freeing the first entry.
>
> aaah, nice ;-)
>
> Acked-by: Jiri Olsa <[email protected]>
Thanks, reproduced and applied, before the patch:
# perf stat -M IpB,IpCall,IpTB,IPC,Retiring_SMT,Frontend_Bound_SMT,Kernel_Utilization,CPU_Utilization --metric-only -a -I 1000 sleep 2
# time CPU_Utilization
1.000470810 free(): double free detected in tcache 2
Aborted (core dumped)
#
- Arnaldo