netdev - Re: [PATCH v3 7/7] perf expr: Migrate expr ids table to a hashmap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAP-5=fWZwuSLaFX+-pgeE_H92Mtp7+_NrwBeRFTqyfPjVRkbWg@mail.gmail.com>
Date:   Mon, 18 May 2020 09:03:45 -0700
From:   Ian Rogers <irogers@...gle.com>
To:     Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andriin@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>,
        Kajol Jain <kjain@...ux.ibm.com>,
        Andi Kleen <ak@...ux.intel.com>,
        John Garry <john.garry@...wei.com>,
        Jin Yao <yao.jin@...ux.intel.com>,
        Kan Liang <kan.liang@...ux.intel.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Kim Phillips <kim.phillips@....com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Leo Yan <leo.yan@...aro.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH v3 7/7] perf expr: Migrate expr ids table to a hashmap

On Mon, May 18, 2020 at 8:45 AM Arnaldo Carvalho de Melo
<arnaldo.melo@...il.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@...e perf]$ perf -vv | grep -i bpf
>                    bpf: [ on  ]  # HAVE_LIBBPF_SUPPORT
> [acme@...e perf]$ nm ~/bin/perf | grep -i libbpf_ | wc -l
> 39
> [acme@...e perf]$ nm ~/bin/perf | grep -i hashmap_ | wc -l
> 17
> [acme@...e perf]$
>
> Explicitely building without LIBBPF:
>
> [acme@...e perf]$ perf -vv | grep -i bpf
>                    bpf: [ OFF ]  # HAVE_LIBBPF_SUPPORT
> [acme@...e perf]$
> [acme@...e perf]$ nm ~/bin/perf | grep -i libbpf_ | wc -l
> 0
> [acme@...e perf]$ nm ~/bin/perf | grep -i hashmap_ | wc -l
> 9
> [acme@...e 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.

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;