On Tue, Feb 13, 2024 at 3:15 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Thu, Feb 8, 2024 at 8:07 PM Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > From: Alexei Starovoitov <ast@xxxxxxxxxx>
> >
> > __uint() macro that is used to specify map attributes like:
> > __uint(type, BPF_MAP_TYPE_ARRAY);
> > __uint(map_flags, BPF_F_MMAPABLE);
> > is limited to 32-bit, since BTF_KIND_ARRAY has u32 "number of elements" field.
> >
> > Introduce __ulong() macro that allows specifying values bigger than 32-bit.
> > In map definition "map_extra" is the only u64 field.
> >
> > Signed-off-by: Alexei Starovoitov <ast@xxxxxxxxxx>
> > ---
> > tools/lib/bpf/bpf_helpers.h | 5 +++++
> > tools/lib/bpf/libbpf.c | 44 ++++++++++++++++++++++++++++++++++---
> > 2 files changed, 46 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> > index 9c777c21da28..0aeac8ea7af2 100644
> > --- a/tools/lib/bpf/bpf_helpers.h
> > +++ b/tools/lib/bpf/bpf_helpers.h
> > @@ -13,6 +13,11 @@
> > #define __uint(name, val) int (*name)[val]
> > #define __type(name, val) typeof(val) *name
> > #define __array(name, val) typeof(val) *name[]
> > +#ifndef __PASTE
> > +#define ___PASTE(a,b) a##b
> > +#define __PASTE(a,b) ___PASTE(a,b)
> > +#endif
>
> we already have ___bpf_concat defined further in this file (it's macro
> so ordering shouldn't matter), let's just use that instead of adding
> another variant
Ohh. forgot about this one. will do.
> > +#define __ulong(name, val) enum { __PASTE(__unique_value, __COUNTER__) = val } name
> >
> > /*
> > * Helper macro to place programs, maps, license in
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 4880d623098d..f8158e250327 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -2243,6 +2243,39 @@ static bool get_map_field_int(const char *map_name, const struct btf *btf,
> > return true;
> > }
> >
> > +static bool get_map_field_long(const char *map_name, const struct btf *btf,
> > + const struct btf_member *m, __u64 *res)
> > +{
> > + const struct btf_type *t = skip_mods_and_typedefs(btf, m->type, NULL);
> > + const char *name = btf__name_by_offset(btf, m->name_off);
> > +
> > + if (btf_is_ptr(t))
> > + return false;
>
> It's not great that anyone that uses __uint(map_extra, ...) would get
> warnings now.
What warning ?
This specific check makes it fallback to ptr without warning.
We have a bloom filter test that uses map_extra.
No warnings there.
> Let's just teach get_map_field_long to recognize __uint vs __ulong?
>
> Let's call into get_map_field_int() here if we have a pointer, and
> then upcast u32 into u64?
makes sense.
> > +
> > + if (!btf_is_enum(t) && !btf_is_enum64(t)) {
> > + pr_warn("map '%s': attr '%s': expected enum or enum64, got %s.\n",
>
> seems like get_map_field_int() is using "PTR" and "ARRAY" all caps
> spelling in warnings, let's use ENUM and ENUM64 for consistency?
done.
> > + map_name, name, btf_kind_str(t));
> > + return false;
> > + }
> > +
> > + if (btf_vlen(t) != 1) {
> > + pr_warn("map '%s': attr '%s': invalid __ulong\n",
> > + map_name, name);
> > + return false;
> > + }
> > +
> > + if (btf_is_enum(t)) {
> > + const struct btf_enum *e = btf_enum(t);
> > +
> > + *res = e->val;
> > + } else {
> > + const struct btf_enum64 *e = btf_enum64(t);
> > +
> > + *res = btf_enum64_value(e);
> > + }
> > + return true;
> > +}
> > +
> > static int pathname_concat(char *buf, size_t buf_sz, const char *path, const char *name)
> > {
> > int len;
> > @@ -2476,10 +2509,15 @@ int parse_btf_map_def(const char *map_name, struct btf *btf,
> > map_def->pinning = val;
> > map_def->parts |= MAP_DEF_PINNING;
> > } else if (strcmp(name, "map_extra") == 0) {
> > - __u32 map_extra;
> > + __u64 map_extra;
> >
> > - if (!get_map_field_int(map_name, btf, m, &map_extra))
> > - return -EINVAL;
> > + if (!get_map_field_long(map_name, btf, m, &map_extra)) {
> > + __u32 map_extra_u32;
> > +
> > + if (!get_map_field_int(map_name, btf, m, &map_extra_u32))
> > + return -EINVAL;
> > + map_extra = map_extra_u32;
> > + }
>
> with the above change it would be a simple
> s/get_map_field_int/get_map_field_long/ (and __u32 -> __u64, of
> course)
so this logic will move into get_map_field_long.
makes sense.
I thought about making get_map_field_int() to handle enum,
but way too many places need refactoring, since it's called like:
get_map_field_int(map_name, btf, m, &map_def->map_type)
get_map_field_int(map_name, btf, m, &map_def->max_entries)
