[PATCH bpf-next 1/2] libbpf: Support uniform BTF-defined key/value specification across all BPF maps

[PATCH bpf-next 1/2] libbpf: Support uniform BTF-defined key/value specification across all BPF maps

[Hengqi Chen]

A bunch of BPF maps do not support specifying types for key and value.
This is non-uniform and inconvenient[0]. Currently, libbpf uses a retry
logic which removes BTF type IDs when BPF map creation failed. Instead
of retrying, this commit recognizes those specialized map and removes
BTF type IDs when creating BPF map.

  [0] Closes: https://github.com/libbpf/libbpf/issues/355

Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
---
 tools/lib/bpf/libbpf.c | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 88d8825fc6f6..7068c4d07337 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -4613,6 +4613,26 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
 			create_attr.inner_map_fd = map->inner_map_fd;
 	}
 
+	if (def->type == BPF_MAP_TYPE_PERF_EVENT_ARRAY ||
+	    def->type == BPF_MAP_TYPE_STACK_TRACE ||
+	    def->type == BPF_MAP_TYPE_CGROUP_ARRAY ||
+	    def->type == BPF_MAP_TYPE_ARRAY_OF_MAPS ||
+	    def->type == BPF_MAP_TYPE_HASH_OF_MAPS ||
+	    def->type == BPF_MAP_TYPE_DEVMAP ||
+	    def->type == BPF_MAP_TYPE_SOCKMAP ||
+	    def->type == BPF_MAP_TYPE_CPUMAP ||
+	    def->type == BPF_MAP_TYPE_XSKMAP ||
+	    def->type == BPF_MAP_TYPE_SOCKHASH ||
+	    def->type == BPF_MAP_TYPE_QUEUE ||
+	    def->type == BPF_MAP_TYPE_STACK ||
+	    def->type == BPF_MAP_TYPE_DEVMAP_HASH) {
+		create_attr.btf_fd = 0;
+		create_attr.btf_key_type_id = 0;
+		create_attr.btf_value_type_id = 0;
+		map->btf_key_type_id = 0;
+		map->btf_value_type_id = 0;
+	}
+
 	if (obj->gen_loader) {
 		bpf_gen__map_create(obj->gen_loader, &create_attr, is_inner ? -1 : map - obj->maps);
 		/* Pretend to have valid FD to pass various fd >= 0 checks.
@@ -4622,21 +4642,6 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
 	} else {
 		map->fd = bpf_create_map_xattr(&create_attr);
 	}
-	if (map->fd < 0 && (create_attr.btf_key_type_id ||
-			    create_attr.btf_value_type_id)) {
-		char *cp, errmsg[STRERR_BUFSIZE];
-
-		err = -errno;
-		cp = libbpf_strerror_r(err, errmsg, sizeof(errmsg));
-		pr_warn("Error in bpf_create_map_xattr(%s):%s(%d). Retrying without BTF.\n",
-			map->name, cp, err);
-		create_attr.btf_fd = 0;
-		create_attr.btf_key_type_id = 0;
-		create_attr.btf_value_type_id = 0;
-		map->btf_key_type_id = 0;
-		map->btf_value_type_id = 0;
-		map->fd = bpf_create_map_xattr(&create_attr);
-	}
 
 	err = map->fd < 0 ? -errno : 0;
 
-- 
2.25.1

[PATCH bpf-next 2/2] selftests/bpf: Test BPF map creation using BTF-defined key/value

[Hengqi Chen]

Test BPF map creation using BTF-defined key/value. The test defines
some specialized maps by specifying BTF types for key/value and
checks those maps are correctly initialized and loaded.

Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
---
 .../selftests/bpf/prog_tests/map_create.c     |  87 ++++++++++++++
 .../selftests/bpf/progs/test_map_create.c     | 110 ++++++++++++++++++
 2 files changed, 197 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/map_create.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_map_create.c

diff --git a/tools/testing/selftests/bpf/prog_tests/map_create.c b/tools/testing/selftests/bpf/prog_tests/map_create.c
new file mode 100644
index 000000000000..6ca32d0dffd2
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/map_create.c
@@ -0,0 +1,87 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2021 Hengqi Chen */
+
+#include <test_progs.h>
+#include "test_map_create.skel.h"
+
+void test_map_create(void)
+{
+	struct test_map_create *skel;
+	int err, fd;
+
+	skel = test_map_create__open();
+	if (!ASSERT_OK_PTR(skel, "test_map_create__open failed"))
+		return;
+
+	err = test_map_create__load(skel);
+	if (!ASSERT_OK(err, "test_map_create__load failed"))
+		goto cleanup;
+
+	fd = bpf_map__fd(skel->maps.map1);
+	if (!ASSERT_GT(fd, 0, "bpf_map__fd failed"))
+		goto cleanup;
+	close(fd);
+
+	fd = bpf_map__fd(skel->maps.map2);
+	if (!ASSERT_GT(fd, 0, "bpf_map__fd failed"))
+		goto cleanup;
+	close(fd);
+
+	fd = bpf_map__fd(skel->maps.map3);
+	if (!ASSERT_GT(fd, 0, "bpf_map__fd failed"))
+		goto cleanup;
+	close(fd);
+
+	fd = bpf_map__fd(skel->maps.map4);
+	if (!ASSERT_GT(fd, 0, "bpf_map__fd failed"))
+		goto cleanup;
+	close(fd);
+
+	fd = bpf_map__fd(skel->maps.map5);
+	if (!ASSERT_GT(fd, 0, "bpf_map__fd failed"))
+		goto cleanup;
+	close(fd);
+
+	fd = bpf_map__fd(skel->maps.map6);
+	if (!ASSERT_GT(fd, 0, "bpf_map__fd failed"))
+		goto cleanup;
+	close(fd);
+
+	fd = bpf_map__fd(skel->maps.map7);
+	if (!ASSERT_GT(fd, 0, "bpf_map__fd failed"))
+		goto cleanup;
+	close(fd);
+
+	fd = bpf_map__fd(skel->maps.map8);
+	if (!ASSERT_GT(fd, 0, "bpf_map__fd failed"))
+		goto cleanup;
+	close(fd);
+
+	fd = bpf_map__fd(skel->maps.map9);
+	if (!ASSERT_GT(fd, 0, "bpf_map__fd failed"))
+		goto cleanup;
+	close(fd);
+
+	fd = bpf_map__fd(skel->maps.map10);
+	if (!ASSERT_GT(fd, 0, "bpf_map__fd failed"))
+		goto cleanup;
+	close(fd);
+
+	fd = bpf_map__fd(skel->maps.map11);
+	if (!ASSERT_GT(fd, 0, "bpf_map__fd failed"))
+		goto cleanup;
+	close(fd);
+
+	fd = bpf_map__fd(skel->maps.map12);
+	if (!ASSERT_GT(fd, 0, "bpf_map__fd failed"))
+		goto cleanup;
+	close(fd);
+
+	fd = bpf_map__fd(skel->maps.map13);
+	if (!ASSERT_GT(fd, 0, "bpf_map__fd failed"))
+		goto cleanup;
+	close(fd);
+
+cleanup:
+	test_map_create__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_map_create.c b/tools/testing/selftests/bpf/progs/test_map_create.c
new file mode 100644
index 000000000000..2e9950e56b0f
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_map_create.c
@@ -0,0 +1,110 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2021 Hengqi Chen */
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+
+#define MAX_ENTRIES	8
+
+struct {
+	__uint(type,BPF_MAP_TYPE_PERF_EVENT_ARRAY);
+	__uint(max_entries, MAX_ENTRIES);
+	__type(key, int);
+	__type(value, int);
+} map1 SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_STACK_TRACE);
+	__uint(max_entries, MAX_ENTRIES);
+	__type(key, int);
+	__type(value, __u64);
+} map2 SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_CGROUP_ARRAY);
+	__uint(max_entries, MAX_ENTRIES);
+	__type(key, int);
+	__type(value, int);
+} map3 SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY_OF_MAPS);
+	__uint(max_entries, MAX_ENTRIES);
+	__type(key, int);
+	__type(value, int);
+	__array(values, struct {
+		__uint(type, BPF_MAP_TYPE_ARRAY);
+		__uint(max_entries, 1);
+		__type(key, __u32);
+		__type(value, __u32);
+	});
+} map4 SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH_OF_MAPS);
+	__uint(max_entries, MAX_ENTRIES);
+	__type(key, int);
+	__type(value, int);
+	__array(values, struct {
+		__uint(type, BPF_MAP_TYPE_ARRAY);
+		__uint(max_entries, 1);
+		__type(key, __u32);
+		__type(value, __u32);
+	});
+} map5 SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_DEVMAP);
+	__uint(max_entries, MAX_ENTRIES);
+	__type(key, int);
+	__type(value, int);
+} map6 SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_SOCKMAP);
+	__uint(max_entries, MAX_ENTRIES);
+	__type(key, int);
+	__type(value, int);
+} map7 SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_CPUMAP);
+	__uint(max_entries, MAX_ENTRIES);
+	__type(key, int);
+	__type(value, int);
+} map8 SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_XSKMAP);
+	__uint(max_entries, MAX_ENTRIES);
+	__type(key, int);
+	__type(value, int);
+} map9 SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_SOCKHASH);
+	__uint(max_entries, MAX_ENTRIES);
+	__type(key, int);
+	__type(value, int);
+} map10 SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_QUEUE);
+	__uint(max_entries, MAX_ENTRIES);
+	__type(value, int);
+} map11 SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_STACK);
+	__uint(max_entries, MAX_ENTRIES);
+	__type(value, int);
+} map12 SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_DEVMAP_HASH);
+	__uint(max_entries, MAX_ENTRIES);
+	__type(key, int);
+	__type(value, int);
+} map13 SEC(".maps");
+
+char _license[] SEC("license") = "GPL";
-- 
2.25.1

