Re: [Patch bpf-next 0/3] bpf: introduce timeout map

On Sun, Dec 13, 2020 at 5:33 PM Andrey Ignatov [off-list ref] wrote:

Cong Wang [off-list ref] [Sat, 2020-12-12 15:18 -0800]:

quoted

On Sat, Dec 12, 2020 at 2:25 PM Cong Wang [off-list ref] wrote:

quoted

On Fri, Dec 11, 2020 at 11:55 AM Andrii Nakryiko
[off-list ref] wrote:

quoted

On Fri, Dec 11, 2020 at 2:28 AM Cong Wang [off-list ref] wrote:

quoted

From: Cong Wang <redacted>

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.