On Tue, Nov 3, 2020 at 7:49 AM David Verbeiren
<david.verbeiren@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> Signed-off-by: David Verbeiren <david.verbeiren@xxxxxxxxxxxx>
> ---
>
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
>
