Cong Wang <xiyou.wangcong@xxxxxxxxx> [Sat, 2020-12-12 15:18 -0800]: > On Sat, Dec 12, 2020 at 2:25 PM Cong Wang <xiyou.wangcong@xxxxxxxxx> wrote: > > > > On Fri, Dec 11, 2020 at 11:55 AM Andrii Nakryiko > > <andrii.nakryiko@xxxxxxxxx> wrote: > > > > > > On Fri, Dec 11, 2020 at 2:28 AM Cong Wang <xiyou.wangcong@xxxxxxxxx> wrote: > > > > > > > > From: Cong Wang <cong.wang@xxxxxxxxxxxxx> > > > > > > > > This patchset introduces a new bpf hash map which has timeout. > > > > Patch 1 is a preparation, patch 2 is the implementation of timeout > > > > map, patch 3 contains a test case for timeout map. Please check each > > > > patch description for more details. > > > > > > > > --- > > > > > > This patch set seems to be breaking existing selftests. Please take a > > > look ([0]). > > > > Interesting, looks unrelated to my patches but let me double check. > > Cc'ing Andrey... > > Looks like the failure is due to the addition of a new member to struct > htab_elem. Any reason why it is hard-coded as 64 in check_hash()? > And what's the point of verifying its size? htab_elem should be only > visible to the kernel itself. > > I can certainly change 64 to whatever its new size is, but I do wonder > why the test is there. Cong, the test is there to make sure that access to map pointers from BPF program works. Please see (41c48f3a9823 "bpf: Support access to bpf map fields") for more details on what "access to map pointer" means, but it's basically a way to access any field (e.g. max_entries) of common `struct bpf_map` or any type-specific struct like `struct bpf_htab` from BPF program, i.e. these structs are visible to not only kernel but also to BPF programs. The point of the test is to access a few fields from every map struct and make sure it works. Changing `struct htab_elem` indeed breaks the `VERIFY(hash->elem_size == 64);` check. But it can be easily updated (from 64 to whatever new size is) or replaced by some other field check. `htab->elem_size` was chosen semi-randomly since any bpf_htab-specific field would work for the test's purposes. Hope it clarifies. Also since you add a new map type it would be great to cover it in tools/testing/selftests/bpf/progs/map_ptr_kern.c as well. -- Andrey Ignatov
