lkml.org 
[lkml]   [2020]   [May]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v3 7/7] perf expr: Migrate expr ids table to a hashmap
Em Mon, May 18, 2020 at 09:03:45AM -0700, Ian Rogers escreveu:
> On Mon, May 18, 2020 at 8:45 AM Arnaldo Carvalho de Melo
> <arnaldo.melo@gmail.com> wrote:
> >
> > Em Fri, May 15, 2020 at 03:17:32PM -0700, Ian Rogers escreveu:
> > > Use a hashmap between a char* string and a double* value. While bpf's
> > > hashmap entries are size_t in size, we can't guarantee sizeof(size_t) >=
> > > sizeof(double). Avoid a memory allocation when gathering ids by making 0.0
> > > a special value encoded as NULL.
> > >
> > > Original map suggestion by Andi Kleen:
> > > https://lore.kernel.org/lkml/20200224210308.GQ160988@tassilo.jf.intel.com/
> > > and seconded by Jiri Olsa:
> > > https://lore.kernel.org/lkml/20200423112915.GH1136647@krava/
> >
> > I'm having trouble here when building it with:
> >
> > make -C tools/perf O=/tmp/build/perf
> >
> > CC /tmp/build/perf/tests/expr.o
> > INSTALL trace_plugins
> > CC /tmp/build/perf/util/metricgroup.o
> > In file included from /home/acme/git/perf/tools/lib/bpf/hashmap.h:18,
> > from /home/acme/git/perf/tools/perf/util/expr.h:6,
> > from tests/expr.c:3:
> > /home/acme/git/perf/tools/lib/bpf/libbpf_internal.h:63: error: "pr_info" redefined [-Werror]
> > 63 | #define pr_info(fmt, ...) __pr(LIBBPF_INFO, fmt, ##__VA_ARGS__)
> > |
> > In file included from tests/expr.c:2:
> > /home/acme/git/perf/tools/perf/util/debug.h:24: note: this is the location of the previous definition
> >
> > It looks like libbpf's hashmap.h is being used instead of the one in
> > tools/perf/util/, yeah, as intended, but then since I don't have the
> > fixes you added to the BPF tree, the build fails, if I instead
> > unconditionally use
> >
> > #include "util/hashmap.h"
> >
> > It works. Please ack.
> >
> > I.e. with the patch below, further tests:
> >
> > [acme@five perf]$ perf -vv | grep -i bpf
> > bpf: [ on ] # HAVE_LIBBPF_SUPPORT
> > [acme@five perf]$ nm ~/bin/perf | grep -i libbpf_ | wc -l
> > 39
> > [acme@five perf]$ nm ~/bin/perf | grep -i hashmap_ | wc -l
> > 17
> > [acme@five perf]$
> >
> > Explicitely building without LIBBPF:
> >
> > [acme@five perf]$ perf -vv | grep -i bpf
> > bpf: [ OFF ] # HAVE_LIBBPF_SUPPORT
> > [acme@five perf]$
> > [acme@five perf]$ nm ~/bin/perf | grep -i libbpf_ | wc -l
> > 0
> > [acme@five perf]$ nm ~/bin/perf | grep -i hashmap_ | wc -l
> > 9
> > [acme@five perf]$
> >
> > Works,
> >
> > - Arnaldo
>
> Hi Arnaldo,
>
> this build issue sounds like this patch is missing:
> https://lore.kernel.org/lkml/20200515221732.44078-3-irogers@google.com/
> The commit message there could have explicitly said having this
> #include causes the conflicting definitions between perf's debug.h and
> libbpf_internal.h's definitions of pr_info, etc.

yeah, understood, but I'm not processing patches for tools/lib/bpf/,
Daniel is, I'll only get that one later, then we can go back to the way
you structured it. Just an extra bit of confusion in this process ;-)

> Let me know how else to help and sorry for the confusion. Thanks,
> Ian
>
>
> > diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> > index d60a8feaf50b..8a2c1074f90f 100644
> > --- a/tools/perf/util/expr.h
> > +++ b/tools/perf/util/expr.h
> > @@ -2,11 +2,14 @@
> > #ifndef PARSE_CTX_H
> > #define PARSE_CTX_H 1
> >
> > -#ifdef HAVE_LIBBPF_SUPPORT
> > -#include <bpf/hashmap.h>
> > -#else
> > -#include "hashmap.h"
> > -#endif
> > +// There are fixes that need to land upstream before we can use libbpf's headers,
> > +// for now use our copy unconditionally, since the data structures at this point
> > +// are exactly the same, no problem.
> > +//#ifdef HAVE_LIBBPF_SUPPORT
> > +//#include <bpf/hashmap.h>
> > +//#else
> > +#include "util/hashmap.h"
> > +//#endif
> >
> > struct expr_parse_ctx {
> > struct hashmap ids;

--

- Arnaldo

\
 
 \ /
  Last update: 2020-05-18 18:15    [W:0.093 / U:0.532 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site