[PATCH bpf-next 0/5] Libbpf API and memfd_create() fixes

[PATCH bpf-next 0/5] Libbpf API and memfd_create() fixes

[Andrii Nakryiko]

Few small fixes identified over last few days. Main fix is for memfd_create()
which causes problems on Android platforms. See individual patches for
details.

Andrii Nakryiko (5):
  libbpf: call memfd_create() syscall directly
  libbpf: add missing LIBBPF_API annotation to libbpf_set_memlock_rlim
    API
  libbpf: add btf__new_split() API that was declared but not implemented
  libbpf: add missed btf_ext__raw_data() API
  selftests/bpf: fix bench runner SIGSEGV

 tools/lib/bpf/bpf.h                 |  2 +-
 tools/lib/bpf/btf.c                 | 11 ++++++++++-
 tools/lib/bpf/libbpf.c              | 11 ++++++++++-
 tools/lib/bpf/libbpf.map            |  5 +++--
 tools/lib/bpf/linker.c              |  2 +-
 tools/testing/selftests/bpf/bench.c | 10 +++++++++-
 6 files changed, 34 insertions(+), 7 deletions(-)

-- 
2.34.1

[PATCH bpf-next 1/5] libbpf: call memfd_create() syscall directly

[Andrii Nakryiko]

Some versions of Android do not implement memfd_create() wrapper in
their libc implementation, leading to build failures ([0]). On the other
hand, memfd_create() is available as a syscall on quite old kernels
(3.17+, while bpf() syscall itself is available since 3.18+), so it is
ok to assume that syscall availability and call into it with syscall()
helper to avoid Android-specific workarounds.

Validated in libbpf-bootstrap's CI ([1]).

  [0] https://github.com/libbpf/libbpf-bootstrap/actions/runs/7701003207/job/20986080319#step:5:83
  [1] https://github.com/libbpf/libbpf-bootstrap/actions/runs/7715988887/job/21031767212?pr=253

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/libbpf.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index db65ea59a05a..b21fda6b9b85 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -1525,11 +1525,20 @@ static Elf64_Sym *find_elf_var_sym(const struct bpf_object *obj, const char *nam
 	return ERR_PTR(-ENOENT);
 }
 
