netdev - Re: [PATCH bpf-next 3/4] samples: bpf: refactor BPF map performance test with libbpf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4BzZsx+pkkdjhJt1AHaUy6=B=nqZdpR+TrRrjreNa0GMWug@mail.gmail.com>
Date:   Wed, 1 Jul 2020 21:33:52 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     "Daniel T. Lee" <danieltimlee@...il.com>
Cc:     Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Yonghong Song <yhs@...com>, Martin KaFai Lau <kafai@...com>,
        Andrii Nakryiko <andriin@...com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH bpf-next 3/4] samples: bpf: refactor BPF map performance
 test with libbpf

On Wed, Jul 1, 2020 at 7:17 PM Daniel T. Lee <danieltimlee@...il.com> wrote:
>
> Previously, in order to set the numa_node attribute at the time of map
> creation using "libbpf", it was necessary to call bpf_create_map_node()
> directly (bpf_load approach), instead of calling bpf_object_load()
> that handles everything on its own, including map creation. And because
> of this problem, this sample had problems with refactoring from bpf_load
> to libbbpf.
>
> However, by commit 1bdb6c9a1c43 ("libbpf: Add a bunch of attribute
> getters/setters for map definitions"), a helper function which allows
> the numa_node attribute to be set in the map prior to calling
> bpf_object_load() has been added.
>
> By using libbpf instead of bpf_load, the inner map definition has
> been explicitly declared with BTF-defined format. And for this reason
> some logic in fixup_map() was not needed and changed or removed.
>
> Signed-off-by: Daniel T. Lee <danieltimlee@...il.com>
> ---
>  samples/bpf/Makefile             |   2 +-
>  samples/bpf/map_perf_test_kern.c | 180 +++++++++++++++----------------
>  samples/bpf/map_perf_test_user.c | 130 +++++++++++++++-------
>  3 files changed, 181 insertions(+), 131 deletions(-)
>

[...]

> +struct inner_lru {
> +       __uint(type, BPF_MAP_TYPE_LRU_HASH);
> +       __type(key, u32);
> +       __type(value, long);
> +       __uint(max_entries, MAX_ENTRIES);
> +       __uint(map_flags, BPF_F_NUMA_NODE); /* from _user.c, set numa_node to 0 */
> +} inner_lru_hash_map SEC(".maps");

you can declaratively set numa_node here with __uint(numa_node, 0),
which is actually a default, but for explicitness it's better

> +
> +struct {
> +       __uint(type, BPF_MAP_TYPE_ARRAY_OF_MAPS);
> +       __uint(max_entries, MAX_NR_CPUS);
> +       __uint(key_size, sizeof(u32));
> +       __array(values, struct inner_lru); /* use inner_lru as inner map */
> +} array_of_lru_hashs SEC(".maps");
> +

[...]

> -static void fixup_map(struct bpf_map_data *map, int idx)
> +static void fixup_map(struct bpf_object *obj)
>  {
> +       struct bpf_map *map;
>         int i;
>
> -       if (!strcmp("inner_lru_hash_map", map->name)) {
> -               inner_lru_hash_idx = idx;
> -               inner_lru_hash_size = map->def.max_entries;
> -       }
> +       bpf_object__for_each_map(map, obj) {
> +               const char *name = bpf_map__name(map);
>
> -       if (!strcmp("array_of_lru_hashs", map->name)) {

I'm a bit too lazy right now to figure out exact logic here, but just
wanted to mention that it is possible to statically set inner map
elements for array_of_maps and hash_of_maps. Please check
tools/testing/selftests/bpf/progs/test_btf_map_in_map.c and see if you
can use this feature to simplify this logic a bit.

> -               if (inner_lru_hash_idx == -1) {
> -                       printf("inner_lru_hash_map must be defined before array_of_lru_hashs\n");
> -                       exit(1);
> +               /* Only change the max_entries for the enabled test(s) */
> +               for (i = 0; i < NR_TESTS; i++) {
> +                       if (!strcmp(test_map_names[i], name) &&
> +                           (check_test_flags(i))) {
> +                               bpf_map__resize(map, num_map_entries);
> +                               continue;
> +                       }
>                 }
> -               map->def.inner_map_idx = inner_lru_hash_idx;
> -               array_of_lru_hashs_idx = idx;
>         }
>

[...]