On Wed, Oct 20, 2021 at 2:09 PM Joanne Koong <joannekoong@xxxxxx> wrote:
>
> On 10/8/21 4:19 PM, Andrii Nakryiko wrote:
>
> > On Wed, Oct 6, 2021 at 3:27 PM Joanne Koong <joannekoong@xxxxxx> wrote:
> >> This patch adds the libbpf infrastructure for supporting a
> >> per-map-type "map_extra" field, whose definition will be
> >> idiosyncratic depending on map type.
> >>
> >> For example, for the bitset map, the lower 4 bits of map_extra
> >> is used to denote the number of hash functions.
> >>
> >> Signed-off-by: Joanne Koong <joannekoong@xxxxxx>
> >> ---
> >> include/uapi/linux/bpf.h | 1 +
> >> tools/include/uapi/linux/bpf.h | 1 +
> >> tools/lib/bpf/bpf.c | 1 +
> >> tools/lib/bpf/bpf.h | 1 +
> >> tools/lib/bpf/bpf_helpers.h | 1 +
> >> tools/lib/bpf/libbpf.c | 25 ++++++++++++++++++++++++-
> >> tools/lib/bpf/libbpf.h | 4 ++++
> >> tools/lib/bpf/libbpf.map | 2 ++
> >> tools/lib/bpf/libbpf_internal.h | 4 +++-
> >> 9 files changed, 38 insertions(+), 2 deletions(-)
> >>
> >> [...]
> >>
> >> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> >> index 7d1741ceaa32..41e3e85e7789 100644
> >> --- a/tools/lib/bpf/bpf.c
> >> +++ b/tools/lib/bpf/bpf.c
> >> @@ -97,6 +97,7 @@ int bpf_create_map_xattr(const struct bpf_create_map_attr *create_attr)
> >> attr.btf_key_type_id = create_attr->btf_key_type_id;
> >> attr.btf_value_type_id = create_attr->btf_value_type_id;
> >> attr.map_ifindex = create_attr->map_ifindex;
> >> + attr.map_extra = create_attr->map_extra;
> >> if (attr.map_type == BPF_MAP_TYPE_STRUCT_OPS)
> >> attr.btf_vmlinux_value_type_id =
> >> create_attr->btf_vmlinux_value_type_id;
> >> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> >> index 6fffb3cdf39b..c4049f2d63cc 100644
> >> --- a/tools/lib/bpf/bpf.h
> >> +++ b/tools/lib/bpf/bpf.h
> >> @@ -50,6 +50,7 @@ struct bpf_create_map_attr {
> >> __u32 inner_map_fd;
> >> __u32 btf_vmlinux_value_type_id;
> >> };
> >> + __u32 map_extra;
> > this struct is frozen, we can't change it. It's fine to not allow
> > passing map_extra in libbpf APIs. We have libbpf 1.0 task to revamp
> > low-level APIs like map creation in a way that will allow good
> > extensibility. You don't have to worry about that in this patch set.
> I see! From my understanding, without "map_extra" added to the
> bpf_create_map_attr struct, it's not possible in the subsequent
> bloom filter benchmark tests to set the map_extra flag, which
Didn't you add bpf_map__set_map_extra() setter for that? Also one can
always do direct bpf syscall (see sys_bpf in tools/lib/bpf/bpf.c), if
absolutely necessary. But set_map_extra() setter is the way to go for
benchmark, I think.
> means we can't set the number of hash functions. (The entrypoint
> for propagating the flags to the kernel at map creation time is
> in the function "bpf_create_map_xattr", which takes in a
> struct bpf_create_map_attr).
>
> 1) To get the benchmark numbers for different # of hash functions, I'll
> test using a modified version of the code where the map_extra flags
> gets propagated to the kernel. I'll add a TODO to the benchmarks
> saying that the specified # of hash functions will get propagated for real
> once libbpf's map creation supports map_extra.
>
>
> 2) Should I drop this libbpf patch altogether from this patchset, and add
> it when we do the libbpf 1.0 task to revamp the map creation APIs? Since
> without extending map creation to include the map_extra, these map_extra
> libbpf changes don't have much effect right now
No, getter/setter API is good to have, please keep them.
>
> What are your thoughts?
>
> > [...]
> >> --
> >> 2.30.2
> >>
