From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Quentin Monnet <quentin@isovalent.com>
Cc: "Michal Suchánek" <msuchanek@suse.de>,
"Alexander Lobakin" <aleksander.lobakin@intel.com>,
"Alexander Lobakin" <alobakin@mailbox.org>,
"Alexei Starovoitov" <ast@kernel.org>,
"Shung-Hsi Yu" <shung-hsi.yu@suse.com>,
"Daniel Borkmann" <daniel@iogearbox.net>,
"Andrii Nakryiko" <andrii@kernel.org>,
"Maciej Fijalkowski" <maciej.fijalkowski@intel.com>,
"Song Liu" <songliubraving@fb.com>,
"Kumar Kartikeya Dwivedi" <memxor@gmail.com>,
bpf@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 bpf 02/11] bpftool: define a local bpf_perf_link to fix accessing its fields
Date: Wed, 3 May 2023 16:52:15 -0700 [thread overview]
Message-ID: <CAEf4BzZgWwPJCkw1XKY03Vtb6B4_iE3dBbjqMB=upM0O=OYRXQ@mail.gmail.com> (raw)
In-Reply-To: <CACdoK4+KdM-sQKMO9WXk7kTL-x=Renjd0MuvSRT3JKbtzByyHQ@mail.gmail.com>
On Wed, May 3, 2023 at 4:44 PM Quentin Monnet <quentin@isovalent.com> wrote:
>
> On Fri, 21 Apr 2023 at 08:39, Michal Suchánek <msuchanek@suse.de> wrote:
> >
> > On Thu, Apr 20, 2023 at 04:07:38PM -0700, Andrii Nakryiko wrote:
> > > On Fri, Apr 14, 2023 at 9:28 AM Michal Suchánek <msuchanek@suse.de> wrote:
> > > >
> > > > On Fri, Apr 14, 2023 at 05:18:27PM +0200, Alexander Lobakin wrote:
> > > > > From: Michal Suchánek <msuchanek@suse.de>
> > > > > Date: Fri, 14 Apr 2023 11:54:57 +0200
> > > > >
> > > > > > Hello,
> > > > >
> > > > > Hey-hey,
> > > > >
> > > > > >
> > > > > > On Thu, Apr 21, 2022 at 12:38:58AM +0000, Alexander Lobakin wrote:
> > > > > >> When building bpftool with !CONFIG_PERF_EVENTS:
> > > > > >>
> > > > > >> skeleton/pid_iter.bpf.c:47:14: error: incomplete definition of type 'struct bpf_perf_link'
> > > > > >> perf_link = container_of(link, struct bpf_perf_link, link);
> > > > > >> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > > >> tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helpers.h:74:22: note: expanded from macro 'container_of'
> > > > > >> ((type *)(__mptr - offsetof(type, member))); \
> > > > > >> ^~~~~~~~~~~~~~~~~~~~~~
> > > > > >> tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helpers.h:68:60: note: expanded from macro 'offsetof'
> > > > > >> #define offsetof(TYPE, MEMBER) ((unsigned long)&((TYPE *)0)->MEMBER)
> > > > > >> ~~~~~~~~~~~^
> > > > > >> skeleton/pid_iter.bpf.c:44:9: note: forward declaration of 'struct bpf_perf_link'
> > > > > >> struct bpf_perf_link *perf_link;
> > > > > >> ^
> > > > > >>
> > > > > >> &bpf_perf_link is being defined and used only under the ifdef.
> > > > > >> Define struct bpf_perf_link___local with the `preserve_access_index`
> > > > > >> attribute inside the pid_iter BPF prog to allow compiling on any
> > > > > >> configs. CO-RE will substitute it with the real struct bpf_perf_link
> > > > > >> accesses later on.
> > > > > >> container_of() is not CO-REd, but it is a noop for
> > > > > >> bpf_perf_link <-> bpf_link and the local copy is a full mirror of
> > > > > >> the original structure.
> > > > > >>
> > > > > >> Fixes: cbdaf71f7e65 ("bpftool: Add bpf_cookie to link output")
> > > > > >
> > > > > > This does not solve the problem completely. Kernels that don't have
> > > > > > CONFIG_PERF_EVENTS in the first place are also missing the enum value
> > > > > > BPF_LINK_TYPE_PERF_EVENT which is used as the condition for handling the
> > > > > > cookie.
> > > > >
> > > > > Sorry, I haven't been working with my home/private stuff for more than a
> > > > > year already. I may get back to it some day when I'm tired of Lua (curse
> > > > > words, sorry :D), but for now the series is "a bit" abandoned.
> > > >
> > > > This part still appllies and works for me with the caveat that
> > > > BPF_LINK_TYPE_PERF_EVENT also needs to be defined.
> > > >
> > > > > I think there was alternative solution proposed there, which promised to
> > > > > be more flexible. But IIRC it also doesn't touch the enum (was it added
> > > > > recently? Because it was building just fine a year ago on config without
> > > > > perf events).
> > > >
> > > > It was added in 5.15. Not sure there is a kernel.org LTS kernel usable
> > > > for CO-RE that does not have it, technically 5.4 would work if it was
> > > > built monolithic, it does not have module BTF, only kernel IIRC.
> > > >
> > > > Nonetheless, the approach to handling features completely missing in the
> > > > running kernel should be figured out one way or another. I would be
> > > > surprised if this was the last feature to be added that bpftool needs to
> > > > know about.
> > >
> > > Are we talking about bpftool built from kernel sources or from Github?
> > > Kernel source version should have access to latest UAPI headers and so
> > > BPF_LINK_TYPE_PERF_EVENT should be available. Github version, if it
> > > doesn't do that already, can use UAPI headers distributed (and used
> > > for building) with libbpf through submodule.
> >
> > It does have a copy of the uapi headers but apparently does not use
> > them. Using them directly might cause conflict with vmlinux.h, though.
>
> Indeed, using the UAPI header here conflicts with vmlinux.h.
>
> Looking again at some code I started last year but never finalised, I
> used the following approach, redefining BPF_LINK_TYPE_PERF_EVENT with
> CO-RE:
>
> enum bpf_link_type___local {
> BPF_LINK_TYPE_PERF_EVENT___local = 7,
> };
>
> Then guarding accordingly in iter():
>
> [...]
> if (obj_type == BPF_OBJ_LINK &&
> bpf_core_enum_value_exists(enum bpf_link_type___local,
> BPF_LINK_TYPE_PERF_EVENT___local)) {
> struct bpf_link *link = (struct bpf_link *) file->private_data;
>
> if (link->type == bpf_core_enum_value(enum bpf_link_type___local,
> BPF_LINK_TYPE_PERF_EVENT___local)) {
> e.has_bpf_cookie = true;
> e.bpf_cookie = get_bpf_cookie(link);
> }
> }
> [...]
>
> Would that approach make sense? I had a VM around with kernel 5.8, and
> bpftool compiles there with that change. If I remember correctly, some
> older kernel versions required yet more CO-RE work in pid_iter.bpf.c,
> and at some point I was struggling, which is why I never submitted
> this set.
>
> If this approach looks correct to you Andrii, I can resubmit these
> patches with my addition so we can at least fix the build on 5.8
> onwards.
Yep, why not? In general, if using vmlinux.h makes life harder and
there is just a small set of types and enums BPF program needs, for
the sake of support of old kernels/distros it might be cleaner just to
define relevant structs, enums, etc explicitly and add
__attribute__((preserve_access_index)) to them to make the
CO-RE-relocatable
>
> Thanks,
> Quentin
next prev parent reply other threads:[~2023-05-03 23:52 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-21 0:38 [PATCH v2 bpf 00/11] bpf: random unpopular userspace fixes (32 bit et al) Alexander Lobakin
2022-04-21 0:38 ` [PATCH v2 bpf 01/11] bpftool: use a local copy of perf_event to fix accessing ::bpf_cookie Alexander Lobakin
2022-04-21 0:38 ` [PATCH v2 bpf 02/11] bpftool: define a local bpf_perf_link to fix accessing its fields Alexander Lobakin
2023-04-14 9:54 ` Michal Suchánek
2023-04-14 15:18 ` Alexander Lobakin
2023-04-14 16:28 ` Michal Suchánek
2023-04-20 23:07 ` Andrii Nakryiko
2023-04-21 7:39 ` Michal Suchánek
2023-05-03 23:43 ` Quentin Monnet
2023-05-03 23:52 ` Andrii Nakryiko [this message]
2023-05-04 8:18 ` Michal Suchánek
2023-05-04 16:48 ` Yonghong Song
2022-04-21 0:39 ` [PATCH v2 bpf 03/11] bpftool: use a local bpf_perf_event_value " Alexander Lobakin
2023-06-06 21:02 ` Nick Desaulniers
2023-06-07 10:42 ` Quentin Monnet
2023-06-07 16:18 ` Alexander Lobakin
2022-04-21 0:39 ` [PATCH v2 bpf 04/11] bpftool: fix fcntl.h include Alexander Lobakin
2022-04-21 0:39 ` [PATCH v2 bpf 05/11] samples/bpf: add 'asm/mach-generic' include path for every MIPS Alexander Lobakin
2022-04-21 0:39 ` [PATCH v2 bpf 06/11] samples/bpf: use host bpftool to generate vmlinux.h, not target Alexander Lobakin
2022-04-21 0:39 ` [PATCH v2 bpf 07/11] samples/bpf: fix uin64_t format literals Alexander Lobakin
2022-04-21 7:46 ` David Laight
2022-04-21 22:55 ` Alexander Lobakin
2022-04-21 0:39 ` [PATCH v2 bpf 08/11] samples/bpf: fix false-positive right-shift underflow warnings Alexander Lobakin
2022-04-21 0:39 ` [PATCH v2 bpf 09/11] samples/bpf: fix include order for non-Glibc environments Alexander Lobakin
2022-04-21 0:39 ` [PATCH v2 bpf 10/11] samples/bpf: fix -Wsequence-point Alexander Lobakin
2022-04-21 0:39 ` [PATCH v2 bpf 11/11] samples/bpf: xdpsock: fix -Wmaybe-uninitialized Alexander Lobakin
2022-04-21 0:40 ` [PATCH v2 bpf 00/11] bpf: random unpopular userspace fixes (32 bit et al) Alexei Starovoitov
2022-04-21 10:52 ` Toke Høiland-Jørgensen
2022-04-21 22:39 ` Alexander Lobakin
2022-04-21 23:17 ` Toke Høiland-Jørgensen
2022-05-03 21:17 ` Alexander Lobakin
2022-05-04 11:34 ` Toke Høiland-Jørgensen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAEf4BzZgWwPJCkw1XKY03Vtb6B4_iE3dBbjqMB=upM0O=OYRXQ@mail.gmail.com' \
--to=andrii.nakryiko@gmail.com \
--cc=aleksander.lobakin@intel.com \
--cc=alobakin@mailbox.org \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=linux-kernel@vger.kernel.org \
--cc=maciej.fijalkowski@intel.com \
--cc=memxor@gmail.com \
--cc=msuchanek@suse.de \
--cc=netdev@vger.kernel.org \
--cc=quentin@isovalent.com \
--cc=shung-hsi.yu@suse.com \
--cc=songliubraving@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).