| Message ID | 20200520182011.32236-1-irogers@google.com |
|---|---|
| Headers | show |
| Series | Share events between metrics | expand |
On Wed, May 20, 2020 at 11:20:04AM -0700, Ian Rogers wrote: SNIP > There are 5 out of 12 metric groups where no events are shared, such > as Power, however, disabling grouping of events always reduces the > number of events. > > The result for Memory_BW needs explanation: > > Metric group: Memory_BW > - No merging (old default, now --metric-no-merge): 9 > - Merging over metrics (new default) : 5 > - No event groups and merging (--metric-no-group): 11 > > Both with and without merging the groups fail to be set up and so the > event counts here are for broken metrics. The --metric-no-group number > is accurate as all the events are scheduled. Ideally a constraint > would be added for these metrics in the json code to avoid grouping. > > v2. rebases on kernel/git/acme/linux.git branch tmp.perf/core, fixes a > missing comma with metric lists (reported-by Jiri Olsa > <jolsa@redhat.com>) and adds early returns to metricgroup__add_metric > (suggested-by Jiri Olsa). Acked-by: Jiri Olsa <jolsa@redhat.com> thanks, jirka > > v1. was prepared on kernel/git/acme/linux.git branch tmp.perf/core > > Compared to RFC v3: fix a bug where unnecessary commas were passed to > parse-events and were echoed. Fix a bug where the same event could be > matched more than once with --metric-no-group, causing there to be > events missing. > https://lore.kernel.org/lkml/20200508053629.210324-1-irogers@google.com/ > > Ian Rogers (7): > perf metricgroup: Always place duration_time last > perf metricgroup: Use early return in add_metric > perf metricgroup: Delay events string creation > perf metricgroup: Order event groups by size > perf metricgroup: Remove duped metric group events > perf metricgroup: Add options to not group or merge > perf metricgroup: Remove unnecessary ',' from events > > tools/perf/Documentation/perf-stat.txt | 19 ++ > tools/perf/builtin-stat.c | 11 +- > tools/perf/util/metricgroup.c | 239 ++++++++++++++++++------- > tools/perf/util/metricgroup.h | 6 +- > tools/perf/util/stat.h | 2 + > 5 files changed, 207 insertions(+), 70 deletions(-) > > -- > 2.26.2.761.g0e0b3e54be-goog >
Em Thu, May 21, 2020 at 01:43:25PM +0200, Jiri Olsa escreveu: > On Wed, May 20, 2020 at 11:20:04AM -0700, Ian Rogers wrote: > > SNIP > > > There are 5 out of 12 metric groups where no events are shared, such > > as Power, however, disabling grouping of events always reduces the > > number of events. > > > > The result for Memory_BW needs explanation: > > > > Metric group: Memory_BW > > - No merging (old default, now --metric-no-merge): 9 > > - Merging over metrics (new default) : 5 > > - No event groups and merging (--metric-no-group): 11 > > > > Both with and without merging the groups fail to be set up and so the > > event counts here are for broken metrics. The --metric-no-group number > > is accurate as all the events are scheduled. Ideally a constraint > > would be added for these metrics in the json code to avoid grouping. > > > > v2. rebases on kernel/git/acme/linux.git branch tmp.perf/core, fixes a > > missing comma with metric lists (reported-by Jiri Olsa > > <jolsa@redhat.com>) and adds early returns to metricgroup__add_metric > > (suggested-by Jiri Olsa). > > Acked-by: Jiri Olsa <jolsa@redhat.com> Applied and pushed to tmp.perf/core, will move to perf/core as soon as testing finishes, - Arnaldo
On 5/20/20 11:50 PM, Ian Rogers wrote: > Metric groups contain metrics. Metrics create groups of events to > ideally be scheduled together. Often metrics refer to the same events, > for example, a cache hit and cache miss rate. Using separate event > groups means these metrics are multiplexed at different times and the > counts don't sum to 100%. More multiplexing also decreases the > accuracy of the measurement. > > This change orders metrics from groups or the command line, so that > the ones with the most events are set up first. Later metrics see if > groups already provide their events, and reuse them if > possible. Unnecessary events and groups are eliminated. > > The option --metric-no-group is added so that metrics aren't placed in > groups. This affects multiplexing and may increase sharing. > > The option --metric-mo-merge is added and with this option the > existing grouping behavior is preserved. > > Using skylakex metrics I ran the following shell code to count the > number of events for each metric group (this ignores metric groups > with a single metric, and one of the duplicated TopdownL1 and > TopDownL1 groups): > > for i in all Branches BrMispredicts Cache_Misses FLOPS Instruction_Type Memory_BW Pipeline Power SMT Summary TopdownL1 TopdownL1_SMT > do > echo Metric group: $i > echo -n " - No merging (old default, now --metric-no-merge): " > /tmp/perf/perf stat -a --metric-no-merge -M $i sleep 1 2>&1 | grep -v "^ *#" | egrep " +[0-9,.]+ [^s]" | wc -l > echo -n " - Merging over metrics (new default) : " > /tmp/perf/perf stat -a -M $i sleep 1 2>&1 | grep -v "^ *#" | egrep " +[0-9,.]+ [^s]"|wc -l > echo -n " - No event groups and merging (--metric-no-group): " > /tmp/perf/perf stat -a --metric-no-group -M $i sleep 1 2>&1 | grep -v "^ *#" | egrep " +[0-9,.]+ [^s]"|wc -l > done > > Metric group: all > - No merging (old default, now --metric-no-merge): 193 > - Merging over metrics (new default) : 142 > - No event groups and merging (--metric-no-group): 84 > Metric group: Branches > - No merging (old default, now --metric-no-merge): 8 > - Merging over metrics (new default) : 8 > - No event groups and merging (--metric-no-group): 4 > Metric group: BrMispredicts > - No merging (old default, now --metric-no-merge): 11 > - Merging over metrics (new default) : 11 > - No event groups and merging (--metric-no-group): 10 > Metric group: Cache_Misses > - No merging (old default, now --metric-no-merge): 11 > - Merging over metrics (new default) : 9 > - No event groups and merging (--metric-no-group): 6 > Metric group: FLOPS > - No merging (old default, now --metric-no-merge): 18 > - Merging over metrics (new default) : 10 > - No event groups and merging (--metric-no-group): 10 > Metric group: Instruction_Type > - No merging (old default, now --metric-no-merge): 6 > - Merging over metrics (new default) : 6 > - No event groups and merging (--metric-no-group): 4 > Metric group: Pipeline > - No merging (old default, now --metric-no-merge): 6 > - Merging over metrics (new default) : 6 > - No event groups and merging (--metric-no-group): 5 > Metric group: Power > - No merging (old default, now --metric-no-merge): 16 > - Merging over metrics (new default) : 16 > - No event groups and merging (--metric-no-group): 10 > Metric group: SMT > - No merging (old default, now --metric-no-merge): 11 > - Merging over metrics (new default) : 8 > - No event groups and merging (--metric-no-group): 7 > Metric group: Summary > - No merging (old default, now --metric-no-merge): 19 > - Merging over metrics (new default) : 17 > - No event groups and merging (--metric-no-group): 17 > Metric group: TopdownL1 > - No merging (old default, now --metric-no-merge): 16 > - Merging over metrics (new default) : 7 > - No event groups and merging (--metric-no-group): 7 > Metric group: TopdownL1_SMT > - No merging (old default, now --metric-no-merge): 24 > - Merging over metrics (new default) : 7 > - No event groups and merging (--metric-no-group): 7 > > There are 5 out of 12 metric groups where no events are shared, such > as Power, however, disabling grouping of events always reduces the > number of events. > > The result for Memory_BW needs explanation: > > Metric group: Memory_BW > - No merging (old default, now --metric-no-merge): 9 > - Merging over metrics (new default) : 5 > - No event groups and merging (--metric-no-group): 11 > > Both with and without merging the groups fail to be set up and so the > event counts here are for broken metrics. The --metric-no-group number > is accurate as all the events are scheduled. Ideally a constraint > would be added for these metrics in the json code to avoid grouping. > > v2. rebases on kernel/git/acme/linux.git branch tmp.perf/core, fixes a > missing comma with metric lists (reported-by Jiri Olsa > <jolsa@redhat.com>) and adds early returns to metricgroup__add_metric > (suggested-by Jiri Olsa). > > v1. was prepared on kernel/git/acme/linux.git branch tmp.perf/core > > Compared to RFC v3: fix a bug where unnecessary commas were passed to > parse-events and were echoed. Fix a bug where the same event could be > matched more than once with --metric-no-group, causing there to be > events missing. > https://lore.kernel.org/lkml/20200508053629.210324-1-irogers@google.com/ > > Ian Rogers (7): > perf metricgroup: Always place duration_time last > perf metricgroup: Use early return in add_metric > perf metricgroup: Delay events string creation > perf metricgroup: Order event groups by size > perf metricgroup: Remove duped metric group events > perf metricgroup: Add options to not group or merge > perf metricgroup: Remove unnecessary ',' from events > Reviewd-By: Kajol Jain <kjain@linux.ibm.com> Tested-By: Kajol Jain <kjain@linux.ibm.com> ( Tested it to see behavior with some metric groups in both x86 and Power machine) Thanks, Kajol Jain > tools/perf/Documentation/perf-stat.txt | 19 ++ > tools/perf/builtin-stat.c | 11 +- > tools/perf/util/metricgroup.c | 239 ++++++++++++++++++------- > tools/perf/util/metricgroup.h | 6 +- > tools/perf/util/stat.h | 2 + > 5 files changed, 207 insertions(+), 70 deletions(-) >
On Thu, May 21, 2020 at 02:22:35PM -0300, Arnaldo Carvalho de Melo wrote: > Em Thu, May 21, 2020 at 01:43:25PM +0200, Jiri Olsa escreveu: > > On Wed, May 20, 2020 at 11:20:04AM -0700, Ian Rogers wrote: > > > > SNIP > > > > > There are 5 out of 12 metric groups where no events are shared, such > > > as Power, however, disabling grouping of events always reduces the > > > number of events. > > > > > > The result for Memory_BW needs explanation: > > > > > > Metric group: Memory_BW > > > - No merging (old default, now --metric-no-merge): 9 > > > - Merging over metrics (new default) : 5 > > > - No event groups and merging (--metric-no-group): 11 > > > > > > Both with and without merging the groups fail to be set up and so the > > > event counts here are for broken metrics. The --metric-no-group number > > > is accurate as all the events are scheduled. Ideally a constraint > > > would be added for these metrics in the json code to avoid grouping. > > > > > > v2. rebases on kernel/git/acme/linux.git branch tmp.perf/core, fixes a > > > missing comma with metric lists (reported-by Jiri Olsa > > > <jolsa@redhat.com>) and adds early returns to metricgroup__add_metric > > > (suggested-by Jiri Olsa). > > > > Acked-by: Jiri Olsa <jolsa@redhat.com> > > Applied and pushed to tmp.perf/core, will move to perf/core as soon as > testing finishes, I checked tmp.perf/core and I'm getting segfault for 'perf test expr' 7: Simple expression parser : Program received signal SIGSEGV, Segmentation fault. 0x000000000067841e in hashmap_find_entry (map=0x7fffffffd0c0, key=0xc83b30, hash=9893851511679796638, pprev=0x0, entry=0x7fffffffc658) at hashmap.c:131 131 for (prev_ptr = &map->buckets[hash], cur = *prev_ptr; (gdb) bt #0 0x000000000067841e in hashmap_find_entry (map=0x7fffffffd0c0, key=0xc83b30, hash=9893851511679796638, pprev=0x0, entry=0x7fffffffc658) at hashmap.c:131 #1 0x000000000067853a in hashmap__insert (map=0x7fffffffd0c0, key=0xc83b30, value=0x0, strategy=HASHMAP_SET, old_key=0x7fffffffc718, old_value=0x7fffffffc710) at hashmap.c:160 #2 0x00000000005d3209 in hashmap__set (map=0x7fffffffd0c0, key=0xc83b30, value=0x0, old_key=0x7fffffffc718, old_value=0x7fffffffc710) at /home/jolsa/kernel/linux-perf/tools/perf/util/hashmap.h:107 #3 0x00000000005d3386 in expr__add_id (ctx=0x7fffffffd0c0, name=0xc83b30 "FOO", val=0) at util/expr.c:45 #4 0x00000000005d27ee in expr_parse (final_val=0x0, ctx=0x7fffffffd0c0, scanner=0xc87990) at util/expr.y:63 #5 0x00000000005d35b7 in __expr__parse (val=0x0, ctx=0x7fffffffd0c0, expr=0x75a84b "FOO + BAR + BAZ + BOZO", start=259, runtime=1) at util/expr.c:102 #6 0x00000000005d36c6 in expr__find_other (expr=0x75a84b "FOO + BAR + BAZ + BOZO", one=0x75a791 "FOO", ctx=0x7fffffffd0c0, runtime=1) at util/expr.c:121 #7 0x00000000004e3aaf in test__expr (t=0xa7bd40 <generic_tests+384>, subtest=-1) at tests/expr.c:55 #8 0x00000000004b5651 in run_test (test=0xa7bd40 <generic_tests+384>, subtest=-1) at tests/builtin-test.c:393 #9 0x00000000004b5787 in test_and_print (t=0xa7bd40 <generic_tests+384>, force_skip=false, subtest=-1) at tests/builtin-test.c:423 #10 0x00000000004b61c4 in __cmd_test (argc=1, argv=0x7fffffffd7f0, skiplist=0x0) at tests/builtin-test.c:628 #11 0x00000000004b6911 in cmd_test (argc=1, argv=0x7fffffffd7f0) at tests/builtin-test.c:772 #12 0x00000000004e977b in run_builtin (p=0xa7eee8 <commands+552>, argc=3, argv=0x7fffffffd7f0) at perf.c:312 #13 0x00000000004e99e8 in handle_internal_command (argc=3, argv=0x7fffffffd7f0) at perf.c:364 #14 0x00000000004e9b2f in run_argv (argcp=0x7fffffffd64c, argv=0x7fffffffd640) at perf.c:408 #15 0x00000000004e9efb in main (argc=3, argv=0x7fffffffd7f0) at perf.c:538 attached patch fixes it for me, but I'm not sure this should be necessary jirka --- diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c index 1cb02ca2b15f..21693fe516c1 100644 --- a/tools/perf/tests/expr.c +++ b/tools/perf/tests/expr.c @@ -52,6 +52,7 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused) TEST_ASSERT_VAL("missing operand", ret == -1); expr__ctx_clear(&ctx); + expr__ctx_init(&ctx); TEST_ASSERT_VAL("find other", expr__find_other("FOO + BAR + BAZ + BOZO", "FOO", &ctx, 1) == 0); @@ -64,6 +65,7 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused) (void **)&val_ptr)); expr__ctx_clear(&ctx); + expr__ctx_init(&ctx); TEST_ASSERT_VAL("find other", expr__find_other("EVENT1\\,param\\=?@ + EVENT2\\,param\\=?@", NULL, &ctx, 3) == 0);
Em Fri, May 22, 2020 at 02:55:46PM +0530, kajoljain escreveu: > On 5/20/20 11:50 PM, Ian Rogers wrote: > > Metric groups contain metrics. Metrics create groups of events to > > ideally be scheduled together. Often metrics refer to the same events, > > for example, a cache hit and cache miss rate. Using separate event > > groups means these metrics are multiplexed at different times and the > > counts don't sum to 100%. More multiplexing also decreases the > > accuracy of the measurement. <SNIP> > > Ian Rogers (7): > > perf metricgroup: Always place duration_time last > > perf metricgroup: Use early return in add_metric > > perf metricgroup: Delay events string creation > > perf metricgroup: Order event groups by size > > perf metricgroup: Remove duped metric group events > > perf metricgroup: Add options to not group or merge > > perf metricgroup: Remove unnecessary ',' from events > Reviewd-By: Kajol Jain <kjain@linux.ibm.com> > Tested-By: Kajol Jain <kjain@linux.ibm.com> ( Tested it to see behavior with some metric groups in both x86 and Power machine) Thanks, added to the patches, - Arnaldo
Em Fri, May 22, 2020 at 12:13:11PM +0200, Jiri Olsa escreveu: > On Thu, May 21, 2020 at 02:22:35PM -0300, Arnaldo Carvalho de Melo wrote: > > Em Thu, May 21, 2020 at 01:43:25PM +0200, Jiri Olsa escreveu: > > > On Wed, May 20, 2020 at 11:20:04AM -0700, Ian Rogers wrote: > > > > > > SNIP > > > > > > > There are 5 out of 12 metric groups where no events are shared, such > > > > as Power, however, disabling grouping of events always reduces the > > > > number of events. > > > > > > > > The result for Memory_BW needs explanation: > > > > > > > > Metric group: Memory_BW > > > > - No merging (old default, now --metric-no-merge): 9 > > > > - Merging over metrics (new default) : 5 > > > > - No event groups and merging (--metric-no-group): 11 > > > > > > > > Both with and without merging the groups fail to be set up and so the > > > > event counts here are for broken metrics. The --metric-no-group number > > > > is accurate as all the events are scheduled. Ideally a constraint > > > > would be added for these metrics in the json code to avoid grouping. > > > > > > > > v2. rebases on kernel/git/acme/linux.git branch tmp.perf/core, fixes a > > > > missing comma with metric lists (reported-by Jiri Olsa > > > > <jolsa@redhat.com>) and adds early returns to metricgroup__add_metric > > > > (suggested-by Jiri Olsa). > > > > > > Acked-by: Jiri Olsa <jolsa@redhat.com> > > > > Applied and pushed to tmp.perf/core, will move to perf/core as soon as > > testing finishes, > > I checked tmp.perf/core and I'm getting segfault for 'perf test expr' Right, reproduced here and... > 7: Simple expression parser : > Program received signal SIGSEGV, Segmentation fault. > 0x000000000067841e in hashmap_find_entry (map=0x7fffffffd0c0, key=0xc83b30, hash=9893851511679796638, pprev=0x0, entry=0x7fffffffc658) at hashmap.c:131 > 131 for (prev_ptr = &map->buckets[hash], cur = *prev_ptr; > (gdb) bt > #0 0x000000000067841e in hashmap_find_entry (map=0x7fffffffd0c0, key=0xc83b30, hash=9893851511679796638, pprev=0x0, entry=0x7fffffffc658) at hashmap.c:131 > #1 0x000000000067853a in hashmap__insert (map=0x7fffffffd0c0, key=0xc83b30, value=0x0, strategy=HASHMAP_SET, old_key=0x7fffffffc718, > old_value=0x7fffffffc710) at hashmap.c:160 > #2 0x00000000005d3209 in hashmap__set (map=0x7fffffffd0c0, key=0xc83b30, value=0x0, old_key=0x7fffffffc718, old_value=0x7fffffffc710) > at /home/jolsa/kernel/linux-perf/tools/perf/util/hashmap.h:107 > #3 0x00000000005d3386 in expr__add_id (ctx=0x7fffffffd0c0, name=0xc83b30 "FOO", val=0) at util/expr.c:45 > #4 0x00000000005d27ee in expr_parse (final_val=0x0, ctx=0x7fffffffd0c0, scanner=0xc87990) at util/expr.y:63 > #5 0x00000000005d35b7 in __expr__parse (val=0x0, ctx=0x7fffffffd0c0, expr=0x75a84b "FOO + BAR + BAZ + BOZO", start=259, runtime=1) at util/expr.c:102 > #6 0x00000000005d36c6 in expr__find_other (expr=0x75a84b "FOO + BAR + BAZ + BOZO", one=0x75a791 "FOO", ctx=0x7fffffffd0c0, runtime=1) at util/expr.c:121 > #7 0x00000000004e3aaf in test__expr (t=0xa7bd40 <generic_tests+384>, subtest=-1) at tests/expr.c:55 > #8 0x00000000004b5651 in run_test (test=0xa7bd40 <generic_tests+384>, subtest=-1) at tests/builtin-test.c:393 > #9 0x00000000004b5787 in test_and_print (t=0xa7bd40 <generic_tests+384>, force_skip=false, subtest=-1) at tests/builtin-test.c:423 > #10 0x00000000004b61c4 in __cmd_test (argc=1, argv=0x7fffffffd7f0, skiplist=0x0) at tests/builtin-test.c:628 > #11 0x00000000004b6911 in cmd_test (argc=1, argv=0x7fffffffd7f0) at tests/builtin-test.c:772 > #12 0x00000000004e977b in run_builtin (p=0xa7eee8 <commands+552>, argc=3, argv=0x7fffffffd7f0) at perf.c:312 > #13 0x00000000004e99e8 in handle_internal_command (argc=3, argv=0x7fffffffd7f0) at perf.c:364 > #14 0x00000000004e9b2f in run_argv (argcp=0x7fffffffd64c, argv=0x7fffffffd640) at perf.c:408 > #15 0x00000000004e9efb in main (argc=3, argv=0x7fffffffd7f0) at perf.c:538 > > attached patch fixes it for me, but I'm not sure this > should be necessary ... applying the patch below makes the segfault go away. Ian, Ack? I can fold it into the patch introducing the problem. - Arnaldo > jirka > > > --- > diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c > index 1cb02ca2b15f..21693fe516c1 100644 > --- a/tools/perf/tests/expr.c > +++ b/tools/perf/tests/expr.c > @@ -52,6 +52,7 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused) > TEST_ASSERT_VAL("missing operand", ret == -1); > > expr__ctx_clear(&ctx); > + expr__ctx_init(&ctx); > TEST_ASSERT_VAL("find other", > expr__find_other("FOO + BAR + BAZ + BOZO", "FOO", > &ctx, 1) == 0); > @@ -64,6 +65,7 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused) > (void **)&val_ptr)); > > expr__ctx_clear(&ctx); > + expr__ctx_init(&ctx); > TEST_ASSERT_VAL("find other", > expr__find_other("EVENT1\\,param\\=?@ + EVENT2\\,param\\=?@", > NULL, &ctx, 3) == 0); > >
On Fri, May 22, 2020 at 7:59 AM Ian Rogers <irogers@google.com> wrote: > > > > On Fri, May 22, 2020, 7:49 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote: >> >> Em Fri, May 22, 2020 at 12:13:11PM +0200, Jiri Olsa escreveu: >> > On Thu, May 21, 2020 at 02:22:35PM -0300, Arnaldo Carvalho de Melo wrote: >> > > Em Thu, May 21, 2020 at 01:43:25PM +0200, Jiri Olsa escreveu: >> > > > On Wed, May 20, 2020 at 11:20:04AM -0700, Ian Rogers wrote: >> > > > >> > > > SNIP >> > > > >> > > > > There are 5 out of 12 metric groups where no events are shared, such >> > > > > as Power, however, disabling grouping of events always reduces the >> > > > > number of events. >> > > > > >> > > > > The result for Memory_BW needs explanation: >> > > > > >> > > > > Metric group: Memory_BW >> > > > > - No merging (old default, now --metric-no-merge): 9 >> > > > > - Merging over metrics (new default) : 5 >> > > > > - No event groups and merging (--metric-no-group): 11 >> > > > > >> > > > > Both with and without merging the groups fail to be set up and so the >> > > > > event counts here are for broken metrics. The --metric-no-group number >> > > > > is accurate as all the events are scheduled. Ideally a constraint >> > > > > would be added for these metrics in the json code to avoid grouping. >> > > > > >> > > > > v2. rebases on kernel/git/acme/linux.git branch tmp.perf/core, fixes a >> > > > > missing comma with metric lists (reported-by Jiri Olsa >> > > > > <jolsa@redhat.com>) and adds early returns to metricgroup__add_metric >> > > > > (suggested-by Jiri Olsa). >> > > > >> > > > Acked-by: Jiri Olsa <jolsa@redhat.com> >> > > >> > > Applied and pushed to tmp.perf/core, will move to perf/core as soon as >> > > testing finishes, >> > >> > I checked tmp.perf/core and I'm getting segfault for 'perf test expr' >> >> Right, reproduced here and... >> >> > 7: Simple expression parser : >> > Program received signal SIGSEGV, Segmentation fault. >> > 0x000000000067841e in hashmap_find_entry (map=0x7fffffffd0c0, key=0xc83b30, hash=9893851511679796638, pprev=0x0, entry=0x7fffffffc658) at hashmap.c:131 >> > 131 for (prev_ptr = &map->buckets[hash], cur = *prev_ptr; >> > (gdb) bt >> > #0 0x000000000067841e in hashmap_find_entry (map=0x7fffffffd0c0, key=0xc83b30, hash=9893851511679796638, pprev=0x0, entry=0x7fffffffc658) at hashmap.c:131 >> > #1 0x000000000067853a in hashmap__insert (map=0x7fffffffd0c0, key=0xc83b30, value=0x0, strategy=HASHMAP_SET, old_key=0x7fffffffc718, >> > old_value=0x7fffffffc710) at hashmap.c:160 >> > #2 0x00000000005d3209 in hashmap__set (map=0x7fffffffd0c0, key=0xc83b30, value=0x0, old_key=0x7fffffffc718, old_value=0x7fffffffc710) >> > at /home/jolsa/kernel/linux-perf/tools/perf/util/hashmap.h:107 >> > #3 0x00000000005d3386 in expr__add_id (ctx=0x7fffffffd0c0, name=0xc83b30 "FOO", val=0) at util/expr.c:45 >> > #4 0x00000000005d27ee in expr_parse (final_val=0x0, ctx=0x7fffffffd0c0, scanner=0xc87990) at util/expr.y:63 >> > #5 0x00000000005d35b7 in __expr__parse (val=0x0, ctx=0x7fffffffd0c0, expr=0x75a84b "FOO + BAR + BAZ + BOZO", start=259, runtime=1) at util/expr.c:102 >> > #6 0x00000000005d36c6 in expr__find_other (expr=0x75a84b "FOO + BAR + BAZ + BOZO", one=0x75a791 "FOO", ctx=0x7fffffffd0c0, runtime=1) at util/expr.c:121 >> > #7 0x00000000004e3aaf in test__expr (t=0xa7bd40 <generic_tests+384>, subtest=-1) at tests/expr.c:55 >> > #8 0x00000000004b5651 in run_test (test=0xa7bd40 <generic_tests+384>, subtest=-1) at tests/builtin-test.c:393 >> > #9 0x00000000004b5787 in test_and_print (t=0xa7bd40 <generic_tests+384>, force_skip=false, subtest=-1) at tests/builtin-test.c:423 >> > #10 0x00000000004b61c4 in __cmd_test (argc=1, argv=0x7fffffffd7f0, skiplist=0x0) at tests/builtin-test.c:628 >> > #11 0x00000000004b6911 in cmd_test (argc=1, argv=0x7fffffffd7f0) at tests/builtin-test.c:772 >> > #12 0x00000000004e977b in run_builtin (p=0xa7eee8 <commands+552>, argc=3, argv=0x7fffffffd7f0) at perf.c:312 >> > #13 0x00000000004e99e8 in handle_internal_command (argc=3, argv=0x7fffffffd7f0) at perf.c:364 >> > #14 0x00000000004e9b2f in run_argv (argcp=0x7fffffffd64c, argv=0x7fffffffd640) at perf.c:408 >> > #15 0x00000000004e9efb in main (argc=3, argv=0x7fffffffd7f0) at perf.c:538 >> > >> > attached patch fixes it for me, but I'm not sure this >> > should be necessary >> >> ... applying the patch below makes the segfault go away. Ian, Ack? I can >> fold it into the patch introducing the problem. > > > I suspect this patch is a memory leak. The underlying issue is likely the outstanding hashmap_clear fix in libbpf. Let me check. > > Thanks, > Ian Tested: $ git checkout -b testing acme/tmp.perf/core $ make ... $ perf test 7 7: Simple expression parser : FAILED! $ git cherry-pick 6bca339175bf [acme-perf-expr-testing 4614bd252003] libbpf: Fix memory leak and possible double-free in hashmap__c lear Author: Andrii Nakryiko <andriin@fb.com> Date: Tue Apr 28 18:21:04 2020 -0700 1 file changed, 7 insertions(+) $ make ... $ perf test 7 7: Simple expression parser : Ok I'd prefer we took the libbpf fix as initializing over the top of the hashmap will leak. This fix is in the tools/perf/util/hashmap.c. Thanks, Ian >> - Arnaldo >> >> > jirka >> > >> > >> > --- >> > diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c >> > index 1cb02ca2b15f..21693fe516c1 100644 >> > --- a/tools/perf/tests/expr.c >> > +++ b/tools/perf/tests/expr.c >> > @@ -52,6 +52,7 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused) >> > TEST_ASSERT_VAL("missing operand", ret == -1); >> > >> > expr__ctx_clear(&ctx); >> > + expr__ctx_init(&ctx); >> > TEST_ASSERT_VAL("find other", >> > expr__find_other("FOO + BAR + BAZ + BOZO", "FOO", >> > &ctx, 1) == 0); >> > @@ -64,6 +65,7 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused) >> > (void **)&val_ptr)); >> > >> > expr__ctx_clear(&ctx); >> > + expr__ctx_init(&ctx); >> > TEST_ASSERT_VAL("find other", >> > expr__find_other("EVENT1\\,param\\=?@ + EVENT2\\,param\\=?@", >> > NULL, &ctx, 3) == 0); >> > >> > >> >> -- >> >> - Arnaldo
On Fri, May 22, 2020 at 10:56:59AM -0700, Ian Rogers wrote: SNIP > >> > #11 0x00000000004b6911 in cmd_test (argc=1, argv=0x7fffffffd7f0) at tests/builtin-test.c:772 > >> > #12 0x00000000004e977b in run_builtin (p=0xa7eee8 <commands+552>, argc=3, argv=0x7fffffffd7f0) at perf.c:312 > >> > #13 0x00000000004e99e8 in handle_internal_command (argc=3, argv=0x7fffffffd7f0) at perf.c:364 > >> > #14 0x00000000004e9b2f in run_argv (argcp=0x7fffffffd64c, argv=0x7fffffffd640) at perf.c:408 > >> > #15 0x00000000004e9efb in main (argc=3, argv=0x7fffffffd7f0) at perf.c:538 > >> > > >> > attached patch fixes it for me, but I'm not sure this > >> > should be necessary > >> > >> ... applying the patch below makes the segfault go away. Ian, Ack? I can > >> fold it into the patch introducing the problem. > > > > > > I suspect this patch is a memory leak. The underlying issue is likely the outstanding hashmap_clear fix in libbpf. Let me check. > > > > Thanks, > > Ian > > Tested: > $ git checkout -b testing acme/tmp.perf/core > $ make ... > $ perf test 7 > 7: Simple expression parser : FAILED! > $ git cherry-pick 6bca339175bf > [acme-perf-expr-testing 4614bd252003] libbpf: Fix memory leak and > possible double-free in hashmap__c yep, it fixes the issue for me, but I see that under different commit number: 229bf8bf4d91 libbpf: Fix memory leak and possible double-free in hashmap__clear jirka > lear > Author: Andrii Nakryiko <andriin@fb.com> > Date: Tue Apr 28 18:21:04 2020 -0700 > 1 file changed, 7 insertions(+) > $ make ... > $ perf test 7 > 7: Simple expression parser : Ok > > I'd prefer we took the libbpf fix as initializing over the top of the > hashmap will leak. This fix is in the tools/perf/util/hashmap.c. > > Thanks, > Ian > > >> - Arnaldo > >> > >> > jirka > >> > > >> > > >> > --- SNIP
Em Sun, May 24, 2020 at 12:19:36AM +0200, Jiri Olsa escreveu: > On Fri, May 22, 2020 at 10:56:59AM -0700, Ian Rogers wrote: > > SNIP > > > >> > #11 0x00000000004b6911 in cmd_test (argc=1, argv=0x7fffffffd7f0) at tests/builtin-test.c:772 > > >> > #12 0x00000000004e977b in run_builtin (p=0xa7eee8 <commands+552>, argc=3, argv=0x7fffffffd7f0) at perf.c:312 > > >> > #13 0x00000000004e99e8 in handle_internal_command (argc=3, argv=0x7fffffffd7f0) at perf.c:364 > > >> > #14 0x00000000004e9b2f in run_argv (argcp=0x7fffffffd64c, argv=0x7fffffffd640) at perf.c:408 > > >> > #15 0x00000000004e9efb in main (argc=3, argv=0x7fffffffd7f0) at perf.c:538 > > >> > > > >> > attached patch fixes it for me, but I'm not sure this > > >> > should be necessary > > >> > > >> ... applying the patch below makes the segfault go away. Ian, Ack? I can > > >> fold it into the patch introducing the problem. > > > > > > > > > I suspect this patch is a memory leak. The underlying issue is likely the outstanding hashmap_clear fix in libbpf. Let me check. > > > > > > Thanks, > > > Ian > > > > Tested: > > $ git checkout -b testing acme/tmp.perf/core > > $ make ... > > $ perf test 7 > > 7: Simple expression parser : FAILED! > > $ git cherry-pick 6bca339175bf > > [acme-perf-expr-testing 4614bd252003] libbpf: Fix memory leak and > > possible double-free in hashmap__c > > yep, it fixes the issue for me, but I see that under different commit number: > > 229bf8bf4d91 libbpf: Fix memory leak and possible double-free in hashmap__clear Ok, so I'll just add a note mentioning that this test will pass as soon as the libbpf fix gets upstream. Thanks, - Arnaldo