Re: [PATCH v2 bpf 02/11] bpftool: define a local bpf_perf_link to fix accessing its fields

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 5/4/23 1:18 AM, Michal Suchánek wrote:
Hello,

On Thu, May 04, 2023 at 12:43:52AM +0100, Quentin Monnet wrote:
On Fri, 21 Apr 2023 at 08:39, Michal Suchánek <msuchanek@xxxxxxx> 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@xxxxxxx> wrote:

On Fri, Apr 14, 2023 at 05:18:27PM +0200, Alexander Lobakin wrote:
From: Michal Suchánek <msuchanek@xxxxxxx>
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,
     };

That's the same as I did except I used simple define instead of this
fake enum.

The enum only has value when it is complete and the compiler can check
that a switch uses only known values, and can confuse things when values
are missing.

Currently, enum value CORE is done though a llvm builtin function. So
if the enum value is used in switch cases like
  switch(...)
  case BPF_LINK_TYPE_PERF_EVENT:
     ...
CORE relocation will not work in that case since the compiler
expects BPF_LINK_TYPE_PERF_EVENT to be a constant.

Thanks

Michal



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux