diff mbox series

[v2,bpf-next,13/20] libbpf: Allow specifying 64-bit integers in map BTF.

Message ID 20240209040608.98927-14-alexei.starovoitov@gmail.com (mailing list archive)
State New
Headers show
Series bpf: Introduce BPF arena. | expand

Commit Message

Alexei Starovoitov Feb. 9, 2024, 4:06 a.m. UTC 
From: Alexei Starovoitov <ast@kernel.org>

__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@kernel.org>
---
 tools/lib/bpf/bpf_helpers.h |  5 +++++
 tools/lib/bpf/libbpf.c      | 44 ++++++++++++++++++++++++++++++++++---
 2 files changed, 46 insertions(+), 3 deletions(-)

Comments

Eduard Zingerman Feb. 12, 2024, 6:58 p.m. UTC | #1 
On Thu, 2024-02-08 at 20:06 -0800, Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@kernel.org>
> 
> __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@kernel.org>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

Another option would be something like:

    #define __ulong(name, val) int (*name)[val >> 32][(val << 32) >> 32]

thus avoiding generation of __unique_value_123 literals,
but these literals probably should not be an issue.
Andrii Nakryiko Feb. 13, 2024, 11:15 p.m. UTC | #2 
On Thu, Feb 8, 2024 at 8:07 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> From: Alexei Starovoitov <ast@kernel.org>
>
> __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@kernel.org>
> ---
>  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

> +#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.
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?

> +
> +       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?

> +                       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)


>                         map_def->map_extra = map_extra;
>                         map_def->parts |= MAP_DEF_MAP_EXTRA;
>                 } else {
> --
> 2.34.1
>
Alexei Starovoitov Feb. 14, 2024, 12:47 a.m. UTC | #3 
On Tue, Feb 13, 2024 at 3:15 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, Feb 8, 2024 at 8:07 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > From: Alexei Starovoitov <ast@kernel.org>
> >
> > __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@kernel.org>
> > ---
> >  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)
Andrii Nakryiko Feb. 14, 2024, 12:51 a.m. UTC | #4 
On Tue, Feb 13, 2024 at 4:47 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Feb 13, 2024 at 3:15 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Thu, Feb 8, 2024 at 8:07 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > From: Alexei Starovoitov <ast@kernel.org>
> > >
> > > __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@kernel.org>
> > > ---
> > >  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.

ah, right, forget about the warning, you exit early. But still makes
sense to handle ulong vs uint transparently


>
> > 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.

yep, seems good to not care about int vs long here

>
> 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)

yeah, not worth it
diff mbox series

Patch

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
+#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;
+
+	if (!btf_is_enum(t) && !btf_is_enum64(t)) {
+		pr_warn("map '%s': attr '%s': expected enum or enum64, got %s.\n",
+			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;
+			}
 			map_def->map_extra = map_extra;
 			map_def->parts |= MAP_DEF_MAP_EXTRA;
 		} else {