Message-ID: <Y71J2WpaMtked02+@krava>
Date:   Tue, 10 Jan 2023 12:19:53 +0100
From:   Jiri Olsa <olsajiri@...il.com>
To:     Ian Rogers <irogers@...gle.com>
Cc:     Jiri Olsa <olsajiri@...il.com>,
        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 09, 2023 at 11:34:44AM -0800, Ian Rogers 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?

yes, seems good.. fedora has libbpf 1.0 already so should not be problem
there at least ;-)

thanks,
jirka

Powered by blists - more mailing lists