| Message ID | 20201211000649.236635-1-xiyou.wangcong@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | bpf: introduce timeout map | expand |
On Fri, Dec 11, 2020 at 2:28 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > From: Cong Wang <cong.wang@bytedance.com> > > 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]). Also patch #3 should have a commit message, even if pretty trivial one. [0] https://travis-ci.com/github/kernel-patches/bpf/builds/207928289 > Cong Wang (3): > bpf: use index instead of hash for map_locked[] > bpf: introduce timeout map > tools: add a test case for bpf timeout map > > include/linux/bpf_types.h | 1 + > include/uapi/linux/bpf.h | 3 +- > kernel/bpf/hashtab.c | 296 +++++++++++++++++++++--- > kernel/bpf/syscall.c | 3 +- > tools/include/uapi/linux/bpf.h | 1 + > tools/testing/selftests/bpf/test_maps.c | 41 ++++ > 6 files changed, 314 insertions(+), 31 deletions(-) > > -- > 2.25.1 >
On Fri, Dec 11, 2020 at 11:55 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Fri, Dec 11, 2020 at 2:28 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > > From: Cong Wang <cong.wang@bytedance.com> > > > > 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. > Also patch #3 should have a commit message, even if pretty trivial one. Yeah, I thought its subject is sufficient for a trivial patch. Thanks.
On Sat, Dec 12, 2020 at 2:25 PM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > On Fri, Dec 11, 2020 at 11:55 AM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Fri, Dec 11, 2020 at 2:28 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > > > > From: Cong Wang <cong.wang@bytedance.com> > > > > > > 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. Thanks.
Cong Wang <xiyou.wangcong@gmail.com> [Sat, 2020-12-12 15:18 -0800]: > On Sat, Dec 12, 2020 at 2:25 PM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > > On Fri, Dec 11, 2020 at 11:55 AM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > > > > > > On Fri, Dec 11, 2020 at 2:28 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > > > > > > From: Cong Wang <cong.wang@bytedance.com> > > > > > > > > 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.
On Sun, Dec 13, 2020 at 5:33 PM Andrey Ignatov <rdna@fb.com> wrote: > > Cong Wang <xiyou.wangcong@gmail.com> [Sat, 2020-12-12 15:18 -0800]: > > On Sat, Dec 12, 2020 at 2:25 PM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > > > > On Fri, Dec 11, 2020 at 11:55 AM Andrii Nakryiko > > > <andrii.nakryiko@gmail.com> wrote: > > > > > > > > On Fri, Dec 11, 2020 at 2:28 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > > > > > > > > From: Cong Wang <cong.wang@bytedance.com> > > > > > > > > > > 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.
From: Cong Wang <cong.wang@bytedance.com> 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. --- Cong Wang (3): bpf: use index instead of hash for map_locked[] bpf: introduce timeout map tools: add a test case for bpf timeout map include/linux/bpf_types.h | 1 + include/uapi/linux/bpf.h | 3 +- kernel/bpf/hashtab.c | 296 +++++++++++++++++++++--- kernel/bpf/syscall.c | 3 +- tools/include/uapi/linux/bpf.h | 1 + tools/testing/selftests/bpf/test_maps.c | 41 ++++ 6 files changed, 314 insertions(+), 31 deletions(-)