+/* Some versions of Android don't provide memfd_create() in their libc
+ * implementation, so avoid complications and just go straight to Linux
+ * syscall.
+ */
+static int sys_memfd_create(const char *name, unsigned flags)
+{
+	return syscall(__NR_memfd_create, name, flags);
+}
+
 static int create_placeholder_fd(void)
 {
 	int fd;
 
-	fd = ensure_good_fd(memfd_create("libbpf-placeholder-fd", MFD_CLOEXEC));
+	fd = ensure_good_fd(sys_memfd_create("libbpf-placeholder-fd", MFD_CLOEXEC));
 	if (fd < 0)
 		return -errno;
 	return fd;
-- 
2.34.1

[PATCH bpf-next 2/5] libbpf: add missing LIBBPF_API annotation to libbpf_set_memlock_rlim API

[Andrii Nakryiko]

LIBBPF_API annotation seems missing on libbpf_set_memlock_rlim API, so
add it to make this API callable from libbpf's shared library version.

Fixes: e542f2c4cd16 ("libbpf: Auto-bump RLIMIT_MEMLOCK if kernel needs it for BPF")
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/bpf.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
index 1441f642c563..f866e98b2436 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -35,7 +35,7 @@
 extern "C" {
 #endif
 
-int libbpf_set_memlock_rlim(size_t memlock_bytes);
+LIBBPF_API int libbpf_set_memlock_rlim(size_t memlock_bytes);
 
 struct bpf_map_create_opts {
 	size_t sz; /* size of this struct for forward/backward compatibility */
-- 
2.34.1

[PATCH bpf-next 3/5] libbpf: add btf__new_split() API that was declared but not implemented

[Andrii Nakryiko]

Seems like original commit adding split BTF support intended to add
btf__new_split() API, and even declared it in libbpf.map, but never
added (trivial) implementation. Fix this.

Fixes: ba451366bf44 ("libbpf: Implement basic split BTF support")
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/btf.c      | 5 +++++
 tools/lib/bpf/libbpf.map | 3 ++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 95db88b36cf3..845034d15420 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -1079,6 +1079,11 @@ struct btf *btf__new(const void *data, __u32 size)
 	return libbpf_ptr(btf_new(data, size, NULL));
 }
 
+struct btf *btf__new_split(const void *data, __u32 size, struct btf *base_btf)
+{
+	return libbpf_ptr(btf_new(data, size, base_btf));
+}
+
 static struct btf *btf_parse_elf(const char *path, struct btf *base_btf,
 				 struct btf_ext **btf_ext)
 {
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index d9e1f57534fa..386964f572a8 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -245,7 +245,6 @@ LIBBPF_0.3.0 {
 		btf__parse_raw_split;
 		btf__parse_split;
 		btf__new_empty_split;
-		btf__new_split;
 		ring_buffer__epoll_fd;
 } LIBBPF_0.2.0;
 
@@ -411,5 +410,7 @@ LIBBPF_1.3.0 {
 } LIBBPF_1.2.0;
 
 LIBBPF_1.4.0 {
+	global:
 		bpf_token_create;
+		btf__new_split;
 } LIBBPF_1.3.0;
-- 
2.34.1

[PATCH bpf-next 4/5] libbpf: add missed btf_ext__raw_data() API

[Andrii Nakryiko]

Another API that was declared in libbpf.map but actual implementation
was missing. btf_ext__get_raw_data() was intended as a discouraged alias
to consistently-named btf_ext__raw_data(), so make this an actuality.

Fixes: 20eccf29e297 ("libbpf: hide and discourage inconsistently named getters")
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/btf.c      | 6 +++++-
 tools/lib/bpf/libbpf.map | 2 +-
 tools/lib/bpf/linker.c   | 2 +-
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 845034d15420..a17b4c9c4213 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -3050,12 +3050,16 @@ struct btf_ext *btf_ext__new(const __u8 *data, __u32 size)
 	return btf_ext;
 }
 
-const void *btf_ext__get_raw_data(const struct btf_ext *btf_ext, __u32 *size)
+const void *btf_ext__raw_data(const struct btf_ext *btf_ext, __u32 *size)
 {
 	*size = btf_ext->data_size;
 	return btf_ext->data;
 }
 
+__attribute__((alias("btf_ext__raw_data")))
+const void *btf_ext__get_raw_data(const struct btf_ext *btf_ext, __u32 *size);
+
+
 struct btf_dedup;
 
 static struct btf_dedup *btf_dedup_new(struct btf *btf, const struct btf_dedup_opts *opts);
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 386964f572a8..86804fd90dd1 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -325,7 +325,6 @@ LIBBPF_0.7.0 {
 		bpf_xdp_detach;
 		bpf_xdp_query;
 		bpf_xdp_query_id;
-		btf_ext__raw_data;
 		libbpf_probe_bpf_helper;
 		libbpf_probe_bpf_map_type;
 		libbpf_probe_bpf_prog_type;
@@ -413,4 +412,5 @@ LIBBPF_1.4.0 {
 	global:
 		bpf_token_create;
 		btf__new_split;
+		btf_ext__raw_data;
 } LIBBPF_1.3.0;
diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
index 16bca56002ab..0d4be829551b 100644
--- a/tools/lib/bpf/linker.c
+++ b/tools/lib/bpf/linker.c
@@ -2732,7 +2732,7 @@ static int finalize_btf(struct bpf_linker *linker)
 
 	/* Emit .BTF.ext section */
 	if (linker->btf_ext) {
-		raw_data = btf_ext__get_raw_data(linker->btf_ext, &raw_sz);
+		raw_data = btf_ext__raw_data(linker->btf_ext, &raw_sz);
 		if (!raw_data)
 			return -ENOMEM;
 
-- 
2.34.1

[PATCH bpf-next 5/5] selftests/bpf: fix bench runner SIGSEGV

[Andrii Nakryiko]

Some benchmarks don't anticipate "consumer" and/or "producer" sides. Add
NULL checks in corresponding places and warn about inappropriate
consumer/producer count argument values.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/testing/selftests/bpf/bench.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/bench.c b/tools/testing/selftests/bpf/bench.c
index 73ce11b0547d..36962fc305eb 100644
--- a/tools/testing/selftests/bpf/bench.c
+++ b/tools/testing/selftests/bpf/bench.c
@@ -330,7 +330,7 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
 		break;
 	case 'c':
 		env.consumer_cnt = strtol(arg, NULL, 10);
-		if (env.consumer_cnt <= 0) {
+		if (env.consumer_cnt < 0) {
 			fprintf(stderr, "Invalid consumer count: %s\n", arg);
 			argp_usage(state);
 		}
@@ -607,6 +607,10 @@ static void setup_benchmark(void)
 		bench->setup();
 
 	for (i = 0; i < env.consumer_cnt; i++) {
+		if (!bench->consumer_thread) {
+			fprintf(stderr, "benchmark doesn't have consumers!\n");
+			exit(1);
+		}
 		err = pthread_create(&state.consumers[i], NULL,
 				     bench->consumer_thread, (void *)(long)i);
 		if (err) {
@@ -626,6 +630,10 @@ static void setup_benchmark(void)
 		env.prod_cpus.next_cpu = env.cons_cpus.next_cpu;
 
 	for (i = 0; i < env.producer_cnt; i++) {
+		if (!bench->producer_thread) {
+			fprintf(stderr, "benchmark doesn't have producers!\n");
+			exit(1);
+		}
 		err = pthread_create(&state.producers[i], NULL,
 				     bench->producer_thread, (void *)(long)i);
 		if (err) {
-- 
2.34.1

Re: [PATCH bpf-next 1/5] libbpf: call memfd_create() syscall directly

[Yonghong Song]

On 1/30/24 11:36 AM, Andrii Nakryiko wrote:
> Some versions of Android do not implement memfd_create() wrapper in
> their libc implementation, leading to build failures ([0]). On the other
> hand, memfd_create() is available as a syscall on quite old kernels
> (3.17+, while bpf() syscall itself is available since 3.18+), so it is
> ok to assume that syscall availability and call into it with syscall()
> helper to avoid Android-specific workarounds.
>
> Validated in libbpf-bootstrap's CI ([1]).
>
>    [0] https://github.com/libbpf/libbpf-bootstrap/actions/runs/7701003207/job/20986080319#step:5:83
>    [1] https://github.com/libbpf/libbpf-bootstrap/actions/runs/7715988887/job/21031767212?pr=253
>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>

Acked-by: Yonghong Song <yonghong.song@linux.dev>

Re: [PATCH bpf-next 2/5] libbpf: add missing LIBBPF_API annotation to libbpf_set_memlock_rlim API

[Yonghong Song]

On 1/30/24 11:36 AM, Andrii Nakryiko wrote:
> LIBBPF_API annotation seems missing on libbpf_set_memlock_rlim API, so
> add it to make this API callable from libbpf's shared library version.
>
> Fixes: e542f2c4cd16 ("libbpf: Auto-bump RLIMIT_MEMLOCK if kernel needs it for BPF")

Maybe we should the following commit as Fixes?

   ab9a5a05dc48 libbpf: fix up few libbpf.map problems

Other than the above, LGTM.

Acked-by: Yonghong Song <yonghong.song@linux.dev>

> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>   tools/lib/bpf/bpf.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> index 1441f642c563..f866e98b2436 100644
> --- a/tools/lib/bpf/bpf.h
> +++ b/tools/lib/bpf/bpf.h
> @@ -35,7 +35,7 @@
>   extern "C" {
>   #endif
>   
> -int libbpf_set_memlock_rlim(size_t memlock_bytes);
> +LIBBPF_API int libbpf_set_memlock_rlim(size_t memlock_bytes);
>   
>   struct bpf_map_create_opts {
>   	size_t sz; /* size of this struct for forward/backward compatibility */

Re: [PATCH bpf-next 3/5] libbpf: add btf__new_split() API that was declared but not implemented

[Yonghong Song]

On 1/30/24 11:36 AM, Andrii Nakryiko wrote:
> Seems like original commit adding split BTF support intended to add
> btf__new_split() API, and even declared it in libbpf.map, but never
> added (trivial) implementation. Fix this.
>
> Fixes: ba451366bf44 ("libbpf: Implement basic split BTF support")
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>

The patch LGTM. We did some cross checking between libbpf.map
and the implementation. What things are missing here to
capture missed implementation or LIBBPF_API marking?

Acked-by: Yonghong Song <yonghong.song@linux.dev>

> ---
>   tools/lib/bpf/btf.c      | 5 +++++
>   tools/lib/bpf/libbpf.map | 3 ++-
>   2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index 95db88b36cf3..845034d15420 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -1079,6 +1079,11 @@ struct btf *btf__new(const void *data, __u32 size)
>   	return libbpf_ptr(btf_new(data, size, NULL));
>   }
>   
> +struct btf *btf__new_split(const void *data, __u32 size, struct btf *base_btf)
> +{
> +	return libbpf_ptr(btf_new(data, size, base_btf));
> +}
> +
>   static struct btf *btf_parse_elf(const char *path, struct btf *base_btf,
>   				 struct btf_ext **btf_ext)
>   {
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index d9e1f57534fa..386964f572a8 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -245,7 +245,6 @@ LIBBPF_0.3.0 {
>   		btf__parse_raw_split;
>   		btf__parse_split;
>   		btf__new_empty_split;
> -		btf__new_split;
>   		ring_buffer__epoll_fd;
>   } LIBBPF_0.2.0;
>   
> @@ -411,5 +410,7 @@ LIBBPF_1.3.0 {
>   } LIBBPF_1.2.0;
>   
>   LIBBPF_1.4.0 {
> +	global:
>   		bpf_token_create;
> +		btf__new_split;
>   } LIBBPF_1.3.0;

Re: [PATCH bpf-next 4/5] libbpf: add missed btf_ext__raw_data() API

[Yonghong Song]

On 1/30/24 11:36 AM, Andrii Nakryiko wrote:
> Another API that was declared in libbpf.map but actual implementation
> was missing. btf_ext__get_raw_data() was intended as a discouraged alias
> to consistently-named btf_ext__raw_data(), so make this an actuality.
>
> Fixes: 20eccf29e297 ("libbpf: hide and discourage inconsistently named getters")
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>

Acked-by: Yonghong Song <yonghong.song@linux.dev>

Re: [PATCH bpf-next 5/5] selftests/bpf: fix bench runner SIGSEGV

[Yonghong Song]

On 1/30/24 11:36 AM, Andrii Nakryiko wrote:
> Some benchmarks don't anticipate "consumer" and/or "producer" sides. Add

For this, you mean some future potential benchmarks, right?

> NULL checks in corresponding places and warn about inappropriate
> consumer/producer count argument values.
>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>

Acked-by: Yonghong Song <yonghong.song@linux.dev>

> ---
>   tools/testing/selftests/bpf/bench.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/bench.c b/tools/testing/selftests/bpf/bench.c
> index 73ce11b0547d..36962fc305eb 100644
> --- a/tools/testing/selftests/bpf/bench.c
> +++ b/tools/testing/selftests/bpf/bench.c
> @@ -330,7 +330,7 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
>   		break;
>   	case 'c':
>   		env.consumer_cnt = strtol(arg, NULL, 10);
> -		if (env.consumer_cnt <= 0) {
> +		if (env.consumer_cnt < 0) {
>   			fprintf(stderr, "Invalid consumer count: %s\n", arg);
>   			argp_usage(state);
>   		}
> @@ -607,6 +607,10 @@ static void setup_benchmark(void)
>   		bench->setup();
>   
>   	for (i = 0; i < env.consumer_cnt; i++) {
> +		if (!bench->consumer_thread) {
> +			fprintf(stderr, "benchmark doesn't have consumers!\n");
> +			exit(1);
> +		}
>   		err = pthread_create(&state.consumers[i], NULL,
>   				     bench->consumer_thread, (void *)(long)i);
>   		if (err) {
> @@ -626,6 +630,10 @@ static void setup_benchmark(void)
>   		env.prod_cpus.next_cpu = env.cons_cpus.next_cpu;
>   
>   	for (i = 0; i < env.producer_cnt; i++) {
> +		if (!bench->producer_thread) {
> +			fprintf(stderr, "benchmark doesn't have producers!\n");
> +			exit(1);
> +		}
>   		err = pthread_create(&state.producers[i], NULL,
>   				     bench->producer_thread, (void *)(long)i);
>   		if (err) {

Re: [PATCH bpf-next 2/5] libbpf: add missing LIBBPF_API annotation to libbpf_set_memlock_rlim API

[Andrii Nakryiko]

On Tue, Jan 30, 2024 at 9:16 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>
>
> On 1/30/24 11:36 AM, Andrii Nakryiko wrote:
> > LIBBPF_API annotation seems missing on libbpf_set_memlock_rlim API, so
> > add it to make this API callable from libbpf's shared library version.
> >
> > Fixes: e542f2c4cd16 ("libbpf: Auto-bump RLIMIT_MEMLOCK if kernel needs it for BPF")
>
> Maybe we should the following commit as Fixes?
>
>    ab9a5a05dc48 libbpf: fix up few libbpf.map problems
>

The one I referenced introduced the problem, the ab9a5a05dc48 one
fixed some problems, but not all of them (for
libbpf_set_memlock_rlim). So it feels like pointing to the originating
commit is better?

> Other than the above, LGTM.
>
> Acked-by: Yonghong Song <yonghong.song@linux.dev>
>
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> >   tools/lib/bpf/bpf.h | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> > index 1441f642c563..f866e98b2436 100644
> > --- a/tools/lib/bpf/bpf.h
> > +++ b/tools/lib/bpf/bpf.h
> > @@ -35,7 +35,7 @@
> >   extern "C" {
> >   #endif
> >
> > -int libbpf_set_memlock_rlim(size_t memlock_bytes);
> > +LIBBPF_API int libbpf_set_memlock_rlim(size_t memlock_bytes);
> >
> >   struct bpf_map_create_opts {
> >       size_t sz; /* size of this struct for forward/backward compatibility */

Re: [PATCH bpf-next 5/5] selftests/bpf: fix bench runner SIGSEGV

[Andrii Nakryiko]

On Tue, Jan 30, 2024 at 11:41 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>
>
> On 1/30/24 11:36 AM, Andrii Nakryiko wrote:
> > Some benchmarks don't anticipate "consumer" and/or "producer" sides. Add
>
> For this, you mean some future potential benchmarks, right?

No, existing ones as well. Like trig-tp and other "trigger"
benchmarks. I ran into this when I was trying to set consumers to 0
explicitly, which wasn't allowed due to <= check. Then I fixed the
check, and I ran into SIGSEGV. So I decided to fix that up.

>
> > NULL checks in corresponding places and warn about inappropriate
> > consumer/producer count argument values.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
>
> Acked-by: Yonghong Song <yonghong.song@linux.dev>
>
> > ---
> >   tools/testing/selftests/bpf/bench.c | 10 +++++++++-
> >   1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/bpf/bench.c b/tools/testing/selftests/bpf/bench.c
> > index 73ce11b0547d..36962fc305eb 100644
> > --- a/tools/testing/selftests/bpf/bench.c
> > +++ b/tools/testing/selftests/bpf/bench.c
> > @@ -330,7 +330,7 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
> >               break;
> >       case 'c':
> >               env.consumer_cnt = strtol(arg, NULL, 10);
> > -             if (env.consumer_cnt <= 0) {
> > +             if (env.consumer_cnt < 0) {
> >                       fprintf(stderr, "Invalid consumer count: %s\n", arg);
> >                       argp_usage(state);
> >               }
> > @@ -607,6 +607,10 @@ static void setup_benchmark(void)
> >               bench->setup();
> >
> >       for (i = 0; i < env.consumer_cnt; i++) {
> > +             if (!bench->consumer_thread) {
> > +                     fprintf(stderr, "benchmark doesn't have consumers!\n");
> > +                     exit(1);
> > +             }
> >               err = pthread_create(&state.consumers[i], NULL,
> >                                    bench->consumer_thread, (void *)(long)i);
> >               if (err) {
> > @@ -626,6 +630,10 @@ static void setup_benchmark(void)
> >               env.prod_cpus.next_cpu = env.cons_cpus.next_cpu;
> >
> >       for (i = 0; i < env.producer_cnt; i++) {
> > +             if (!bench->producer_thread) {
> > +                     fprintf(stderr, "benchmark doesn't have producers!\n");
> > +                     exit(1);
> > +             }
> >               err = pthread_create(&state.producers[i], NULL,
> >                                    bench->producer_thread, (void *)(long)i);
> >               if (err) {
>

Re: [PATCH bpf-next 3/5] libbpf: add btf__new_split() API that was declared but not implemented

[Andrii Nakryiko]

On Tue, Jan 30, 2024 at 9:30 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>
>
> On 1/30/24 11:36 AM, Andrii Nakryiko wrote:
> > Seems like original commit adding split BTF support intended to add
> > btf__new_split() API, and even declared it in libbpf.map, but never
> > added (trivial) implementation. Fix this.
> >
> > Fixes: ba451366bf44 ("libbpf: Implement basic split BTF support")
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
>
> The patch LGTM. We did some cross checking between libbpf.map
> and the implementation. What things are missing here to
> capture missed implementation or LIBBPF_API marking?

Yes, we still have it, and it does detect issues when API wasn't added
into libbpf.map.

I haven't investigated exactly why it didn't catch the issue when API
is in libbpf.map, but is not marked with LIBBPF_API, but I suspect
it's because existing check doesn't take into account visibility of
the symbol, so there is some room for improvement.

Similarly, not sure why it didn't detect that btf_ext__new_split()
wasn't even implemented, probably because we don't distinguish UNDEF
and FUNC symbols? So something to follow up on for sure.

>
> Acked-by: Yonghong Song <yonghong.song@linux.dev>
>
> > ---
> >   tools/lib/bpf/btf.c      | 5 +++++
> >   tools/lib/bpf/libbpf.map | 3 ++-
> >   2 files changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> > index 95db88b36cf3..845034d15420 100644
> > --- a/tools/lib/bpf/btf.c
> > +++ b/tools/lib/bpf/btf.c
> > @@ -1079,6 +1079,11 @@ struct btf *btf__new(const void *data, __u32 size)
> >       return libbpf_ptr(btf_new(data, size, NULL));
> >   }
> >
> > +struct btf *btf__new_split(const void *data, __u32 size, struct btf *base_btf)
> > +{
> > +     return libbpf_ptr(btf_new(data, size, base_btf));
> > +}
> > +
> >   static struct btf *btf_parse_elf(const char *path, struct btf *base_btf,
> >                                struct btf_ext **btf_ext)
> >   {
> > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > index d9e1f57534fa..386964f572a8 100644
> > --- a/tools/lib/bpf/libbpf.map
> > +++ b/tools/lib/bpf/libbpf.map
> > @@ -245,7 +245,6 @@ LIBBPF_0.3.0 {
> >               btf__parse_raw_split;
> >               btf__parse_split;
> >               btf__new_empty_split;
> > -             btf__new_split;
> >               ring_buffer__epoll_fd;
> >   } LIBBPF_0.2.0;
> >
> > @@ -411,5 +410,7 @@ LIBBPF_1.3.0 {
> >   } LIBBPF_1.2.0;
> >
> >   LIBBPF_1.4.0 {
> > +     global:
> >               bpf_token_create;
> > +             btf__new_split;
> >   } LIBBPF_1.3.0;

Re: [PATCH bpf-next 2/5] libbpf: add missing LIBBPF_API annotation to libbpf_set_memlock_rlim API

[Yonghong Song]

On 1/31/24 9:09 AM, Andrii Nakryiko wrote:
> On Tue, Jan 30, 2024 at 9:16 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>>
>> On 1/30/24 11:36 AM, Andrii Nakryiko wrote:
>>> LIBBPF_API annotation seems missing on libbpf_set_memlock_rlim API, so
>>> add it to make this API callable from libbpf's shared library version.
>>>
>>> Fixes: e542f2c4cd16 ("libbpf: Auto-bump RLIMIT_MEMLOCK if kernel needs it for BPF")
>> Maybe we should the following commit as Fixes?
>>
>>     ab9a5a05dc48 libbpf: fix up few libbpf.map problems
>>
> The one I referenced introduced the problem, the ab9a5a05dc48 one
> fixed some problems, but not all of them (for
> libbpf_set_memlock_rlim). So it feels like pointing to the originating
> commit is better?

Maybe we can put two Fixes here? Just having e542f2c4cd16 is a little
confusing since libbpf_set_memlock_rlim is not in libbpf.map with
e542f2c4cd16.

>
>> Other than the above, LGTM.
>>
>> Acked-by: Yonghong Song <yonghong.song@linux.dev>
>>
>>> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
>>> ---
>>>    tools/lib/bpf/bpf.h | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
>>> index 1441f642c563..f866e98b2436 100644
>>> --- a/tools/lib/bpf/bpf.h
>>> +++ b/tools/lib/bpf/bpf.h
>>> @@ -35,7 +35,7 @@
>>>    extern "C" {
>>>    #endif
>>>
>>> -int libbpf_set_memlock_rlim(size_t memlock_bytes);
>>> +LIBBPF_API int libbpf_set_memlock_rlim(size_t memlock_bytes);
>>>
>>>    struct bpf_map_create_opts {
>>>        size_t sz; /* size of this struct for forward/backward compatibility */

Re: [PATCH bpf-next 5/5] selftests/bpf: fix bench runner SIGSEGV

[Yonghong Song]

On 1/31/24 9:17 AM, Andrii Nakryiko wrote:
> On Tue, Jan 30, 2024 at 11:41 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>>
>> On 1/30/24 11:36 AM, Andrii Nakryiko wrote:
>>> Some benchmarks don't anticipate "consumer" and/or "producer" sides. Add
>> For this, you mean some future potential benchmarks, right?
> No, existing ones as well. Like trig-tp and other "trigger"
> benchmarks. I ran into this when I was trying to set consumers to 0
> explicitly, which wasn't allowed due to <= check. Then I fixed the
> check, and I ran into SIGSEGV. So I decided to fix that up.

Some description like this in the commit message will be good.

>
>>> NULL checks in corresponding places and warn about inappropriate
>>> consumer/producer count argument values.
>>>
>>> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
>> Acked-by: Yonghong Song <yonghong.song@linux.dev>
>>
>>> ---
>>>    tools/testing/selftests/bpf/bench.c | 10 +++++++++-
>>>    1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/testing/selftests/bpf/bench.c b/tools/testing/selftests/bpf/bench.c
>>> index 73ce11b0547d..36962fc305eb 100644
>>> --- a/tools/testing/selftests/bpf/bench.c
>>> +++ b/tools/testing/selftests/bpf/bench.c
>>> @@ -330,7 +330,7 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
>>>                break;
>>>        case 'c':
>>>                env.consumer_cnt = strtol(arg, NULL, 10);
>>> -             if (env.consumer_cnt <= 0) {
>>> +             if (env.consumer_cnt < 0) {
>>>                        fprintf(stderr, "Invalid consumer count: %s\n", arg);
>>>                        argp_usage(state);
>>>                }
>>> @@ -607,6 +607,10 @@ static void setup_benchmark(void)
>>>                bench->setup();
>>>
>>>        for (i = 0; i < env.consumer_cnt; i++) {
>>> +             if (!bench->consumer_thread) {
>>> +                     fprintf(stderr, "benchmark doesn't have consumers!\n");
>>> +                     exit(1);
>>> +             }
>>>                err = pthread_create(&state.consumers[i], NULL,
>>>                                     bench->consumer_thread, (void *)(long)i);
>>>                if (err) {
>>> @@ -626,6 +630,10 @@ static void setup_benchmark(void)
>>>                env.prod_cpus.next_cpu = env.cons_cpus.next_cpu;
>>>
>>>        for (i = 0; i < env.producer_cnt; i++) {
>>> +             if (!bench->producer_thread) {
>>> +                     fprintf(stderr, "benchmark doesn't have producers!\n");
>>> +                     exit(1);
>>> +             }
>>>                err = pthread_create(&state.producers[i], NULL,
>>>                                     bench->producer_thread, (void *)(long)i);
>>>                if (err) {

Re: [PATCH bpf-next 3/5] libbpf: add btf__new_split() API that was declared but not implemented

[Yonghong Song]

On 1/31/24 9:20 AM, Andrii Nakryiko wrote:
> On Tue, Jan 30, 2024 at 9:30 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>>
>> On 1/30/24 11:36 AM, Andrii Nakryiko wrote:
>>> Seems like original commit adding split BTF support intended to add
>>> btf__new_split() API, and even declared it in libbpf.map, but never
>>> added (trivial) implementation. Fix this.
>>>
>>> Fixes: ba451366bf44 ("libbpf: Implement basic split BTF support")
>>> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
>> The patch LGTM. We did some cross checking between libbpf.map
>> and the implementation. What things are missing here to
>> capture missed implementation or LIBBPF_API marking?
> Yes, we still have it, and it does detect issues when API wasn't added
> into libbpf.map.
>
> I haven't investigated exactly why it didn't catch the issue when API
> is in libbpf.map, but is not marked with LIBBPF_API, but I suspect
> it's because existing check doesn't take into account visibility of
> the symbol, so there is some room for improvement.
>
> Similarly, not sure why it didn't detect that btf_ext__new_split()
> wasn't even implemented, probably because we don't distinguish UNDEF
> and FUNC symbols? So something to follow up on for sure.

Agreed. A followup to improve API matching between libbpf.map and code
can be done later.

>
>> Acked-by: Yonghong Song <yonghong.song@linux.dev>
>>
>>> ---
>>>    tools/lib/bpf/btf.c      | 5 +++++
>>>    tools/lib/bpf/libbpf.map | 3 ++-
>>>    2 files changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
>>> index 95db88b36cf3..845034d15420 100644
>>> --- a/tools/lib/bpf/btf.c
>>> +++ b/tools/lib/bpf/btf.c
>>> @@ -1079,6 +1079,11 @@ struct btf *btf__new(const void *data, __u32 size)
>>>        return libbpf_ptr(btf_new(data, size, NULL));
>>>    }
>>>
>>> +struct btf *btf__new_split(const void *data, __u32 size, struct btf *base_btf)
>>> +{
>>> +     return libbpf_ptr(btf_new(data, size, base_btf));
>>> +}
>>> +
>>>    static struct btf *btf_parse_elf(const char *path, struct btf *base_btf,
>>>                                 struct btf_ext **btf_ext)
>>>    {
>>> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
>>> index d9e1f57534fa..386964f572a8 100644
>>> --- a/tools/lib/bpf/libbpf.map
>>> +++ b/tools/lib/bpf/libbpf.map
>>> @@ -245,7 +245,6 @@ LIBBPF_0.3.0 {
>>>                btf__parse_raw_split;
>>>                btf__parse_split;
>>>                btf__new_empty_split;
>>> -             btf__new_split;
>>>                ring_buffer__epoll_fd;
>>>    } LIBBPF_0.2.0;
>>>
>>> @@ -411,5 +410,7 @@ LIBBPF_1.3.0 {
>>>    } LIBBPF_1.2.0;
>>>
>>>    LIBBPF_1.4.0 {
>>> +     global:
>>>                bpf_token_create;
>>> +             btf__new_split;
>>>    } LIBBPF_1.3.0;

Re: [PATCH bpf-next 2/5] libbpf: add missing LIBBPF_API annotation to libbpf_set_memlock_rlim API

[Andrii Nakryiko]

On Wed, Jan 31, 2024 at 9:23 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>
>
> On 1/31/24 9:09 AM, Andrii Nakryiko wrote:
> > On Tue, Jan 30, 2024 at 9:16 PM Yonghong Song <yonghong.song@linux.dev> wrote:
> >>
> >> On 1/30/24 11:36 AM, Andrii Nakryiko wrote:
> >>> LIBBPF_API annotation seems missing on libbpf_set_memlock_rlim API, so
> >>> add it to make this API callable from libbpf's shared library version.
> >>>
> >>> Fixes: e542f2c4cd16 ("libbpf: Auto-bump RLIMIT_MEMLOCK if kernel needs it for BPF")
> >> Maybe we should the following commit as Fixes?
> >>
> >>     ab9a5a05dc48 libbpf: fix up few libbpf.map problems
> >>
> > The one I referenced introduced the problem, the ab9a5a05dc48 one
> > fixed some problems, but not all of them (for
> > libbpf_set_memlock_rlim). So it feels like pointing to the originating
> > commit is better?
>
> Maybe we can put two Fixes here? Just having e542f2c4cd16 is a little
> confusing since libbpf_set_memlock_rlim is not in libbpf.map with
> e542f2c4cd16.

I don't mind, but I'll hold off on sending v2 just for this, maybe
someone can add it while applying.

>
> >
> >> Other than the above, LGTM.
> >>
> >> Acked-by: Yonghong Song <yonghong.song@linux.dev>
> >>
> >>> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> >>> ---
> >>>    tools/lib/bpf/bpf.h | 2 +-
> >>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> >>> index 1441f642c563..f866e98b2436 100644
> >>> --- a/tools/lib/bpf/bpf.h
> >>> +++ b/tools/lib/bpf/bpf.h
> >>> @@ -35,7 +35,7 @@
> >>>    extern "C" {
> >>>    #endif
> >>>
> >>> -int libbpf_set_memlock_rlim(size_t memlock_bytes);
> >>> +LIBBPF_API int libbpf_set_memlock_rlim(size_t memlock_bytes);
> >>>
> >>>    struct bpf_map_create_opts {
> >>>        size_t sz; /* size of this struct for forward/backward compatibility */