Message-ID: <CAP-5=fVtMEahrPMJNVOqsZGj-GoTxUnWsdzK7M+9F2+DhG_STQ@mail.gmail.com>
Date:   Mon, 9 Jan 2023 11:34:44 -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: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

> > > 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