| Message ID | 20200515165007.217120-1-irogers@google.com |
|---|---|
| Headers | show |
| Series | Copy hashmap to tools/perf/util, use in perf expr | expand |
Em Fri, May 15, 2020 at 09:50:00AM -0700, Ian Rogers escreveu: > Perf's expr code currently builds an array of strings then removes > duplicates. The array is larger than necessary and has recently been > increased in size. When this was done it was commented that a hashmap > would be preferable. > > libbpf has a hashmap but libbpf isn't currently required to build > perf. To satisfy various concerns this change copies libbpf's hashmap > into tools/perf/util, it then adds a check in perf that the two are in > sync. > > Andrii's patch to hashmap from bpf-next is brought into this set to > fix issues with hashmap__clear. > > Two minor changes to libbpf's hashmap are made that remove an unused > dependency and fix a compiler warning. Andrii/Alexei/Daniel, what do you think about me merging these fixes in my perf-tools-next branch? - Arnaldo > Two perf test changes are also brought in as they need refactoring to > account for the expr API change and it is expected they will land > ahead of this. > https://lore.kernel.org/lkml/20200513062236.854-2-irogers@google.com/ > > Tested with 'perf test' and 'make -C tools/perf build-test'. > > The hashmap change was originally part of an RFC: > https://lore.kernel.org/lkml/20200508053629.210324-1-irogers@google.com/ > > v2. moves hashmap into tools/perf/util rather than libapi, to allow > hashmap's libbpf symbols to be visible when built statically for > testing. > > Andrii Nakryiko (1): > libbpf: Fix memory leak and possible double-free in hashmap__clear > > Ian Rogers (6): > libbpf hashmap: Remove unused #include > libbpf hashmap: Fix signedness warnings > tools lib/api: Copy libbpf hashmap to tools/perf/util > perf test: Provide a subtest callback to ask for the reason for > skipping a subtest > perf test: Improve pmu event metric testing > perf expr: Migrate expr ids table to a hashmap > > tools/lib/bpf/hashmap.c | 10 +- > tools/lib/bpf/hashmap.h | 1 - > tools/perf/check-headers.sh | 4 + > tools/perf/tests/builtin-test.c | 18 ++- > tools/perf/tests/expr.c | 40 +++--- > tools/perf/tests/pmu-events.c | 169 ++++++++++++++++++++++- > tools/perf/tests/tests.h | 4 + > tools/perf/util/Build | 4 + > tools/perf/util/expr.c | 129 +++++++++-------- > tools/perf/util/expr.h | 26 ++-- > tools/perf/util/expr.y | 22 +-- > tools/perf/util/hashmap.c | 238 ++++++++++++++++++++++++++++++++ > tools/perf/util/hashmap.h | 177 ++++++++++++++++++++++++ > tools/perf/util/metricgroup.c | 87 ++++++------ > tools/perf/util/stat-shadow.c | 49 ++++--- > 15 files changed, 798 insertions(+), 180 deletions(-) > create mode 100644 tools/perf/util/hashmap.c > create mode 100644 tools/perf/util/hashmap.h > > -- > 2.26.2.761.g0e0b3e54be-goog >
On Fri, May 15, 2020 at 10:01 AM Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> wrote: > > Em Fri, May 15, 2020 at 09:50:00AM -0700, Ian Rogers escreveu: > > Perf's expr code currently builds an array of strings then removes > > duplicates. The array is larger than necessary and has recently been > > increased in size. When this was done it was commented that a hashmap > > would be preferable. > > > > libbpf has a hashmap but libbpf isn't currently required to build > > perf. To satisfy various concerns this change copies libbpf's hashmap > > into tools/perf/util, it then adds a check in perf that the two are in > > sync. > > > > Andrii's patch to hashmap from bpf-next is brought into this set to > > fix issues with hashmap__clear. > > > > Two minor changes to libbpf's hashmap are made that remove an unused > > dependency and fix a compiler warning. > > Andrii/Alexei/Daniel, what do you think about me merging these fixes in my > perf-tools-next branch? I'm ok with the idea, but it's up to maintainers to coordinate this :) > > - Arnaldo > > > Two perf test changes are also brought in as they need refactoring to > > account for the expr API change and it is expected they will land > > ahead of this. > > https://lore.kernel.org/lkml/20200513062236.854-2-irogers@google.com/ > > > > Tested with 'perf test' and 'make -C tools/perf build-test'. > > > > The hashmap change was originally part of an RFC: > > https://lore.kernel.org/lkml/20200508053629.210324-1-irogers@google.com/ > > > > v2. moves hashmap into tools/perf/util rather than libapi, to allow > > hashmap's libbpf symbols to be visible when built statically for > > testing. > > > > Andrii Nakryiko (1): > > libbpf: Fix memory leak and possible double-free in hashmap__clear > > > > Ian Rogers (6): > > libbpf hashmap: Remove unused #include > > libbpf hashmap: Fix signedness warnings > > tools lib/api: Copy libbpf hashmap to tools/perf/util > > perf test: Provide a subtest callback to ask for the reason for > > skipping a subtest > > perf test: Improve pmu event metric testing > > perf expr: Migrate expr ids table to a hashmap > > > > tools/lib/bpf/hashmap.c | 10 +- > > tools/lib/bpf/hashmap.h | 1 - > > tools/perf/check-headers.sh | 4 + > > tools/perf/tests/builtin-test.c | 18 ++- > > tools/perf/tests/expr.c | 40 +++--- > > tools/perf/tests/pmu-events.c | 169 ++++++++++++++++++++++- > > tools/perf/tests/tests.h | 4 + > > tools/perf/util/Build | 4 + > > tools/perf/util/expr.c | 129 +++++++++-------- > > tools/perf/util/expr.h | 26 ++-- > > tools/perf/util/expr.y | 22 +-- > > tools/perf/util/hashmap.c | 238 ++++++++++++++++++++++++++++++++ > > tools/perf/util/hashmap.h | 177 ++++++++++++++++++++++++ > > tools/perf/util/metricgroup.c | 87 ++++++------ > > tools/perf/util/stat-shadow.c | 49 ++++--- > > 15 files changed, 798 insertions(+), 180 deletions(-) > > create mode 100644 tools/perf/util/hashmap.c > > create mode 100644 tools/perf/util/hashmap.h > > > > -- > > 2.26.2.761.g0e0b3e54be-goog > > > > -- > > - Arnaldo
<bpf@vger.kernel.org>,Stephane Eranian <eranian@google.com> From: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> Message-ID: <79BCBAF7-BF5F-4556-A923-56E9D82FB570@gmail.com> On May 15, 2020 4:42:46 PM GMT-03:00, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: >On Fri, May 15, 2020 at 10:01 AM Arnaldo Carvalho de Melo ><arnaldo.melo@gmail.com> wrote: >> >> Em Fri, May 15, 2020 at 09:50:00AM -0700, Ian Rogers escreveu: >> > Perf's expr code currently builds an array of strings then removes >> > duplicates. The array is larger than necessary and has recently >been >> > increased in size. When this was done it was commented that a >hashmap >> > would be preferable. >> > >> > libbpf has a hashmap but libbpf isn't currently required to build >> > perf. To satisfy various concerns this change copies libbpf's >hashmap >> > into tools/perf/util, it then adds a check in perf that the two are >in >> > sync. >> > >> > Andrii's patch to hashmap from bpf-next is brought into this set to >> > fix issues with hashmap__clear. >> > >> > Two minor changes to libbpf's hashmap are made that remove an >unused >> > dependency and fix a compiler warning. >> >> Andrii/Alexei/Daniel, what do you think about me merging these fixes >in my >> perf-tools-next branch? > >I'm ok with the idea, but it's up to maintainers to coordinate this :) Good to know, do I'll take all patches except the ones touching libppf, will just make sure the copy is done with the patches applied. At some point they'll land in libbpf and the warning from check_headers.sh will be resolved. Thanks, - Arnaldo
On Fri, May 15, 2020 at 2:19 PM <arnaldo.melo@gmail.com> wrote: > > <bpf@vger.kernel.org>,Stephane Eranian <eranian@google.com> > From: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> > Message-ID: <79BCBAF7-BF5F-4556-A923-56E9D82FB570@gmail.com> > > > > On May 15, 2020 4:42:46 PM GMT-03:00, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > >On Fri, May 15, 2020 at 10:01 AM Arnaldo Carvalho de Melo > ><arnaldo.melo@gmail.com> wrote: > >> > >> Em Fri, May 15, 2020 at 09:50:00AM -0700, Ian Rogers escreveu: > >> > Perf's expr code currently builds an array of strings then removes > >> > duplicates. The array is larger than necessary and has recently > >been > >> > increased in size. When this was done it was commented that a > >hashmap > >> > would be preferable. > >> > > >> > libbpf has a hashmap but libbpf isn't currently required to build > >> > perf. To satisfy various concerns this change copies libbpf's > >hashmap > >> > into tools/perf/util, it then adds a check in perf that the two are > >in > >> > sync. > >> > > >> > Andrii's patch to hashmap from bpf-next is brought into this set to > >> > fix issues with hashmap__clear. > >> > > >> > Two minor changes to libbpf's hashmap are made that remove an > >unused > >> > dependency and fix a compiler warning. > >> > >> Andrii/Alexei/Daniel, what do you think about me merging these fixes > >in my > >> perf-tools-next branch? > > > >I'm ok with the idea, but it's up to maintainers to coordinate this :) > > Good to know, do I'll take all patches except the ones touching libppf, will just make sure the copy is done with the patches applied. > > At some point they'll land in libbpf and the warning from check_headers.sh will be resolved. So tools/perf/util's hashmap will be ahead of libbpf's, as without the fixes the perf build is broken by Werror. This will cause check_headers to warn in perf builds, which would usually mean our header was older than the source one, but in this case it means the opposite, we're waiting for the libbpf patches to merge. Aside from some interim warnings everything will resolve itself and Arnaldo avoids landing patches in libbpf that can interfere with bpf-next. It takes some getting your head around but sounds good to me :-) I think the only workable alternatives would be to explore having a single version of the code in some kind of shared libhashmap or to implement another hashmap in libapi. I'd like to get the rest of this work unblocked and so it'd be nice to land this and we can always refactor later - I like Arnaldo's plan. Can a bpf maintainer make sure the hashmap changes get pulled into bpf-next? Thanks! Ian > Thanks, > > - Arnaldo > > -- > Sent from my Android device with K-9 Mail. Please excuse my brevity.
On 5/15/20 11:18 PM, arnaldo.melo@gmail.com wrote: [...] >>> Andrii/Alexei/Daniel, what do you think about me merging these fixes >> in my >>> perf-tools-next branch? >> >> I'm ok with the idea, but it's up to maintainers to coordinate this :) > > Good to know, do I'll take all patches except the ones touching libppf, will just make sure the copy is done with the patches applied. > > At some point they'll land in libbpf and the warning from check_headers.sh will be resolved. Works for me, I've just taken in Ian's two libbpf ones into bpf-next. Thanks everyone, Daniel
Perf's expr code currently builds an array of strings then removes duplicates. The array is larger than necessary and has recently been increased in size. When this was done it was commented that a hashmap would be preferable. libbpf has a hashmap but libbpf isn't currently required to build perf. To satisfy various concerns this change copies libbpf's hashmap into tools/perf/util, it then adds a check in perf that the two are in sync. Andrii's patch to hashmap from bpf-next is brought into this set to fix issues with hashmap__clear. Two minor changes to libbpf's hashmap are made that remove an unused dependency and fix a compiler warning. Two perf test changes are also brought in as they need refactoring to account for the expr API change and it is expected they will land ahead of this. https://lore.kernel.org/lkml/20200513062236.854-2-irogers@google.com/ Tested with 'perf test' and 'make -C tools/perf build-test'. The hashmap change was originally part of an RFC: https://lore.kernel.org/lkml/20200508053629.210324-1-irogers@google.com/ v2. moves hashmap into tools/perf/util rather than libapi, to allow hashmap's libbpf symbols to be visible when built statically for testing. Andrii Nakryiko (1): libbpf: Fix memory leak and possible double-free in hashmap__clear Ian Rogers (6): libbpf hashmap: Remove unused #include libbpf hashmap: Fix signedness warnings tools lib/api: Copy libbpf hashmap to tools/perf/util perf test: Provide a subtest callback to ask for the reason for skipping a subtest perf test: Improve pmu event metric testing perf expr: Migrate expr ids table to a hashmap tools/lib/bpf/hashmap.c | 10 +- tools/lib/bpf/hashmap.h | 1 - tools/perf/check-headers.sh | 4 + tools/perf/tests/builtin-test.c | 18 ++- tools/perf/tests/expr.c | 40 +++--- tools/perf/tests/pmu-events.c | 169 ++++++++++++++++++++++- tools/perf/tests/tests.h | 4 + tools/perf/util/Build | 4 + tools/perf/util/expr.c | 129 +++++++++-------- tools/perf/util/expr.h | 26 ++-- tools/perf/util/expr.y | 22 +-- tools/perf/util/hashmap.c | 238 ++++++++++++++++++++++++++++++++ tools/perf/util/hashmap.h | 177 ++++++++++++++++++++++++ tools/perf/util/metricgroup.c | 87 ++++++------ tools/perf/util/stat-shadow.c | 49 ++++--- 15 files changed, 798 insertions(+), 180 deletions(-) create mode 100644 tools/perf/util/hashmap.c create mode 100644 tools/perf/util/hashmap.h