* [PATCH bpf-next 1/4] bpftool: use a local copy of perf_event to fix accessing ::bpf_cookie
2023-05-12 10:33 [PATCH bpf-next 0/4] bpftool: Fix skeletons compilation for older kernels Quentin Monnet
@ 2023-05-12 10:33 ` Quentin Monnet
2023-05-12 10:33 ` [PATCH bpf-next 2/4] bpftool: define a local bpf_perf_link to fix accessing its fields Quentin Monnet
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: Quentin Monnet @ 2023-05-12 10:33 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf,
Quentin Monnet, Alexander Lobakin, Michal Suchánek,
Alexander Lobakin
From: Alexander Lobakin <alobakin@pm.me>
When CONFIG_PERF_EVENTS is not set, struct perf_event remains empty.
However, the structure is being used by bpftool indirectly via BTF.
This leads to:
skeleton/pid_iter.bpf.c:49:30: error: no member named 'bpf_cookie' in 'struct perf_event'
return BPF_CORE_READ(event, bpf_cookie);
~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
...
skeleton/pid_iter.bpf.c:49:9: error: returning 'void' from a function with incompatible result type '__u64' (aka 'unsigned long long')
return BPF_CORE_READ(event, bpf_cookie);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Tools and samples can't use any CONFIG_ definitions, so the fields
used there should always be present.
Define struct perf_event___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 perf_event
accesses later on.
Fixes: cbdaf71f7e65 ("bpftool: Add bpf_cookie to link output")
Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Alexander Lobakin <alobakin@pm.me>
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
---
tools/bpf/bpftool/skeleton/pid_iter.bpf.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
index eb05ea53afb1..e2af8e5fb29e 100644
--- a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
+++ b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
@@ -15,6 +15,10 @@ enum bpf_obj_type {
BPF_OBJ_BTF,
};
+struct perf_event___local {
+ u64 bpf_cookie;
+} __attribute__((preserve_access_index));
+
extern const void bpf_link_fops __ksym;
extern const void bpf_map_fops __ksym;
extern const void bpf_prog_fops __ksym;
@@ -41,8 +45,8 @@ static __always_inline __u32 get_obj_id(void *ent, enum bpf_obj_type type)
/* could be used only with BPF_LINK_TYPE_PERF_EVENT links */
static __u64 get_bpf_cookie(struct bpf_link *link)
{
+ struct perf_event___local *event;
struct bpf_perf_link *perf_link;
- struct perf_event *event;
perf_link = container_of(link, struct bpf_perf_link, link);
event = BPF_CORE_READ(perf_link, perf_file, private_data);
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH bpf-next 2/4] bpftool: define a local bpf_perf_link to fix accessing its fields
2023-05-12 10:33 [PATCH bpf-next 0/4] bpftool: Fix skeletons compilation for older kernels Quentin Monnet
2023-05-12 10:33 ` [PATCH bpf-next 1/4] bpftool: use a local copy of perf_event to fix accessing ::bpf_cookie Quentin Monnet
@ 2023-05-12 10:33 ` Quentin Monnet
2023-05-12 14:47 ` Yonghong Song
2023-05-12 10:33 ` [PATCH bpf-next 3/4] bpftool: Use a local copy of BPF_LINK_TYPE_PERF_EVENT in pid_iter.bpf.c Quentin Monnet
2023-05-12 10:33 ` [PATCH bpf-next 4/4] bpftool: use a local bpf_perf_event_value to fix accessing its fields Quentin Monnet
3 siblings, 1 reply; 13+ messages in thread
From: Quentin Monnet @ 2023-05-12 10:33 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf,
Quentin Monnet, Alexander Lobakin, Michal Suchánek,
Alexander Lobakin
From: Alexander Lobakin <alobakin@pm.me>
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")
Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Alexander Lobakin <alobakin@pm.me>
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
---
tools/bpf/bpftool/skeleton/pid_iter.bpf.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
index e2af8e5fb29e..3a4c4f7d83d8 100644
--- a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
+++ b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
@@ -15,6 +15,11 @@ enum bpf_obj_type {
BPF_OBJ_BTF,
};
+struct bpf_perf_link___local {
+ struct bpf_link link;
+ struct file *perf_file;
+} __attribute__((preserve_access_index));
+
struct perf_event___local {
u64 bpf_cookie;
} __attribute__((preserve_access_index));
@@ -45,10 +50,10 @@ static __always_inline __u32 get_obj_id(void *ent, enum bpf_obj_type type)
/* could be used only with BPF_LINK_TYPE_PERF_EVENT links */
static __u64 get_bpf_cookie(struct bpf_link *link)
{
+ struct bpf_perf_link___local *perf_link;
struct perf_event___local *event;
- struct bpf_perf_link *perf_link;
- perf_link = container_of(link, struct bpf_perf_link, link);
+ perf_link = container_of(link, struct bpf_perf_link___local, link);
event = BPF_CORE_READ(perf_link, perf_file, private_data);
return BPF_CORE_READ(event, bpf_cookie);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH bpf-next 2/4] bpftool: define a local bpf_perf_link to fix accessing its fields
2023-05-12 10:33 ` [PATCH bpf-next 2/4] bpftool: define a local bpf_perf_link to fix accessing its fields Quentin Monnet
@ 2023-05-12 14:47 ` Yonghong Song
2023-05-15 16:43 ` Quentin Monnet
0 siblings, 1 reply; 13+ messages in thread
From: Yonghong Song @ 2023-05-12 14:47 UTC (permalink / raw)
To: Quentin Monnet, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko
Cc: Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf,
Alexander Lobakin, Michal Suchánek, Alexander Lobakin
On 5/12/23 3:33 AM, Quentin Monnet wrote:
> From: Alexander Lobakin <alobakin@pm.me>
>
> 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
'container_of() is not CO-REd' is incorrect.
#define container_of(ptr, type, member) \
({ \
void *__mptr = (void *)(ptr); \
((type *)(__mptr - offsetof(type, member))); \
})
offsetof() will do necessary CO-RE relocation if the field is specified
with preserve_access_index attribute. So container_of will actually
do CO-RE relocation as well.
> 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")
> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Alexander Lobakin <alobakin@pm.me>
> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> ---
> tools/bpf/bpftool/skeleton/pid_iter.bpf.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
> index e2af8e5fb29e..3a4c4f7d83d8 100644
> --- a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
> +++ b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
> @@ -15,6 +15,11 @@ enum bpf_obj_type {
> BPF_OBJ_BTF,
> };
>
> +struct bpf_perf_link___local {
> + struct bpf_link link;
> + struct file *perf_file;
> +} __attribute__((preserve_access_index));
> +
> struct perf_event___local {
> u64 bpf_cookie;
> } __attribute__((preserve_access_index));
> @@ -45,10 +50,10 @@ static __always_inline __u32 get_obj_id(void *ent, enum bpf_obj_type type)
> /* could be used only with BPF_LINK_TYPE_PERF_EVENT links */
> static __u64 get_bpf_cookie(struct bpf_link *link)
> {
> + struct bpf_perf_link___local *perf_link;
> struct perf_event___local *event;
> - struct bpf_perf_link *perf_link;
>
> - perf_link = container_of(link, struct bpf_perf_link, link);
> + perf_link = container_of(link, struct bpf_perf_link___local, link);
> event = BPF_CORE_READ(perf_link, perf_file, private_data);
> return BPF_CORE_READ(event, bpf_cookie);
> }
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH bpf-next 2/4] bpftool: define a local bpf_perf_link to fix accessing its fields
2023-05-12 14:47 ` Yonghong Song
@ 2023-05-15 16:43 ` Quentin Monnet
0 siblings, 0 replies; 13+ messages in thread
From: Quentin Monnet @ 2023-05-15 16:43 UTC (permalink / raw)
To: Yonghong Song, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko
Cc: Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf,
Alexander Lobakin, Michal Suchánek, Alexander Lobakin
2023-05-12 07:47 UTC-0700 ~ Yonghong Song <yhs@meta.com>
>
>
> On 5/12/23 3:33 AM, Quentin Monnet wrote:
>> From: Alexander Lobakin <alobakin@pm.me>
>>
>> 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
>
> 'container_of() is not CO-REd' is incorrect.
>
> #define container_of(ptr, type, member) \
> ({ \
> void *__mptr = (void *)(ptr); \
> ((type *)(__mptr - offsetof(type, member))); \
> })
>
>
> offsetof() will do necessary CO-RE relocation if the field is specified
> with preserve_access_index attribute. So container_of will actually
> do CO-RE relocation as well.
Thanks! I'll amend the description for the next iteration.
Quentin
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH bpf-next 3/4] bpftool: Use a local copy of BPF_LINK_TYPE_PERF_EVENT in pid_iter.bpf.c
2023-05-12 10:33 [PATCH bpf-next 0/4] bpftool: Fix skeletons compilation for older kernels Quentin Monnet
2023-05-12 10:33 ` [PATCH bpf-next 1/4] bpftool: use a local copy of perf_event to fix accessing ::bpf_cookie Quentin Monnet
2023-05-12 10:33 ` [PATCH bpf-next 2/4] bpftool: define a local bpf_perf_link to fix accessing its fields Quentin Monnet
@ 2023-05-12 10:33 ` Quentin Monnet
2023-05-12 10:33 ` [PATCH bpf-next 4/4] bpftool: use a local bpf_perf_event_value to fix accessing its fields Quentin Monnet
3 siblings, 0 replies; 13+ messages in thread
From: Quentin Monnet @ 2023-05-12 10:33 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf,
Quentin Monnet, Alexander Lobakin, Michal Suchánek
In order to allow the BPF program in bpftool's pid_iter.bpf.c to compile
correctly on hosts where vmlinux.h does not define
BPF_LINK_TYPE_PERF_EVENT (running kernel versions lower than 5.15, for
example), define and use a local copy of the enum value. This requires
LLVM 12 or newer to build the BPF program.
Fixes: cbdaf71f7e65 ("bpftool: Add bpf_cookie to link output")
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
---
tools/bpf/bpftool/skeleton/pid_iter.bpf.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
index 3a4c4f7d83d8..26004f0c5a6a 100644
--- a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
+++ b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
@@ -24,6 +24,10 @@ struct perf_event___local {
u64 bpf_cookie;
} __attribute__((preserve_access_index));
+enum bpf_link_type___local {
+ BPF_LINK_TYPE_PERF_EVENT___local = 7,
+};
+
extern const void bpf_link_fops __ksym;
extern const void bpf_map_fops __ksym;
extern const void bpf_prog_fops __ksym;
@@ -93,10 +97,13 @@ int iter(struct bpf_iter__task_file *ctx)
e.pid = task->tgid;
e.id = get_obj_id(file->private_data, obj_type);
- if (obj_type == BPF_OBJ_LINK) {
+ 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 (BPF_CORE_READ(link, type) == BPF_LINK_TYPE_PERF_EVENT) {
+ 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);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH bpf-next 4/4] bpftool: use a local bpf_perf_event_value to fix accessing its fields
2023-05-12 10:33 [PATCH bpf-next 0/4] bpftool: Fix skeletons compilation for older kernels Quentin Monnet
` (2 preceding siblings ...)
2023-05-12 10:33 ` [PATCH bpf-next 3/4] bpftool: Use a local copy of BPF_LINK_TYPE_PERF_EVENT in pid_iter.bpf.c Quentin Monnet
@ 2023-05-12 10:33 ` Quentin Monnet
2023-05-12 12:59 ` Jiri Olsa
2023-05-16 21:30 ` Andrii Nakryiko
3 siblings, 2 replies; 13+ messages in thread
From: Quentin Monnet @ 2023-05-12 10:33 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf,
Quentin Monnet, Alexander Lobakin, Michal Suchánek,
Alexander Lobakin
From: Alexander Lobakin <alobakin@pm.me>
Fix the following error when building bpftool:
CLANG profiler.bpf.o
CLANG pid_iter.bpf.o
skeleton/profiler.bpf.c:18:21: error: invalid application of 'sizeof' to an incomplete type 'struct bpf_perf_event_value'
__uint(value_size, sizeof(struct bpf_perf_event_value));
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helpers.h:13:39: note: expanded from macro '__uint'
tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helper_defs.h:7:8: note: forward declaration of 'struct bpf_perf_event_value'
struct bpf_perf_event_value;
^
struct bpf_perf_event_value is being used in the kernel only when
CONFIG_BPF_EVENTS is enabled, so it misses a BTF entry then.
Define struct bpf_perf_event_value___local with the
`preserve_access_index` attribute inside the pid_iter BPF prog to
allow compiling on any configs. It is a full mirror of a UAPI
structure, so is compatible both with and w/o CO-RE.
bpf_perf_event_read_value() requires a pointer of the original type,
so a cast is needed.
Fixes: 47c09d6a9f67 ("bpftool: Introduce "prog profile" command")
Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Alexander Lobakin <alobakin@pm.me>
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
---
tools/bpf/bpftool/skeleton/profiler.bpf.c | 27 ++++++++++++++---------
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/tools/bpf/bpftool/skeleton/profiler.bpf.c b/tools/bpf/bpftool/skeleton/profiler.bpf.c
index ce5b65e07ab1..2f80edc682f1 100644
--- a/tools/bpf/bpftool/skeleton/profiler.bpf.c
+++ b/tools/bpf/bpftool/skeleton/profiler.bpf.c
@@ -4,6 +4,12 @@
#include <bpf/bpf_helpers.h>
#include <bpf/bpf_tracing.h>
+struct bpf_perf_event_value___local {
+ __u64 counter;
+ __u64 enabled;
+ __u64 running;
+} __attribute__((preserve_access_index));
+
/* map of perf event fds, num_cpu * num_metric entries */
struct {
__uint(type, BPF_MAP_TYPE_PERF_EVENT_ARRAY);
@@ -15,14 +21,14 @@ struct {
struct {
__uint(type, BPF_MAP_TYPE_PERCPU_ARRAY);
__uint(key_size, sizeof(u32));
- __uint(value_size, sizeof(struct bpf_perf_event_value));
+ __uint(value_size, sizeof(struct bpf_perf_event_value___local));
} fentry_readings SEC(".maps");
/* accumulated readings */
struct {
__uint(type, BPF_MAP_TYPE_PERCPU_ARRAY);
__uint(key_size, sizeof(u32));
- __uint(value_size, sizeof(struct bpf_perf_event_value));
+ __uint(value_size, sizeof(struct bpf_perf_event_value___local));
} accum_readings SEC(".maps");
/* sample counts, one per cpu */
@@ -39,7 +45,7 @@ const volatile __u32 num_metric = 1;
SEC("fentry/XXX")
int BPF_PROG(fentry_XXX)
{
- struct bpf_perf_event_value *ptrs[MAX_NUM_MATRICS];
+ struct bpf_perf_event_value___local *ptrs[MAX_NUM_MATRICS];
u32 key = bpf_get_smp_processor_id();
u32 i;
@@ -53,10 +59,10 @@ int BPF_PROG(fentry_XXX)
}
for (i = 0; i < num_metric && i < MAX_NUM_MATRICS; i++) {
- struct bpf_perf_event_value reading;
+ struct bpf_perf_event_value___local reading;
int err;
- err = bpf_perf_event_read_value(&events, key, &reading,
+ err = bpf_perf_event_read_value(&events, key, (void *)&reading,
sizeof(reading));
if (err)
return 0;
@@ -68,14 +74,14 @@ int BPF_PROG(fentry_XXX)
}
static inline void
-fexit_update_maps(u32 id, struct bpf_perf_event_value *after)
+fexit_update_maps(u32 id, struct bpf_perf_event_value___local *after)
{
- struct bpf_perf_event_value *before, diff;
+ struct bpf_perf_event_value___local *before, diff;
before = bpf_map_lookup_elem(&fentry_readings, &id);
/* only account samples with a valid fentry_reading */
if (before && before->counter) {
- struct bpf_perf_event_value *accum;
+ struct bpf_perf_event_value___local *accum;
diff.counter = after->counter - before->counter;
diff.enabled = after->enabled - before->enabled;
@@ -93,7 +99,7 @@ fexit_update_maps(u32 id, struct bpf_perf_event_value *after)
SEC("fexit/XXX")
int BPF_PROG(fexit_XXX)
{
- struct bpf_perf_event_value readings[MAX_NUM_MATRICS];
+ struct bpf_perf_event_value___local readings[MAX_NUM_MATRICS];
u32 cpu = bpf_get_smp_processor_id();
u32 i, zero = 0;
int err;
@@ -102,7 +108,8 @@ int BPF_PROG(fexit_XXX)
/* read all events before updating the maps, to reduce error */
for (i = 0; i < num_metric && i < MAX_NUM_MATRICS; i++) {
err = bpf_perf_event_read_value(&events, cpu + i * num_cpu,
- readings + i, sizeof(*readings));
+ (void *)(readings + i),
+ sizeof(*readings));
if (err)
return 0;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH bpf-next 4/4] bpftool: use a local bpf_perf_event_value to fix accessing its fields
2023-05-12 10:33 ` [PATCH bpf-next 4/4] bpftool: use a local bpf_perf_event_value to fix accessing its fields Quentin Monnet
@ 2023-05-12 12:59 ` Jiri Olsa
2023-05-15 16:53 ` Quentin Monnet
2023-07-07 9:53 ` Quentin Monnet
2023-05-16 21:30 ` Andrii Nakryiko
1 sibling, 2 replies; 13+ messages in thread
From: Jiri Olsa @ 2023-05-12 12:59 UTC (permalink / raw)
To: Quentin Monnet
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, bpf, Alexander Lobakin,
Michal Suchánek, Alexander Lobakin
On Fri, May 12, 2023 at 11:33:54AM +0100, Quentin Monnet wrote:
> From: Alexander Lobakin <alobakin@pm.me>
>
> Fix the following error when building bpftool:
>
> CLANG profiler.bpf.o
> CLANG pid_iter.bpf.o
> skeleton/profiler.bpf.c:18:21: error: invalid application of 'sizeof' to an incomplete type 'struct bpf_perf_event_value'
> __uint(value_size, sizeof(struct bpf_perf_event_value));
> ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helpers.h:13:39: note: expanded from macro '__uint'
> tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helper_defs.h:7:8: note: forward declaration of 'struct bpf_perf_event_value'
> struct bpf_perf_event_value;
> ^
>
> struct bpf_perf_event_value is being used in the kernel only when
> CONFIG_BPF_EVENTS is enabled, so it misses a BTF entry then.
hi,
when I switch off CONFIG_BPF_EVENTS the bpftool build fails for me
with missing BTF error:
GEN vmlinux.h
libbpf: failed to find '.BTF' ELF section in /home/jolsa/kernel/linux-qemu/vmlinux
Error: failed to load BTF from /home/jolsa/kernel/linux-qemu/vmlinux: No data available
make: *** [Makefile:208: vmlinux.h] Error 195
make: *** Deleting file 'vmlinux.h'
so I wonder you need to care about bpf_perf_event_value
in that case
jirka
> Define struct bpf_perf_event_value___local with the
> `preserve_access_index` attribute inside the pid_iter BPF prog to
> allow compiling on any configs. It is a full mirror of a UAPI
> structure, so is compatible both with and w/o CO-RE.
> bpf_perf_event_read_value() requires a pointer of the original type,
> so a cast is needed.
>
> Fixes: 47c09d6a9f67 ("bpftool: Introduce "prog profile" command")
> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Alexander Lobakin <alobakin@pm.me>
> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> ---
> tools/bpf/bpftool/skeleton/profiler.bpf.c | 27 ++++++++++++++---------
> 1 file changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/tools/bpf/bpftool/skeleton/profiler.bpf.c b/tools/bpf/bpftool/skeleton/profiler.bpf.c
> index ce5b65e07ab1..2f80edc682f1 100644
> --- a/tools/bpf/bpftool/skeleton/profiler.bpf.c
> +++ b/tools/bpf/bpftool/skeleton/profiler.bpf.c
> @@ -4,6 +4,12 @@
> #include <bpf/bpf_helpers.h>
> #include <bpf/bpf_tracing.h>
>
> +struct bpf_perf_event_value___local {
> + __u64 counter;
> + __u64 enabled;
> + __u64 running;
> +} __attribute__((preserve_access_index));
> +
> /* map of perf event fds, num_cpu * num_metric entries */
> struct {
> __uint(type, BPF_MAP_TYPE_PERF_EVENT_ARRAY);
> @@ -15,14 +21,14 @@ struct {
> struct {
> __uint(type, BPF_MAP_TYPE_PERCPU_ARRAY);
> __uint(key_size, sizeof(u32));
> - __uint(value_size, sizeof(struct bpf_perf_event_value));
> + __uint(value_size, sizeof(struct bpf_perf_event_value___local));
> } fentry_readings SEC(".maps");
>
> /* accumulated readings */
> struct {
> __uint(type, BPF_MAP_TYPE_PERCPU_ARRAY);
> __uint(key_size, sizeof(u32));
> - __uint(value_size, sizeof(struct bpf_perf_event_value));
> + __uint(value_size, sizeof(struct bpf_perf_event_value___local));
> } accum_readings SEC(".maps");
>
> /* sample counts, one per cpu */
> @@ -39,7 +45,7 @@ const volatile __u32 num_metric = 1;
> SEC("fentry/XXX")
> int BPF_PROG(fentry_XXX)
> {
> - struct bpf_perf_event_value *ptrs[MAX_NUM_MATRICS];
> + struct bpf_perf_event_value___local *ptrs[MAX_NUM_MATRICS];
> u32 key = bpf_get_smp_processor_id();
> u32 i;
>
> @@ -53,10 +59,10 @@ int BPF_PROG(fentry_XXX)
> }
>
> for (i = 0; i < num_metric && i < MAX_NUM_MATRICS; i++) {
> - struct bpf_perf_event_value reading;
> + struct bpf_perf_event_value___local reading;
> int err;
>
> - err = bpf_perf_event_read_value(&events, key, &reading,
> + err = bpf_perf_event_read_value(&events, key, (void *)&reading,
> sizeof(reading));
> if (err)
> return 0;
> @@ -68,14 +74,14 @@ int BPF_PROG(fentry_XXX)
> }
>
> static inline void
> -fexit_update_maps(u32 id, struct bpf_perf_event_value *after)
> +fexit_update_maps(u32 id, struct bpf_perf_event_value___local *after)
> {
> - struct bpf_perf_event_value *before, diff;
> + struct bpf_perf_event_value___local *before, diff;
>
> before = bpf_map_lookup_elem(&fentry_readings, &id);
> /* only account samples with a valid fentry_reading */
> if (before && before->counter) {
> - struct bpf_perf_event_value *accum;
> + struct bpf_perf_event_value___local *accum;
>
> diff.counter = after->counter - before->counter;
> diff.enabled = after->enabled - before->enabled;
> @@ -93,7 +99,7 @@ fexit_update_maps(u32 id, struct bpf_perf_event_value *after)
> SEC("fexit/XXX")
> int BPF_PROG(fexit_XXX)
> {
> - struct bpf_perf_event_value readings[MAX_NUM_MATRICS];
> + struct bpf_perf_event_value___local readings[MAX_NUM_MATRICS];
> u32 cpu = bpf_get_smp_processor_id();
> u32 i, zero = 0;
> int err;
> @@ -102,7 +108,8 @@ int BPF_PROG(fexit_XXX)
> /* read all events before updating the maps, to reduce error */
> for (i = 0; i < num_metric && i < MAX_NUM_MATRICS; i++) {
> err = bpf_perf_event_read_value(&events, cpu + i * num_cpu,
> - readings + i, sizeof(*readings));
> + (void *)(readings + i),
> + sizeof(*readings));
> if (err)
> return 0;
> }
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH bpf-next 4/4] bpftool: use a local bpf_perf_event_value to fix accessing its fields
2023-05-12 12:59 ` Jiri Olsa
@ 2023-05-15 16:53 ` Quentin Monnet
2023-07-07 9:53 ` Quentin Monnet
1 sibling, 0 replies; 13+ messages in thread
From: Quentin Monnet @ 2023-05-15 16:53 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, bpf, Alexander Lobakin,
Michal Suchánek, Alexander Lobakin
2023-05-12 14:59 UTC+0200 ~ Jiri Olsa <olsajiri@gmail.com>
> On Fri, May 12, 2023 at 11:33:54AM +0100, Quentin Monnet wrote:
>> From: Alexander Lobakin <alobakin@pm.me>
>>
>> Fix the following error when building bpftool:
>>
>> CLANG profiler.bpf.o
>> CLANG pid_iter.bpf.o
>> skeleton/profiler.bpf.c:18:21: error: invalid application of 'sizeof' to an incomplete type 'struct bpf_perf_event_value'
>> __uint(value_size, sizeof(struct bpf_perf_event_value));
>> ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helpers.h:13:39: note: expanded from macro '__uint'
>> tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helper_defs.h:7:8: note: forward declaration of 'struct bpf_perf_event_value'
>> struct bpf_perf_event_value;
>> ^
>>
>> struct bpf_perf_event_value is being used in the kernel only when
>> CONFIG_BPF_EVENTS is enabled, so it misses a BTF entry then.
>
> hi,
> when I switch off CONFIG_BPF_EVENTS the bpftool build fails for me
> with missing BTF error:
>
> GEN vmlinux.h
> libbpf: failed to find '.BTF' ELF section in /home/jolsa/kernel/linux-qemu/vmlinux
> Error: failed to load BTF from /home/jolsa/kernel/linux-qemu/vmlinux: No data available
> make: *** [Makefile:208: vmlinux.h] Error 195
> make: *** Deleting file 'vmlinux.h'
>
> so I wonder you need to care about bpf_perf_event_value
> in that case
>
> jirka
Thanks for testing, Jiri! As I understand, this is the vmlinux.h
generation failing, not the compilation from the updated skeletons, so
I'm not sure this is related to this patch, could be an existing bug.
Have you tried the same build without the patches, by any chance?
I'll try to reproduce on my side to try and figure out why libbpf's
bpf_parse_elf() fails to find the .BTF section.
Quentin
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 4/4] bpftool: use a local bpf_perf_event_value to fix accessing its fields
2023-05-12 12:59 ` Jiri Olsa
2023-05-15 16:53 ` Quentin Monnet
@ 2023-07-07 9:53 ` Quentin Monnet
1 sibling, 0 replies; 13+ messages in thread
From: Quentin Monnet @ 2023-07-07 9:53 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, bpf, Alexander Lobakin,
Michal Suchánek, Alexander Lobakin
2023-05-12 14:59 UTC+0200 ~ Jiri Olsa <olsajiri@gmail.com>
> On Fri, May 12, 2023 at 11:33:54AM +0100, Quentin Monnet wrote:
>> From: Alexander Lobakin <alobakin@pm.me>
>>
>> Fix the following error when building bpftool:
>>
>> CLANG profiler.bpf.o
>> CLANG pid_iter.bpf.o
>> skeleton/profiler.bpf.c:18:21: error: invalid application of 'sizeof' to an incomplete type 'struct bpf_perf_event_value'
>> __uint(value_size, sizeof(struct bpf_perf_event_value));
>> ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helpers.h:13:39: note: expanded from macro '__uint'
>> tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helper_defs.h:7:8: note: forward declaration of 'struct bpf_perf_event_value'
>> struct bpf_perf_event_value;
>> ^
>>
>> struct bpf_perf_event_value is being used in the kernel only when
>> CONFIG_BPF_EVENTS is enabled, so it misses a BTF entry then.
>
> hi,
> when I switch off CONFIG_BPF_EVENTS the bpftool build fails for me
> with missing BTF error:
>
> GEN vmlinux.h
> libbpf: failed to find '.BTF' ELF section in /home/jolsa/kernel/linux-qemu/vmlinux
> Error: failed to load BTF from /home/jolsa/kernel/linux-qemu/vmlinux: No data available
> make: *** [Makefile:208: vmlinux.h] Error 195
> make: *** Deleting file 'vmlinux.h'
>
> so I wonder you need to care about bpf_perf_event_value
> in that case
>
> jirka
Coming back to this - I haven't been able to reproduce. I turned
CONFIG_BPF_EVENTS off but had the vmlinux.h building fine in my case,
and applying the patchset fixes the build for bpftool. Is there any
chance your .../vmlinux was not generated correctly?
I'll re-submit the series after addressing Yonghong's feedback on the
commit log.
Thanks,
Quentin
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 4/4] bpftool: use a local bpf_perf_event_value to fix accessing its fields
2023-05-12 10:33 ` [PATCH bpf-next 4/4] bpftool: use a local bpf_perf_event_value to fix accessing its fields Quentin Monnet
2023-05-12 12:59 ` Jiri Olsa
@ 2023-05-16 21:30 ` Andrii Nakryiko
2023-05-17 15:02 ` Quentin Monnet
1 sibling, 1 reply; 13+ messages in thread
From: Andrii Nakryiko @ 2023-05-16 21:30 UTC (permalink / raw)
To: Quentin Monnet
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf,
Alexander Lobakin, Michal Suchánek, Alexander Lobakin
On Fri, May 12, 2023 at 3:34 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> From: Alexander Lobakin <alobakin@pm.me>
>
> Fix the following error when building bpftool:
>
> CLANG profiler.bpf.o
> CLANG pid_iter.bpf.o
> skeleton/profiler.bpf.c:18:21: error: invalid application of 'sizeof' to an incomplete type 'struct bpf_perf_event_value'
> __uint(value_size, sizeof(struct bpf_perf_event_value));
> ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helpers.h:13:39: note: expanded from macro '__uint'
> tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helper_defs.h:7:8: note: forward declaration of 'struct bpf_perf_event_value'
> struct bpf_perf_event_value;
> ^
>
> struct bpf_perf_event_value is being used in the kernel only when
> CONFIG_BPF_EVENTS is enabled, so it misses a BTF entry then.
> Define struct bpf_perf_event_value___local with the
> `preserve_access_index` attribute inside the pid_iter BPF prog to
> allow compiling on any configs. It is a full mirror of a UAPI
> structure, so is compatible both with and w/o CO-RE.
> bpf_perf_event_read_value() requires a pointer of the original type,
> so a cast is needed.
>
> Fixes: 47c09d6a9f67 ("bpftool: Introduce "prog profile" command")
> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Alexander Lobakin <alobakin@pm.me>
> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> ---
What's the point of using vmlinux.h at all if we redefine every single
type? bpf_perf_event_value is part of BPF UAPI, so if we included
linux/bpf.h header we'd get it.
This feels a bit split-brained. We either drop vmlinux.h completely
and use UAPI headers + CO-RE-relocatable definitions of internal
types, or we make sure that vmlinux.h does work (e.g., by pre-checking
in a very small version of it). Both using vmlinux.h and not relying
on it having necessary types seems like the worst of both worlds?...
Quentin, can you see if you can come up with some simple way to use
vmlinux.h if building from inside kernel repo, but using a minimized
vmlinux.h generated using `bpftool gen min_core_btf` when building
from Github mirror? Sync script could also generate this minimal
vmlinux.h automatically, presumably?
> tools/bpf/bpftool/skeleton/profiler.bpf.c | 27 ++++++++++++++---------
> 1 file changed, 17 insertions(+), 10 deletions(-)
>
[...]
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH bpf-next 4/4] bpftool: use a local bpf_perf_event_value to fix accessing its fields
2023-05-16 21:30 ` Andrii Nakryiko
@ 2023-05-17 15:02 ` Quentin Monnet
2023-05-17 16:58 ` Andrii Nakryiko
0 siblings, 1 reply; 13+ messages in thread
From: Quentin Monnet @ 2023-05-17 15:02 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf,
Alexander Lobakin, Michal Suchánek, Alexander Lobakin
2023-05-16 14:30 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
> On Fri, May 12, 2023 at 3:34 AM Quentin Monnet <quentin@isovalent.com> wrote:
>>
>> From: Alexander Lobakin <alobakin@pm.me>
>>
>> Fix the following error when building bpftool:
>>
>> CLANG profiler.bpf.o
>> CLANG pid_iter.bpf.o
>> skeleton/profiler.bpf.c:18:21: error: invalid application of 'sizeof' to an incomplete type 'struct bpf_perf_event_value'
>> __uint(value_size, sizeof(struct bpf_perf_event_value));
>> ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helpers.h:13:39: note: expanded from macro '__uint'
>> tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helper_defs.h:7:8: note: forward declaration of 'struct bpf_perf_event_value'
>> struct bpf_perf_event_value;
>> ^
>>
>> struct bpf_perf_event_value is being used in the kernel only when
>> CONFIG_BPF_EVENTS is enabled, so it misses a BTF entry then.
>> Define struct bpf_perf_event_value___local with the
>> `preserve_access_index` attribute inside the pid_iter BPF prog to
>> allow compiling on any configs. It is a full mirror of a UAPI
>> structure, so is compatible both with and w/o CO-RE.
>> bpf_perf_event_read_value() requires a pointer of the original type,
>> so a cast is needed.
>>
>> Fixes: 47c09d6a9f67 ("bpftool: Introduce "prog profile" command")
>> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
>> Signed-off-by: Alexander Lobakin <alobakin@pm.me>
>> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
>> ---
>
> What's the point of using vmlinux.h at all if we redefine every single
> type? bpf_perf_event_value is part of BPF UAPI, so if we included
> linux/bpf.h header we'd get it.
I gave a quick try at the UAPI header before posting this patch, but it
was an Ubuntu box and I got the "asm/types.h not found" error. If I
remember correctly, one way to fix this is to have the gcc-multilib,
which I'd rather avoid to add as a dependency; or adding the correct
include path for x86_64 at least, which I haven't tried for bpftool yet.
>
> This feels a bit split-brained. We either drop vmlinux.h completely
> and use UAPI headers + CO-RE-relocatable definitions of internal
> types, or we make sure that vmlinux.h does work (e.g., by pre-checking
> in a very small version of it). Both using vmlinux.h and not relying
> on it having necessary types seems like the worst of both worlds?...
Yeah I do feel like I'm missing something in this set and the approach
is not optimal. What do you mean exactly by "pre-checking in a very
small version of it"? Checking for the availability of a few types and
exit early from the functions if they're missing, because we're assuming
we won't have support for the feature?
> Quentin, can you see if you can come up with some simple way to use
> vmlinux.h if building from inside kernel repo, but using a minimized
> vmlinux.h generated using `bpftool gen min_core_btf` when building
> from Github mirror? Sync script could also generate this minimal
> vmlinux.h automatically, presumably?
Sure, I can look into it. But not right now - I'd like to get the
current issue, and the (unrelated) LLVM feature detection, sorted before
starting on this.
Thanks for your feedback!
Quentin
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH bpf-next 4/4] bpftool: use a local bpf_perf_event_value to fix accessing its fields
2023-05-17 15:02 ` Quentin Monnet
@ 2023-05-17 16:58 ` Andrii Nakryiko
0 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2023-05-17 16:58 UTC (permalink / raw)
To: Quentin Monnet
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf,
Alexander Lobakin, Michal Suchánek, Alexander Lobakin
On Wed, May 17, 2023 at 8:02 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> 2023-05-16 14:30 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
> > On Fri, May 12, 2023 at 3:34 AM Quentin Monnet <quentin@isovalent.com> wrote:
> >>
> >> From: Alexander Lobakin <alobakin@pm.me>
> >>
> >> Fix the following error when building bpftool:
> >>
> >> CLANG profiler.bpf.o
> >> CLANG pid_iter.bpf.o
> >> skeleton/profiler.bpf.c:18:21: error: invalid application of 'sizeof' to an incomplete type 'struct bpf_perf_event_value'
> >> __uint(value_size, sizeof(struct bpf_perf_event_value));
> >> ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helpers.h:13:39: note: expanded from macro '__uint'
> >> tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helper_defs.h:7:8: note: forward declaration of 'struct bpf_perf_event_value'
> >> struct bpf_perf_event_value;
> >> ^
> >>
> >> struct bpf_perf_event_value is being used in the kernel only when
> >> CONFIG_BPF_EVENTS is enabled, so it misses a BTF entry then.
> >> Define struct bpf_perf_event_value___local with the
> >> `preserve_access_index` attribute inside the pid_iter BPF prog to
> >> allow compiling on any configs. It is a full mirror of a UAPI
> >> structure, so is compatible both with and w/o CO-RE.
> >> bpf_perf_event_read_value() requires a pointer of the original type,
> >> so a cast is needed.
> >>
> >> Fixes: 47c09d6a9f67 ("bpftool: Introduce "prog profile" command")
> >> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> >> Signed-off-by: Alexander Lobakin <alobakin@pm.me>
> >> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> >> ---
> >
> > What's the point of using vmlinux.h at all if we redefine every single
> > type? bpf_perf_event_value is part of BPF UAPI, so if we included
> > linux/bpf.h header we'd get it.
>
> I gave a quick try at the UAPI header before posting this patch, but it
> was an Ubuntu box and I got the "asm/types.h not found" error. If I
> remember correctly, one way to fix this is to have the gcc-multilib,
> which I'd rather avoid to add as a dependency; or adding the correct
> include path for x86_64 at least, which I haven't tried for bpftool yet.
>
> >
> > This feels a bit split-brained. We either drop vmlinux.h completely
> > and use UAPI headers + CO-RE-relocatable definitions of internal
> > types, or we make sure that vmlinux.h does work (e.g., by pre-checking
> > in a very small version of it). Both using vmlinux.h and not relying
> > on it having necessary types seems like the worst of both worlds?...
>
> Yeah I do feel like I'm missing something in this set and the approach
> is not optimal. What do you mean exactly by "pre-checking in a very
> small version of it"? Checking for the availability of a few types and
> exit early from the functions if they're missing, because we're assuming
> we won't have support for the feature?
So I was thinking that we'll keep relying on vmlinux BTF and proper
Kconfig for bpftool build inside kernel repo, no changes there. But
then we can use `bpftool gen min_core_btf` on resulting .bpf.o files
to dump only types that are actually used/referenced by bpftool's BPF
object files, and then we can generate vmlinux.h from that minimized
BTF.
I haven't tried it, and I'm sure there will be some hiccups along the
way, but that was the idea.
>
> > Quentin, can you see if you can come up with some simple way to use
> > vmlinux.h if building from inside kernel repo, but using a minimized
> > vmlinux.h generated using `bpftool gen min_core_btf` when building
> > from Github mirror? Sync script could also generate this minimal
> > vmlinux.h automatically, presumably?
>
> Sure, I can look into it. But not right now - I'd like to get the
> current issue, and the (unrelated) LLVM feature detection, sorted before
> starting on this.
Sounds good. In general, your patches look good to me, I was hoping we
can avoid unnecessary definitions of UAPI types, but if that doesn't
work out of the box, then I'm fine with it.
>
> Thanks for your feedback!
> Quentin
^ permalink raw reply [flat|nested] 13+ messages in thread