Re: [PATCH bpf-next 2/2] selftests/bpf: Test BPF map creation using BTF-defined key/value

[Andrii Nakryiko]

On Sun, Sep 5, 2021 at 3:09 AM Hengqi Chen <hengqi.chen@gmail.com> wrote:
>
> Test BPF map creation using BTF-defined key/value. The test defines
> some specialized maps by specifying BTF types for key/value and
> checks those maps are correctly initialized and loaded.
>
> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> ---
>  .../selftests/bpf/prog_tests/map_create.c     |  87 ++++++++++++++
>  .../selftests/bpf/progs/test_map_create.c     | 110 ++++++++++++++++++
>  2 files changed, 197 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/map_create.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_map_create.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/map_create.c b/tools/testing/selftests/bpf/prog_tests/map_create.c
> new file mode 100644
> index 000000000000..6ca32d0dffd2
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/map_create.c
> @@ -0,0 +1,87 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (c) 2021 Hengqi Chen */
> +
> +#include <test_progs.h>
> +#include "test_map_create.skel.h"
> +
> +void test_map_create(void)
> +{
> +       struct test_map_create *skel;
> +       int err, fd;
> +
> +       skel = test_map_create__open();
> +       if (!ASSERT_OK_PTR(skel, "test_map_create__open failed"))
> +               return;
> +
> +       err = test_map_create__load(skel);

If load() succeeds, all the maps will definitely be created, so all
the below tests are meaningless.

I think it's better to just change all the existing map definitions
used throughout selftests to use key/value types, instead of
key_size/value_size. That will automatically test this feature without
adding an extra test. Unfortunately to really test that the logic is
working, we'd need to check that libbpf doesn't emit the warning about
retrying map creation w/o BTF, but I think one-time manual check
(please use ./test_progs -v to see libbpf warnings during tests)
should be sufficient for this.

> +       if (!ASSERT_OK(err, "test_map_create__load failed"))
> +               goto cleanup;
> +
> +       fd = bpf_map__fd(skel->maps.map1);
> +       if (!ASSERT_GT(fd, 0, "bpf_map__fd failed"))
> +               goto cleanup;
> +       close(fd);
> +
> +       fd = bpf_map__fd(skel->maps.map2);
> +       if (!ASSERT_GT(fd, 0, "bpf_map__fd failed"))
> +               goto cleanup;
> +       close(fd);

[...]

Re: [PATCH bpf-next 2/2] selftests/bpf: Test BPF map creation using BTF-defined key/value

[Hengqi Chen]

On 9/9/21 12:29 PM, Andrii Nakryiko wrote:
> On Sun, Sep 5, 2021 at 3:09 AM Hengqi Chen <hengqi.chen@gmail.com> wrote:
>>
>> Test BPF map creation using BTF-defined key/value. The test defines
>> some specialized maps by specifying BTF types for key/value and
>> checks those maps are correctly initialized and loaded.
>>
>> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
>> ---
>>  .../selftests/bpf/prog_tests/map_create.c     |  87 ++++++++++++++
>>  .../selftests/bpf/progs/test_map_create.c     | 110 ++++++++++++++++++
>>  2 files changed, 197 insertions(+)
>>  create mode 100644 tools/testing/selftests/bpf/prog_tests/map_create.c
>>  create mode 100644 tools/testing/selftests/bpf/progs/test_map_create.c
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/map_create.c b/tools/testing/selftests/bpf/prog_tests/map_create.c
>> new file mode 100644
>> index 000000000000..6ca32d0dffd2
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/prog_tests/map_create.c
>> @@ -0,0 +1,87 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright (c) 2021 Hengqi Chen */
>> +
>> +#include <test_progs.h>
>> +#include "test_map_create.skel.h"
>> +
>> +void test_map_create(void)
>> +{
>> +       struct test_map_create *skel;
>> +       int err, fd;
>> +
>> +       skel = test_map_create__open();
>> +       if (!ASSERT_OK_PTR(skel, "test_map_create__open failed"))
>> +               return;
>> +
>> +       err = test_map_create__load(skel);
> 
> If load() succeeds, all the maps will definitely be created, so all
> the below tests are meaningless.
> 
> I think it's better to just change all the existing map definitions
> used throughout selftests to use key/value types, instead of
> key_size/value_size. That will automatically test this feature without
> adding an extra test. Unfortunately to really test that the logic is
> working, we'd need to check that libbpf doesn't emit the warning about
> retrying map creation w/o BTF, but I think one-time manual check
> (please use ./test_progs -v to see libbpf warnings during tests)
> should be sufficient for this.
> 

Hello, Andrii

I updated these existing tests as you suggested, 
but I was unable to run the whole bpf selftests locally.

Running ./test_progs -v made my system hang up,
didn't find the root cause yet.

Instead I just run the modified test with the following commands:

sudo ./test_progs -v --name=kfree_skb,perf_event_stackmap,btf_map_in_map,pe_preserve_elems,stacktrace_map,stacktrace_build_id,xdp_bpf2bpf,select_reuseport,tcpbpf_user

>> +       if (!ASSERT_OK(err, "test_map_create__load failed"))
>> +               goto cleanup;
>> +
>> +       fd = bpf_map__fd(skel->maps.map1);
>> +       if (!ASSERT_GT(fd, 0, "bpf_map__fd failed"))
>> +               goto cleanup;
>> +       close(fd);
>> +
>> +       fd = bpf_map__fd(skel->maps.map2);
>> +       if (!ASSERT_GT(fd, 0, "bpf_map__fd failed"))
>> +               goto cleanup;
>> +       close(fd);
> 
> [...]
> 

Re: [PATCH bpf-next 2/2] selftests/bpf: Test BPF map creation using BTF-defined key/value

[Andrii Nakryiko]

On Thu, Sep 30, 2021 at 9:05 AM Hengqi Chen <hengqi.chen@gmail.com> wrote:
>
>
>
> On 9/9/21 12:29 PM, Andrii Nakryiko wrote:
> > On Sun, Sep 5, 2021 at 3:09 AM Hengqi Chen <hengqi.chen@gmail.com> wrote:
> >>
> >> Test BPF map creation using BTF-defined key/value. The test defines
> >> some specialized maps by specifying BTF types for key/value and
> >> checks those maps are correctly initialized and loaded.
> >>
> >> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> >> ---
> >>  .../selftests/bpf/prog_tests/map_create.c     |  87 ++++++++++++++
> >>  .../selftests/bpf/progs/test_map_create.c     | 110 ++++++++++++++++++
> >>  2 files changed, 197 insertions(+)
> >>  create mode 100644 tools/testing/selftests/bpf/prog_tests/map_create.c
> >>  create mode 100644 tools/testing/selftests/bpf/progs/test_map_create.c
> >>
> >> diff --git a/tools/testing/selftests/bpf/prog_tests/map_create.c b/tools/testing/selftests/bpf/prog_tests/map_create.c
> >> new file mode 100644
> >> index 000000000000..6ca32d0dffd2
> >> --- /dev/null
> >> +++ b/tools/testing/selftests/bpf/prog_tests/map_create.c
> >> @@ -0,0 +1,87 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +/* Copyright (c) 2021 Hengqi Chen */
> >> +
> >> +#include <test_progs.h>
> >> +#include "test_map_create.skel.h"
> >> +
> >> +void test_map_create(void)
> >> +{
> >> +       struct test_map_create *skel;
> >> +       int err, fd;
> >> +
> >> +       skel = test_map_create__open();
> >> +       if (!ASSERT_OK_PTR(skel, "test_map_create__open failed"))
> >> +               return;
> >> +
> >> +       err = test_map_create__load(skel);
> >
> > If load() succeeds, all the maps will definitely be created, so all
> > the below tests are meaningless.
> >
> > I think it's better to just change all the existing map definitions
> > used throughout selftests to use key/value types, instead of
> > key_size/value_size. That will automatically test this feature without
> > adding an extra test. Unfortunately to really test that the logic is
> > working, we'd need to check that libbpf doesn't emit the warning about
> > retrying map creation w/o BTF, but I think one-time manual check
> > (please use ./test_progs -v to see libbpf warnings during tests)
> > should be sufficient for this.
> >
>
> Hello, Andrii
>
> I updated these existing tests as you suggested,
> but I was unable to run the whole bpf selftests locally.
>
> Running ./test_progs -v made my system hang up,
> didn't find the root cause yet.

This is strange. Do you know at which test this happens? Do you get
kernel warning/oops when this happens in dmesg?

But overall, the development and testing workflow for people working
on bpf/bpf-next involves building latest kernel and running selftests
inside the QEMU VM for testing. We also have vmtest.sh script in
selftests/bpf that automates a lot of building steps. It will build
kernel, test_progs and other selftest binaries, and will spin up QEMU
VM with the same image that our BPF CIs are using. You just need to
have very recent/latest Clang available and similarly pahole (from
dwarves package) should be up to date and available through $PATH.
After that, running ./vmtest.sh will just run all ./test_progs
automatically.

Either way, our CI will also run your changes through the tests
(except there are some intermittent issues right now, so we'll have to
wait a bit for that to kick in). You can monitor [0], or the link will
actually appear on each of your patches (e.g., [1]) in "Checks"
section, once everything is up and running properly.

  [0] https://github.com/kernel-patches/bpf/pulls
  [1] https://patchwork.kernel.org/project/netdevbpf/patch/20210930161456.3444544-2-hengqi.chen@gmail.com/

>
> Instead I just run the modified test with the following commands:
>
> sudo ./test_progs -v --name=kfree_skb,perf_event_stackmap,btf_map_in_map,pe_preserve_elems,stacktrace_map,stacktrace_build_id,xdp_bpf2bpf,select_reuseport,tcpbpf_user
>
> >> +       if (!ASSERT_OK(err, "test_map_create__load failed"))
> >> +               goto cleanup;
> >> +
> >> +       fd = bpf_map__fd(skel->maps.map1);
> >> +       if (!ASSERT_GT(fd, 0, "bpf_map__fd failed"))
> >> +               goto cleanup;
> >> +       close(fd);
> >> +
> >> +       fd = bpf_map__fd(skel->maps.map2);
> >> +       if (!ASSERT_GT(fd, 0, "bpf_map__fd failed"))
> >> +               goto cleanup;
> >> +       close(fd);
> >
> > [...]
> >

Re: [PATCH bpf-next 2/2] selftests/bpf: Test BPF map creation using BTF-defined key/value

[Hengqi Chen]

On 10/1/21 2:39 AM, Andrii Nakryiko wrote:
> On Thu, Sep 30, 2021 at 9:05 AM Hengqi Chen <hengqi.chen@gmail.com> wrote:
>>
>>
>>
>> On 9/9/21 12:29 PM, Andrii Nakryiko wrote:
>>> On Sun, Sep 5, 2021 at 3:09 AM Hengqi Chen <hengqi.chen@gmail.com> wrote:
>>>>
>>>> Test BPF map creation using BTF-defined key/value. The test defines
>>>> some specialized maps by specifying BTF types for key/value and
>>>> checks those maps are correctly initialized and loaded.
>>>>
>>>> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
>>>> ---
>>>>  .../selftests/bpf/prog_tests/map_create.c     |  87 ++++++++++++++
>>>>  .../selftests/bpf/progs/test_map_create.c     | 110 ++++++++++++++++++
>>>>  2 files changed, 197 insertions(+)
>>>>  create mode 100644 tools/testing/selftests/bpf/prog_tests/map_create.c
>>>>  create mode 100644 tools/testing/selftests/bpf/progs/test_map_create.c
>>>>
>>>> diff --git a/tools/testing/selftests/bpf/prog_tests/map_create.c b/tools/testing/selftests/bpf/prog_tests/map_create.c
>>>> new file mode 100644
>>>> index 000000000000..6ca32d0dffd2
>>>> --- /dev/null
>>>> +++ b/tools/testing/selftests/bpf/prog_tests/map_create.c
>>>> @@ -0,0 +1,87 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +/* Copyright (c) 2021 Hengqi Chen */
>>>> +
>>>> +#include <test_progs.h>
>>>> +#include "test_map_create.skel.h"
>>>> +
>>>> +void test_map_create(void)
>>>> +{
>>>> +       struct test_map_create *skel;
>>>> +       int err, fd;
>>>> +
>>>> +       skel = test_map_create__open();
>>>> +       if (!ASSERT_OK_PTR(skel, "test_map_create__open failed"))
>>>> +               return;
>>>> +
>>>> +       err = test_map_create__load(skel);
>>>
>>> If load() succeeds, all the maps will definitely be created, so all
>>> the below tests are meaningless.
>>>
>>> I think it's better to just change all the existing map definitions
>>> used throughout selftests to use key/value types, instead of
>>> key_size/value_size. That will automatically test this feature without
>>> adding an extra test. Unfortunately to really test that the logic is
>>> working, we'd need to check that libbpf doesn't emit the warning about
>>> retrying map creation w/o BTF, but I think one-time manual check
>>> (please use ./test_progs -v to see libbpf warnings during tests)
>>> should be sufficient for this.
>>>
>>
>> Hello, Andrii
>>
>> I updated these existing tests as you suggested,
>> but I was unable to run the whole bpf selftests locally.
>>
>> Running ./test_progs -v made my system hang up,
>> didn't find the root cause yet.
> 
> This is strange. Do you know at which test this happens? Do you get
> kernel warning/oops when this happens in dmesg?
> 

No, when this situation occurred, I just restarted my laptop.
Will look into this later.

> But overall, the development and testing workflow for people working
> on bpf/bpf-next involves building latest kernel and running selftests
> inside the QEMU VM for testing. We also have vmtest.sh script in
> selftests/bpf that automates a lot of building steps. It will build
> kernel, test_progs and other selftest binaries, and will spin up QEMU
> VM with the same image that our BPF CIs are using. You just need to
> have very recent/latest Clang available and similarly pahole (from
> dwarves package) should be up to date and available through $PATH.
> After that, running ./vmtest.sh will just run all ./test_progs
> automatically.
> 

This workflow info is quite useful for me. Thanks.

> Either way, our CI will also run your changes through the tests
> (except there are some intermittent issues right now, so we'll have to
> wait a bit for that to kick in). You can monitor [0], or the link will
> actually appear on each of your patches (e.g., [1]) in "Checks"
> section, once everything is up and running properly.
> 
>   [0] https://github.com/kernel-patches/bpf/pulls
>   [1] https://patchwork.kernel.org/project/netdevbpf/patch/20210930161456.3444544-2-hengqi.chen@gmail.com/
> 
>>
>> Instead I just run the modified test with the following commands:
>>
>> sudo ./test_progs -v --name=kfree_skb,perf_event_stackmap,btf_map_in_map,pe_preserve_elems,stacktrace_map,stacktrace_build_id,xdp_bpf2bpf,select_reuseport,tcpbpf_user
>>
>>>> +       if (!ASSERT_OK(err, "test_map_create__load failed"))
>>>> +               goto cleanup;
>>>> +
>>>> +       fd = bpf_map__fd(skel->maps.map1);
>>>> +       if (!ASSERT_GT(fd, 0, "bpf_map__fd failed"))
>>>> +               goto cleanup;
>>>> +       close(fd);
>>>> +
>>>> +       fd = bpf_map__fd(skel->maps.map2);
>>>> +       if (!ASSERT_GT(fd, 0, "bpf_map__fd failed"))
>>>> +               goto cleanup;
>>>> +       close(fd);
>>>
>>> [...]
>>>

Re: [PATCH bpf-next 1/2] libbpf: Support uniform BTF-defined key/value specification across all BPF maps

[Andrii Nakryiko]

On Sun, Sep 5, 2021 at 3:09 AM Hengqi Chen <hengqi.chen@gmail.com> wrote:
>
> A bunch of BPF maps do not support specifying types for key and value.

s/types/BTF types/, it's a bit confusing otherwise

> This is non-uniform and inconvenient[0]. Currently, libbpf uses a retry
> logic which removes BTF type IDs when BPF map creation failed. Instead
> of retrying, this commit recognizes those specialized map and removes

s/map/maps/

> BTF type IDs when creating BPF map.
>
>   [0] Closes: https://github.com/libbpf/libbpf/issues/355
>
> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> ---

For patch sets consisting of two or more patches, we ask for a cover
letter, so for the next revision please provide a cover letter with an
overall description of what the series is about.

>  tools/lib/bpf/libbpf.c | 35 ++++++++++++++++++++---------------
>  1 file changed, 20 insertions(+), 15 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 88d8825fc6f6..7068c4d07337 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -4613,6 +4613,26 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
>                         create_attr.inner_map_fd = map->inner_map_fd;
>         }
>
> +       if (def->type == BPF_MAP_TYPE_PERF_EVENT_ARRAY ||
> +           def->type == BPF_MAP_TYPE_STACK_TRACE ||
> +           def->type == BPF_MAP_TYPE_CGROUP_ARRAY ||
> +           def->type == BPF_MAP_TYPE_ARRAY_OF_MAPS ||
> +           def->type == BPF_MAP_TYPE_HASH_OF_MAPS ||
> +           def->type == BPF_MAP_TYPE_DEVMAP ||
> +           def->type == BPF_MAP_TYPE_SOCKMAP ||
> +           def->type == BPF_MAP_TYPE_CPUMAP ||
> +           def->type == BPF_MAP_TYPE_XSKMAP ||
> +           def->type == BPF_MAP_TYPE_SOCKHASH ||
> +           def->type == BPF_MAP_TYPE_QUEUE ||
> +           def->type == BPF_MAP_TYPE_STACK ||
> +           def->type == BPF_MAP_TYPE_DEVMAP_HASH) {
> +               create_attr.btf_fd = 0;
> +               create_attr.btf_key_type_id = 0;
> +               create_attr.btf_value_type_id = 0;
> +               map->btf_key_type_id = 0;
> +               map->btf_value_type_id = 0;
> +       }

Let's do this as a more succinct switch statement. Consider also
slightly rearranging entries to keep "related" map types together:
  - SOCKMAP + SOCKHASH
  - DEVMAP + DEVMAP_HASH + CPUMAP + XSKMAP

Thanks!


> +
>         if (obj->gen_loader) {
>                 bpf_gen__map_create(obj->gen_loader, &create_attr, is_inner ? -1 : map - obj->maps);
>                 /* Pretend to have valid FD to pass various fd >= 0 checks.
> @@ -4622,21 +4642,6 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
>         } else {
>                 map->fd = bpf_create_map_xattr(&create_attr);
>         }
> -       if (map->fd < 0 && (create_attr.btf_key_type_id ||
> -                           create_attr.btf_value_type_id)) {
> -               char *cp, errmsg[STRERR_BUFSIZE];
> -
> -               err = -errno;
> -               cp = libbpf_strerror_r(err, errmsg, sizeof(errmsg));
> -               pr_warn("Error in bpf_create_map_xattr(%s):%s(%d). Retrying without BTF.\n",
> -                       map->name, cp, err);
> -               create_attr.btf_fd = 0;
> -               create_attr.btf_key_type_id = 0;
> -               create_attr.btf_value_type_id = 0;
> -               map->btf_key_type_id = 0;
> -               map->btf_value_type_id = 0;
> -               map->fd = bpf_create_map_xattr(&create_attr);
> -       }
>

Please don't remove this fallback logic. There are multiple situations
where libbpf might need to retry map creation without BTF.

>         err = map->fd < 0 ? -errno : 0;
>
> --
> 2.25.1
>

Re: [PATCH bpf-next 1/2] libbpf: Support uniform BTF-defined key/value specification across all BPF maps

[Hengqi Chen]

On 9/9/21 12:26 PM, Andrii Nakryiko wrote:
> On Sun, Sep 5, 2021 at 3:09 AM Hengqi Chen <hengqi.chen@gmail.com> wrote:
>>
>> A bunch of BPF maps do not support specifying types for key and value.
> 
> s/types/BTF types/, it's a bit confusing otherwise
> 
>> This is non-uniform and inconvenient[0]. Currently, libbpf uses a retry
>> logic which removes BTF type IDs when BPF map creation failed. Instead
>> of retrying, this commit recognizes those specialized map and removes
> 
> s/map/maps/
> 
>> BTF type IDs when creating BPF map.
>>
>>   [0] Closes: https://github.com/libbpf/libbpf/issues/355
>>
>> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
>> ---
> 
> For patch sets consisting of two or more patches, we ask for a cover
> letter, so for the next revision please provide a cover letter with an
> overall description of what the series is about.
> 

Hello, Andrii



Sorry for the long delay. Will send a v2 for review.

>>  tools/lib/bpf/libbpf.c | 35 ++++++++++++++++++++---------------
>>  1 file changed, 20 insertions(+), 15 deletions(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 88d8825fc6f6..7068c4d07337 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -4613,6 +4613,26 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
>>                         create_attr.inner_map_fd = map->inner_map_fd;
>>         }
>>
>> +       if (def->type == BPF_MAP_TYPE_PERF_EVENT_ARRAY ||
>> +           def->type == BPF_MAP_TYPE_STACK_TRACE ||
>> +           def->type == BPF_MAP_TYPE_CGROUP_ARRAY ||
>> +           def->type == BPF_MAP_TYPE_ARRAY_OF_MAPS ||
>> +           def->type == BPF_MAP_TYPE_HASH_OF_MAPS ||
>> +           def->type == BPF_MAP_TYPE_DEVMAP ||
>> +           def->type == BPF_MAP_TYPE_SOCKMAP ||
>> +           def->type == BPF_MAP_TYPE_CPUMAP ||
>> +           def->type == BPF_MAP_TYPE_XSKMAP ||
>> +           def->type == BPF_MAP_TYPE_SOCKHASH ||
>> +           def->type == BPF_MAP_TYPE_QUEUE ||
>> +           def->type == BPF_MAP_TYPE_STACK ||
>> +           def->type == BPF_MAP_TYPE_DEVMAP_HASH) {
>> +               create_attr.btf_fd = 0;
>> +               create_attr.btf_key_type_id = 0;
>> +               create_attr.btf_value_type_id = 0;
>> +               map->btf_key_type_id = 0;
>> +               map->btf_value_type_id = 0;
>> +       }
> 
> Let's do this as a more succinct switch statement. Consider also
> slightly rearranging entries to keep "related" map types together:
>   - SOCKMAP + SOCKHASH
>   - DEVMAP + DEVMAP_HASH + CPUMAP + XSKMAP
> 
> Thanks!
>> 
>> +
>>         if (obj->gen_loader) {
>>                 bpf_gen__map_create(obj->gen_loader, &create_attr, is_inner ? -1 : map - obj->maps);
>>                 /* Pretend to have valid FD to pass various fd >= 0 checks.
>> @@ -4622,21 +4642,6 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
>>         } else {
>>                 map->fd = bpf_create_map_xattr(&create_attr);
>>         }
>> -       if (map->fd < 0 && (create_attr.btf_key_type_id ||
>> -                           create_attr.btf_value_type_id)) {
>> -               char *cp, errmsg[STRERR_BUFSIZE];
>> -
>> -               err = -errno;
>> -               cp = libbpf_strerror_r(err, errmsg, sizeof(errmsg));
>> -               pr_warn("Error in bpf_create_map_xattr(%s):%s(%d). Retrying without BTF.\n",
>> -                       map->name, cp, err);
>> -               create_attr.btf_fd = 0;
>> -               create_attr.btf_key_type_id = 0;
>> -               create_attr.btf_value_type_id = 0;
>> -               map->btf_key_type_id = 0;
>> -               map->btf_value_type_id = 0;
>> -               map->fd = bpf_create_map_xattr(&create_attr);
>> -       }
>>
> 
> Please don't remove this fallback logic. There are multiple situations
> where libbpf might need to retry map creation without BTF.
> 
>>         err = map->fd < 0 ? -errno : 0;
>>
>> --
>> 2.25.1
>>

[PATCH bpf-next 0/2] libbpf: Support uniform BTF-defined key/value specification across all BPF maps

[Hengqi Chen]

Currently a bunch of (usually pretty specialized) BPF maps do not support
specifying BTF types for they key and value. For such maps, specifying
their definition like this:

  struct {
      __uint(type, BPF_MAP_TYPE_PERF_EVENT_ARRAY);
      __type(key, int);
      __type(value, int);
  } my_perf_buf SEC(".maps");

Would actually produce warnings about retrying BPF map creation without BTF.
Users are forced to know such nuances and use __uint(key_size, 4) instead.
This is non-uniform, annoying, and inconvenient.

This patch set teaches libbpf to recognize those specialized maps and removes
BTF type IDs when creating BPF map. Also, update existing BPF selftests to
exericse this change.

Hengqi Chen (2):
  libbpf: Support uniform BTF-defined key/value specification across all
    BPF maps
  selftests/bpf: Use BTF-defined key/value for map definitions

 tools/lib/bpf/libbpf.c                        | 24 +++++++++++++++++++
 tools/testing/selftests/bpf/progs/kfree_skb.c |  4 ++--
 .../selftests/bpf/progs/perf_event_stackmap.c |  4 ++--
 .../bpf/progs/sockmap_verdict_prog.c          | 12 +++++-----
 .../selftests/bpf/progs/test_btf_map_in_map.c | 14 +++++------
 .../selftests/bpf/progs/test_map_in_map.c     | 10 ++++----
 .../bpf/progs/test_map_in_map_invalid.c       |  2 +-
 .../bpf/progs/test_pe_preserve_elems.c        |  8 +++----
 .../selftests/bpf/progs/test_perf_buffer.c    |  4 ++--
 .../bpf/progs/test_select_reuseport_kern.c    |  4 ++--
 .../bpf/progs/test_stacktrace_build_id.c      |  4 ++--
 .../selftests/bpf/progs/test_stacktrace_map.c |  4 ++--
 .../selftests/bpf/progs/test_tcpnotify_kern.c |  4 ++--
 .../selftests/bpf/progs/test_xdp_bpf2bpf.c    |  4 ++--
 14 files changed, 62 insertions(+), 40 deletions(-)

--
2.30.2

[PATCH bpf-next 1/2] libbpf: Support uniform BTF-defined key/value specification across all BPF maps

[Hengqi Chen]

A bunch of BPF maps do not support specifying BTF types for key and value.
This is non-uniform and inconvenient[0]. Currently, libbpf uses a retry
logic which removes BTF type IDs when BPF map creation failed. Instead
of retrying, this commit recognizes those specialized maps and removes
BTF type IDs when creating BPF map.

  [0] Closes: https://github.com/libbpf/libbpf/issues/355

Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
---
 tools/lib/bpf/libbpf.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index ef5db34bf913..b96eb3a64cca 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -4643,6 +4643,30 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
 			create_attr.inner_map_fd = map->inner_map_fd;
 	}

+	switch (def->type) {
+	case BPF_MAP_TYPE_PERF_EVENT_ARRAY:
+	case BPF_MAP_TYPE_CGROUP_ARRAY:
+	case BPF_MAP_TYPE_STACK_TRACE:
+	case BPF_MAP_TYPE_ARRAY_OF_MAPS:
+	case BPF_MAP_TYPE_HASH_OF_MAPS:
+	case BPF_MAP_TYPE_DEVMAP:
+	case BPF_MAP_TYPE_DEVMAP_HASH:
+	case BPF_MAP_TYPE_CPUMAP:
+	case BPF_MAP_TYPE_XSKMAP:
+	case BPF_MAP_TYPE_SOCKMAP:
+	case BPF_MAP_TYPE_SOCKHASH:
+	case BPF_MAP_TYPE_QUEUE:
+	case BPF_MAP_TYPE_STACK:
+	case BPF_MAP_TYPE_RINGBUF:
+		create_attr.btf_fd = 0;
+		create_attr.btf_key_type_id = 0;
+		create_attr.btf_value_type_id = 0;
+		map->btf_key_type_id = 0;
+		map->btf_value_type_id = 0;
+	default:
+		break;
+	}
+
 	if (obj->gen_loader) {
 		bpf_gen__map_create(obj->gen_loader, &create_attr, is_inner ? -1 : map - obj->maps);
 		/* Pretend to have valid FD to pass various fd >= 0 checks.
--
2.30.2