Re: [PATCH bpf v3] bpf: zero-fill re-used per-cpu map element

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Tue, Nov 3, 2020 at 7:49 AM David Verbeiren
<david.verbeiren@tessares.net> wrote:
>
> Zero-fill element values for all other cpus than current, just as
> when not using prealloc. This is the only way the bpf program can
> ensure known initial values for all cpus ('onallcpus' cannot be
> set when coming from the bpf program).
>
> The scenario is: bpf program inserts some elements in a per-cpu
> map, then deletes some (or userspace does). When later adding
> new elements using bpf_map_update_elem(), the bpf program can
> only set the value of the new elements for the current cpu.
> When prealloc is enabled, previously deleted elements are re-used.
> Without the fix, values for other cpus remain whatever they were
> when the re-used entry was previously freed.
>
> A selftest is added to validate correct operation in above
> scenario as well as in case of LRU per-cpu map element re-use.
>
> Fixes: 6c9059817432 ("bpf: pre-allocate hash map elements")
> Acked-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> Signed-off-by: David Verbeiren <david.verbeiren@tessares.net>
> ---
>

Tests look really nice, thanks! I'm worried about still racy once
check, see suggestions below. Otherwise looks great!

> Notes:
>     v3:
>       - Added selftest that was initially provided as separate
>         patch, and reworked to
>         * use skeleton (Andrii, Song Liu)
>         * skip test if <=1 CPU (Song Liu)
>
>     v2:
>       - Moved memset() to separate pcpu_init_value() function,
>         which replaces pcpu_copy_value() but delegates to it
>         for the cases where no memset() is needed (Andrii).
>       - This function now also avoids doing the memset() for
>         the current cpu for which the value must be set
>         anyhow (Andrii).
>       - Same pcpu_init_value() used for per-cpu LRU map
>         (Andrii).
>
>  kernel/bpf/hashtab.c                          |  30 ++-
>  .../selftests/bpf/prog_tests/map_init.c       | 213 ++++++++++++++++++
>  .../selftests/bpf/progs/test_map_init.c       |  34 +++
>  3 files changed, 275 insertions(+), 2 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/map_init.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_map_init.c
>

[...]

> diff --git a/tools/testing/selftests/bpf/prog_tests/map_init.c b/tools/testing/selftests/bpf/prog_tests/map_init.c
> new file mode 100644
> index 000000000000..386d9439bad9
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/map_init.c
> @@ -0,0 +1,213 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +// Copyright (c) 2020 Tessares SA <http://www.tessares.net>
> +

nit: see below, /* */

> +#include <test_progs.h>
> +#include "test_map_init.skel.h"
> +

[...]

> diff --git a/tools/testing/selftests/bpf/progs/test_map_init.c b/tools/testing/selftests/bpf/progs/test_map_init.c
> new file mode 100644
> index 000000000000..280a45e366d6
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_map_init.c
> @@ -0,0 +1,34 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2020 Tessares SA <http://www.tessares.net>

nit: I think copyright line has to be in /* */ comment block

> +
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +
> +__u64 inKey = 0;
> +__u64 inValue = 0;
> +__u32 once = 0;
> +
> +struct {
> +       __uint(type, BPF_MAP_TYPE_PERCPU_HASH);
> +       __uint(max_entries, 2);
> +       __type(key, __u64);
> +       __type(value, __u64);
> +} hashmap1 SEC(".maps");
> +
> +
> +SEC("raw_tp/sys_enter")
> +int sys_enter(const void *ctx)
> +{
> +       /* Just do it once so the value is only updated for a single CPU.
> +        * Indeed, this tracepoint will quickly be hit from different CPUs.
> +        */
> +       if (!once) {
> +               __sync_fetch_and_add(&once, 1);

This is quite racy, actually, especially for the generic sys_enter
tracepoint. The way I did this before (see progs/trigger_bench.c) was
through doing a "tp/syscalls/sys_enter_getpgid" tracepoint program and
checking for thread id. Or you can use bpf_test_run, probably, with a
different type of BPF program. I just find bpf_test_run() too
inconvenient with all the extra setup, so I usually stick to
tracepoints.

> +
> +               bpf_map_update_elem(&hashmap1, &inKey, &inValue, BPF_NOEXIST);
> +       }
> +
> +       return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> --
> 2.29.0
>