Message-ID: <CAP-5=fX_MQBaER86c1UEuSMYGoQ0uv4siv==_ewr6H5xhb0XpA@mail.gmail.com>
Date:   Mon, 9 Jan 2023 12:40:31 -0800
From:   Ian Rogers <irogers@...gle.com>
To:     Jiri Olsa <olsajiri@...il.com>
Cc:     Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mike Leach <mike.leach@...aro.org>,
        linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
        bpf@...r.kernel.org, peterz@...radead.org, mingo@...hat.com,
        mark.rutland@....com, alexander.shishkin@...ux.intel.com,
        namhyung@...nel.org
Subject: Re: [PATCH v3 1/2] perf build: Properly guard libbpf includes

On Mon, Jan 9, 2023 at 11:34 AM Ian Rogers <irogers@...gle.com> wrote:
>
> On Mon, Jan 9, 2023 at 11:29 AM Ian Rogers <irogers@...gle.com> wrote:
> >
> > On Mon, Jan 9, 2023 at 10:37 AM Ian Rogers <irogers@...gle.com> wrote:
> > >
> > > On Mon, Jan 9, 2023 at 10:10 AM Jiri Olsa <olsajiri@...il.com> wrote:
> > > >
> > > > On Mon, Jan 09, 2023 at 12:12:15PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > > Em Fri, Jan 06, 2023 at 11:06:46AM -0800, Ian Rogers escreveu:
> > > > > > So trying to get build-test working on my Debian derived distro is a
> > > > > > PITA with broken feature detection for options I don't normally use.
> > > > >
> > > > > Its really difficult to have perf building with so many dependent
> > > > > libraries, mowing out some should be in order.
> > > > >
> > > > > > I'll try to fix this.
> > > > >
> > > > > Thanks.
> > > > >
> > > > > > In any case I think I've spotted what is really happening here and it
> > > > > > isn't a failure but a feature :-D The build is specifying
> > > > >
> > > > > I get it.
> > > > >
> > > > > > LIBBPF_DYNAMIC=1 which means you get the libbpf headers from
> > > > > > /usr/include. I think the build is trying to do this on a system with
> > > > > > an old libbpf and hence getting the failures above. Previously, even
> > > > > > though we wanted the dynamic headers we still had a -I, this time for
> > > > > > the install_headers version. Now you really are using the system
> > > > > > version and it is broken. This means a few things:
> > > > > > - the libbpf feature test should fail if code like above is going to fail,
> > > > >
> > > > > Agreed.
> > > > >
> > > > > > - we may want to contemplate supporting older libbpfs (I'd rather not),
> > > > >
> > > > > I'd rather require everybody to be up to the latest trends, but I really
> > > > > don't think that is a reasonable expectation.
> > > > >
> > > > > > - does build-test have a way to skip known issues like this?
> > > > >
> > > > > Unsure, Jiri?
> > > >
> > > > I don't think so it just triggers the build, it's up to the features check
> > > > to disable the feature if the library is not compatible with perf code
> > > >
> > > > could we add that specific libbpf call to the libbpf feature check?
> > >
> > > Looking at the failure closer, the failing code is code inside a
> > > feature check trying to workaround the feature not being present. We
> > > need to do something like:
> > >
> > > ```
> > > diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
> > > index 6e9b06cf06ee..a1c3cc230273 100644
> > > --- a/tools/perf/util/bpf-loader.c
> > > +++ b/tools/perf/util/bpf-loader.c
> > > @@ -33,17 +33,18 @@
> > > #include <internal/xyarray.h>
> > >
> > > #ifndef HAVE_LIBBPF_BPF_PROGRAM__SET_INSNS
> > > -int bpf_program__set_insns(struct bpf_program *prog __maybe_unused,
> > > -                          struct bpf_insn *new_insns __maybe_unused,
> > > size_t new_insn_cnt __maybe_un
> > > used)
> > > +static int bpf_program__set_insns(struct bpf_program *prog __maybe_unused,
> > > +                                 struct bpf_insn *new_insns __maybe_unused,
> > > +                                 size_t new_insn_cnt __maybe_unused)
> > > {
> > >        pr_err("%s: not support, update libbpf\n", __func__);
> > >        return -ENOTSUP;
> > > }
> > >
> > > -int libbpf_register_prog_handler(const char *sec __maybe_unused,
> > > -                                 enum bpf_prog_type prog_type __maybe_unused,
> > > -                                 enum bpf_attach_type exp_attach_type
> > > __maybe_unused,
> > > -                                 const struct
> > > libbpf_prog_handler_opts *opts __maybe_unused)
> > > +static int libbpf_register_prog_handler(const char *sec __maybe_unused,
> > > +                                       enum bpf_prog_type prog_type
> > > __maybe_unused,
> > > +                                       enum bpf_attach_type
> > > exp_attach_type __maybe_unused,
> > > +                                       const void *opts __maybe_unused)
> > > {
> > >        pr_err("%s: not support, update libbpf\n", __func__);
> > >        return -ENOTSUP;
> > > ```
> > >
> > > There are some other fixes necessary too. I'll try to write the fuller
> > > patch but I have no means for testing except for undefining
> > > HAVE_LIBBPF_BPF_PROGRAM__SET_INSNS.
> > >
> > > Thanks,
> > > Ian
> >
> > So libbpf_prog_handler_opts is missing in the failing build, this
> > points to a libbpf before 0.8. I'm somewhat concerned that to work
> > around these linkage problems we're adding runtime errors - we may
> > build but the functionality is totally crippled. Is it worth
> > maintaining these broken builds or to just upfront fail the feature
> > test?
> >
> > We can also switch the feature tests for LIBBPF_MAJOR_VERSION and
> > LIBBPF_MINOR_VERSION checks. This would have the property of letting
> > us tie the error messages to what version of libbpf is assumed.
> >
> > In this case we could have a feature test for the libbpf version and
> > if the version is before libbpf 0.8 fail the feature check. A quick
> > way to do this is:
> > ```
> > diff --git a/tools/build/feature/test-libbpf.c
> > b/tools/build/feature/test-libbpf.c
> > index a508756cf4cc..dadd8186b71d 100644
> > --- a/tools/build/feature/test-libbpf.c
> > +++ b/tools/build/feature/test-libbpf.c
> > @@ -1,6 +1,10 @@
> > // SPDX-License-Identifier: GPL-2.0
> > #include <bpf/libbpf.h>
> >
> > +#if (LIBBPF_MAJOR_VERSION == 0) && (LIBBPF_MINOR_VERSION < 8)
> > +#error At least libbpf 0.8 is assumed for Linux tools.
> > +#endif
> > +
> > int main(void)
> > {
> >        return bpf_object__open("test") ? 0 : -1;
> > ```
> >
> > Thanks,
> > Ian
>
> Oh, just to note. While failing the feature test is disappointing for
> a libbpf that isn't very old, we have the newer libbpf to statically
> build in. Developers won't be impacted due to the static route. If you
> are a distro maintainer, you should just update your libbpf. So we
> could just bump the API assumption to 1.0 as I believe that'd have the
> advantage of removing feature tests, workarounds, untested code (like
> what broke here), etc.
>
> What do you think?
>
> Thanks,
> Ian

The removal of pre-libbpf 1.0 support is in:
https://lore.kernel.org/lkml/20230109203424.1157561-1-irogers@google.com/
So my proposal is that for 6.2 these two patches are sufficient. In
perf/core (to be in 6.3) it makes sense to add both sets of patches to
fix the build issue reported here. The problem with that is that some
build tests will fail for 6.2 if they are testing libbpf dynamic with
a libbpf earlier than 1.0. If that is an issue then cherry-picking the
first or all of the pre-libbpf 1.0 removal patches should address it.

Thanks,
Ian

> > > > jirka
> > > >
> > > > >
> > > > > But yeah, previous experiences with Andrii were that we can do not too
> > > > > costly feature checks, not using .c programs that would fail if some
> > > > > required feature wasn't present but instead would just do some grep on a
> > > > > header and if some "smell" wasn't scent, just fail the cap query.
> > > > >
> > > > > - Arnaldo

Powered by blists - more mailing lists