| Message ID | 20201027221324.27894-1-david.verbeiren@tessares.net |
|---|---|
| State | Not Applicable |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf,v2] bpf: zero-fill re-used per-cpu map element | expand |
| Context | Check | Description |
|---|---|---|
| jkicinski/cover_letter | success | Link |
| jkicinski/fixes_present | success | Link |
| jkicinski/patch_count | success | Link |
| jkicinski/tree_selection | success | Clearly marked for bpf |
| jkicinski/subject_prefix | success | Link |
| jkicinski/source_inline | success | Was 0 now: 0 |
| jkicinski/verify_signedoff | success | Link |
| jkicinski/module_param | success | Was 0 now: 0 |
| jkicinski/build_32bit | fail | Errors and warnings before: 18 this patch: 18 |
| jkicinski/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/verify_fixes | success | Link |
| jkicinski/checkpatch | fail | Link |
| jkicinski/build_allmodconfig_warn | success | Errors and warnings before: 14 this patch: 14 |
| jkicinski/header_inline | success | Link |
| jkicinski/stable | success | Stable not CCed |
On Tue, Oct 27, 2020 at 3:15 PM 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. > > 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> > --- > Looks good, but would be good to have a unit test (see below). Maybe in a follow up. Acked-by: Andrii Nakryiko <andrii@kernel.org> > Notes: > 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). > > Note that I could not test the per-cpu LRU other than > by running the bpf selftests. lru_map and maps tests > passed but for the rest of the test suite, I don't > think I know how to spot problems... It would be good to write a new selftest specifically for this. You can create a single-element pre-allocated per-CPU hashmap. From user-space, initialize it to non-zeros on all CPUs. Then delete that key (it will get put on the free list). Then trigger BPF program, do an update (that should take an element from the freelist), doesn't matter which value you set (could be zero). Then from user-space get all per-CPU values for than new key. It should all be zeroes with your fix and non-zero without it. It sounds more complicated than it would look like in practice :) > > Question: Is it ok to use raw_smp_processor_id() in > these contexts? bpf prog context should be fine, I think. > Is it also ok in the syscall context? From the BPF program side it's definitely ok, because we disable CPU migration even for sleepable programs. For syscall context, it always uses onallcpus=true, so we'll never run this logic from syscall context. So I think it's fine. > > kernel/bpf/hashtab.c | 30 ++++++++++++++++++++++++++++-- > 1 file changed, 28 insertions(+), 2 deletions(-) [...]
On Tue, Oct 27, 2020 at 11:55 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > Looks good, but would be good to have a unit test (see below). Maybe > in a follow up. Here is the associated new selftest, implementing the sequence you proposed (thanks for that!), and also one for LRU case: https://lore.kernel.org/bpf/20201029111730.6881-1-david.verbeiren@tessares.net/ I hope I did it in the "right" framework of the day.
On Thu, Oct 29, 2020 at 7:44 AM David Verbeiren <david.verbeiren@tessares.net> wrote: > > On Tue, Oct 27, 2020 at 11:55 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > Looks good, but would be good to have a unit test (see below). Maybe > > in a follow up. > > Here is the associated new selftest, implementing the sequence you > proposed (thanks for that!), and also one for LRU case: > https://lore.kernel.org/bpf/20201029111730.6881-1-david.verbeiren@tessares.net/ > > I hope I did it in the "right" framework of the day. You did it in a very laborious and hard way, which will be harder to maintain, unfortunately. But it should be trivial to simplify it with skeleton. It's probably better to combine the selftest patch with the kernel fix in this patch and send it as v2?
diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c index 1815e97d4c9c..1fccba6e88c4 100644 --- a/kernel/bpf/hashtab.c +++ b/kernel/bpf/hashtab.c @@ -821,6 +821,32 @@ static void pcpu_copy_value(struct bpf_htab *htab, void __percpu *pptr, } } +static void pcpu_init_value(struct bpf_htab *htab, void __percpu *pptr, + void *value, bool onallcpus) +{ + /* When using prealloc and not setting the initial value on all cpus, + * zero-fill element values for other cpus (just as what happens when + * not using prealloc). Otherwise, bpf program has no way to ensure + * known initial values for cpus other than current one + * (onallcpus=false always when coming from bpf prog). + */ + if (htab_is_prealloc(htab) && !onallcpus) { + u32 size = round_up(htab->map.value_size, 8); + int current_cpu = raw_smp_processor_id(); + int cpu; + + for_each_possible_cpu(cpu) { + if (cpu == current_cpu) + bpf_long_memcpy(per_cpu_ptr(pptr, cpu), value, + size); + else + memset(per_cpu_ptr(pptr, cpu), 0, size); + } + } else { + pcpu_copy_value(htab, pptr, value, onallcpus); + } +} + static bool fd_htab_map_needs_adjust(const struct bpf_htab *htab) { return htab->map.map_type == BPF_MAP_TYPE_HASH_OF_MAPS && @@ -891,7 +917,7 @@ static struct htab_elem *alloc_htab_elem(struct bpf_htab *htab, void *key, } } - pcpu_copy_value(htab, pptr, value, onallcpus); + pcpu_init_value(htab, pptr, value, onallcpus); if (!prealloc) htab_elem_set_ptr(l_new, key_size, pptr); @@ -1183,7 +1209,7 @@ static int __htab_lru_percpu_map_update_elem(struct bpf_map *map, void *key, pcpu_copy_value(htab, htab_elem_get_ptr(l_old, key_size), value, onallcpus); } else { - pcpu_copy_value(htab, htab_elem_get_ptr(l_new, key_size), + pcpu_init_value(htab, htab_elem_get_ptr(l_new, key_size), value, onallcpus); hlist_nulls_add_head_rcu(&l_new->hash_node, head); l_new = NULL;
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. 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> --- Notes: 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). Note that I could not test the per-cpu LRU other than by running the bpf selftests. lru_map and maps tests passed but for the rest of the test suite, I don't think I know how to spot problems... Question: Is it ok to use raw_smp_processor_id() in these contexts? bpf prog context should be fine, I think. Is it also ok in the syscall context? kernel/bpf/hashtab.c | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-)