On Sun, Dec 13, 2020 at 5:33 PM Andrey Ignatov <rdna@xxxxxx> wrote: > > 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. I see, I was not aware of this. > > 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. Good to know it is useful, I will have to change 64 to 72, as I tried to use sizeof but struct htab_elem is not visible to that test. > > 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. Yeah, will do. Thanks.
