| Message ID | 20190626232133.3800637-2-andriin@fb.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | capture integers in BTF type info for map defs | expand |
> On Jun 26, 2019, at 4:21 PM, Andrii Nakryiko <andriin@fb.com> wrote: > > Change BTF-defined map definitions to capture compile-time integer > values as part of BTF type definition, to avoid split of key/value type > information and actual type/size/flags initialization for maps. If I have an old bpf program and compiled it with new llvm, will it work with new libbpf? > > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > --- > tools/lib/bpf/libbpf.c | 58 +++++++++++------------ > tools/testing/selftests/bpf/bpf_helpers.h | 3 ++ > 2 files changed, 31 insertions(+), 30 deletions(-) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 68f45a96769f..f2b02032a8e6 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -1028,40 +1028,40 @@ static const struct btf_type *skip_mods_and_typedefs(const struct btf *btf, > } > } > > -static bool get_map_field_int(const char *map_name, > - const struct btf *btf, > +/* > + * Fetch integer attribute of BTF map definition. Such attributes are > + * represented using a pointer to an array, in which dimensionality of array > + * encodes specified integer value. E.g., int (*type)[BPF_MAP_TYPE_ARRAY]; > + * encodes `type => BPF_MAP_TYPE_ARRAY` key/value pair completely using BTF > + * type definition, while using only sizeof(void *) space in ELF data section. > + */ > +static bool get_map_field_int(const char *map_name, const struct btf *btf, > const struct btf_type *def, > - const struct btf_member *m, > - const void *data, __u32 *res) { > + const struct btf_member *m, __u32 *res) { > const struct btf_type *t = skip_mods_and_typedefs(btf, m->type); > const char *name = btf__name_by_offset(btf, m->name_off); > - __u32 int_info = *(const __u32 *)(const void *)(t + 1); > + const struct btf_array *arr_info; > + const struct btf_type *arr_t; > > - if (BTF_INFO_KIND(t->info) != BTF_KIND_INT) { > - pr_warning("map '%s': attr '%s': expected INT, got %u.\n", > + if (BTF_INFO_KIND(t->info) != BTF_KIND_PTR) { > + pr_warning("map '%s': attr '%s': expected PTR, got %u.\n", > map_name, name, BTF_INFO_KIND(t->info)); > return false; > } > - if (t->size != 4 || BTF_INT_BITS(int_info) != 32 || > - BTF_INT_OFFSET(int_info)) { > - pr_warning("map '%s': attr '%s': expected 32-bit non-bitfield integer, " > - "got %u-byte (%d-bit) one with bit offset %d.\n", > - map_name, name, t->size, BTF_INT_BITS(int_info), > - BTF_INT_OFFSET(int_info)); > - return false; > - } > - if (BTF_INFO_KFLAG(def->info) && BTF_MEMBER_BITFIELD_SIZE(m->offset)) { > - pr_warning("map '%s': attr '%s': bitfield is not supported.\n", > - map_name, name); > + > + arr_t = btf__type_by_id(btf, t->type); > + if (!arr_t) { > + pr_warning("map '%s': attr '%s': type [%u] not found.\n", > + map_name, name, t->type); > return false; > } > - if (m->offset % 32) { > - pr_warning("map '%s': attr '%s': unaligned fields are not supported.\n", > - map_name, name); > + if (BTF_INFO_KIND(arr_t->info) != BTF_KIND_ARRAY) { > + pr_warning("map '%s': attr '%s': expected ARRAY, got %u.\n", > + map_name, name, BTF_INFO_KIND(arr_t->info)); > return false; > } > - > - *res = *(const __u32 *)(data + m->offset / 8); > + arr_info = (const void *)(arr_t + 1); > + *res = arr_info->nelems; > return true; > } > > @@ -1074,7 +1074,6 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj, > const struct btf_var_secinfo *vi; > const struct btf_var *var_extra; > const struct btf_member *m; > - const void *def_data; > const char *map_name; > struct bpf_map *map; > int vlen, i; > @@ -1131,7 +1130,6 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj, > pr_debug("map '%s': at sec_idx %d, offset %zu.\n", > map_name, map->sec_idx, map->sec_offset); > > - def_data = data->d_buf + vi->offset; > vlen = BTF_INFO_VLEN(def->info); > m = (const void *)(def + 1); > for (i = 0; i < vlen; i++, m++) { > @@ -1144,19 +1142,19 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj, > } > if (strcmp(name, "type") == 0) { > if (!get_map_field_int(map_name, obj->btf, def, m, > - def_data, &map->def.type)) > + &map->def.type)) > return -EINVAL; > pr_debug("map '%s': found type = %u.\n", > map_name, map->def.type); > } else if (strcmp(name, "max_entries") == 0) { > if (!get_map_field_int(map_name, obj->btf, def, m, > - def_data, &map->def.max_entries)) > + &map->def.max_entries)) > return -EINVAL; > pr_debug("map '%s': found max_entries = %u.\n", > map_name, map->def.max_entries); > } else if (strcmp(name, "map_flags") == 0) { > if (!get_map_field_int(map_name, obj->btf, def, m, > - def_data, &map->def.map_flags)) > + &map->def.map_flags)) > return -EINVAL; > pr_debug("map '%s': found map_flags = %u.\n", > map_name, map->def.map_flags); > @@ -1164,7 +1162,7 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj, > __u32 sz; > > if (!get_map_field_int(map_name, obj->btf, def, m, > - def_data, &sz)) > + &sz)) > return -EINVAL; > pr_debug("map '%s': found key_size = %u.\n", > map_name, sz); > @@ -1207,7 +1205,7 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj, > __u32 sz; > > if (!get_map_field_int(map_name, obj->btf, def, m, > - def_data, &sz)) > + &sz)) > return -EINVAL; > pr_debug("map '%s': found value_size = %u.\n", > map_name, sz); > diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h > index 1a5b1accf091..aa5ddf58c088 100644 > --- a/tools/testing/selftests/bpf/bpf_helpers.h > +++ b/tools/testing/selftests/bpf/bpf_helpers.h > @@ -8,6 +8,9 @@ > */ > #define SEC(NAME) __attribute__((section(NAME), used)) > > +#define __int(name, val) int (*name)[val] > +#define __type(name, val) val *name > + I think we need these two in libbpf. Thanks, Song > /* helper macro to print out debug messages */ > #define bpf_printk(fmt, ...) \ > ({ \ > -- > 2.17.1 >
On Thu, Jun 27, 2019 at 10:27 AM Song Liu <songliubraving@fb.com> wrote: > > > > > On Jun 26, 2019, at 4:21 PM, Andrii Nakryiko <andriin@fb.com> wrote: > > > > Change BTF-defined map definitions to capture compile-time integer > > values as part of BTF type definition, to avoid split of key/value type > > information and actual type/size/flags initialization for maps. > > If I have an old bpf program and compiled it with new llvm, will it > work with new libbpf? You mean BPF programs that used previous incarnation of BTF-defined maps? No, they won't work. But we never released them, so I think it's ok to change them. Nothing should be using that except for selftests, which I fixed. > > > > > > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > > --- <snip> > > diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h > > index 1a5b1accf091..aa5ddf58c088 100644 > > --- a/tools/testing/selftests/bpf/bpf_helpers.h > > +++ b/tools/testing/selftests/bpf/bpf_helpers.h > > @@ -8,6 +8,9 @@ > > */ > > #define SEC(NAME) __attribute__((section(NAME), used)) > > > > +#define __int(name, val) int (*name)[val] > > +#define __type(name, val) val *name > > + > > I think we need these two in libbpf. Yes, but it's another story for another set of patches. We'll need to provide bpf_helpers as part of libbpf for inclusion into BPF programs, but there are a bunch of problems right now with existing bpf_heplers.h that prevents us from just copying it into libbpf. We'll need to resolve those first. But then again, there is no use of __int and __type for user-space programs, so for now it's ok. > > Thanks, > Song > > > /* helper macro to print out debug messages */ > > #define bpf_printk(fmt, ...) \ > > ({ \ > > -- > > 2.17.1 > > >
> On Jun 27, 2019, at 10:47 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Thu, Jun 27, 2019 at 10:27 AM Song Liu <songliubraving@fb.com> wrote: >> >> >> >>> On Jun 26, 2019, at 4:21 PM, Andrii Nakryiko <andriin@fb.com> wrote: >>> >>> Change BTF-defined map definitions to capture compile-time integer >>> values as part of BTF type definition, to avoid split of key/value type >>> information and actual type/size/flags initialization for maps. >> >> If I have an old bpf program and compiled it with new llvm, will it >> work with new libbpf? > > You mean BPF programs that used previous incarnation of BTF-defined > maps? No, they won't work. But we never released them, so I think it's > ok to change them. Nothing should be using that except for selftests, > which I fixed. I see. This makes sense. > >> >> >>> >>> Signed-off-by: Andrii Nakryiko <andriin@fb.com> >>> --- > > <snip> > >>> diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h >>> index 1a5b1accf091..aa5ddf58c088 100644 >>> --- a/tools/testing/selftests/bpf/bpf_helpers.h >>> +++ b/tools/testing/selftests/bpf/bpf_helpers.h >>> @@ -8,6 +8,9 @@ >>> */ >>> #define SEC(NAME) __attribute__((section(NAME), used)) >>> >>> +#define __int(name, val) int (*name)[val] >>> +#define __type(name, val) val *name >>> + >> >> I think we need these two in libbpf. > > Yes, but it's another story for another set of patches. We'll need to > provide bpf_helpers as part of libbpf for inclusion into BPF programs, > but there are a bunch of problems right now with existing > bpf_heplers.h that prevents us from just copying it into libbpf. We'll > need to resolve those first. > > But then again, there is no use of __int and __type for user-space > programs, so for now it's ok. OK. How about we put these two lines in an separate patch? Thanks, Song
On Thu, Jun 27, 2019 at 10:56 AM Song Liu <songliubraving@fb.com> wrote: > > > > > On Jun 27, 2019, at 10:47 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > > On Thu, Jun 27, 2019 at 10:27 AM Song Liu <songliubraving@fb.com> wrote: > >> > >> > >> > >>> On Jun 26, 2019, at 4:21 PM, Andrii Nakryiko <andriin@fb.com> wrote: > >>> > >>> Change BTF-defined map definitions to capture compile-time integer > >>> values as part of BTF type definition, to avoid split of key/value type > >>> information and actual type/size/flags initialization for maps. > >> > >> If I have an old bpf program and compiled it with new llvm, will it > >> work with new libbpf? > > > > You mean BPF programs that used previous incarnation of BTF-defined > > maps? No, they won't work. But we never released them, so I think it's > > ok to change them. Nothing should be using that except for selftests, > > which I fixed. > > I see. This makes sense. > > > > >> > >> > >>> > >>> Signed-off-by: Andrii Nakryiko <andriin@fb.com> > >>> --- > > > > <snip> > > > >>> diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h > >>> index 1a5b1accf091..aa5ddf58c088 100644 > >>> --- a/tools/testing/selftests/bpf/bpf_helpers.h > >>> +++ b/tools/testing/selftests/bpf/bpf_helpers.h > >>> @@ -8,6 +8,9 @@ > >>> */ > >>> #define SEC(NAME) __attribute__((section(NAME), used)) > >>> > >>> +#define __int(name, val) int (*name)[val] > >>> +#define __type(name, val) val *name > >>> + > >> > >> I think we need these two in libbpf. > > > > Yes, but it's another story for another set of patches. We'll need to > > provide bpf_helpers as part of libbpf for inclusion into BPF programs, > > but there are a bunch of problems right now with existing > > bpf_heplers.h that prevents us from just copying it into libbpf. We'll > > need to resolve those first. > > > > But then again, there is no use of __int and __type for user-space > > programs, so for now it's ok. > > OK. How about we put these two lines in an separate patch? Sure, no problem. > > Thanks, > Song >
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 68f45a96769f..f2b02032a8e6 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -1028,40 +1028,40 @@ static const struct btf_type *skip_mods_and_typedefs(const struct btf *btf, } } -static bool get_map_field_int(const char *map_name, - const struct btf *btf, +/* + * Fetch integer attribute of BTF map definition. Such attributes are + * represented using a pointer to an array, in which dimensionality of array + * encodes specified integer value. E.g., int (*type)[BPF_MAP_TYPE_ARRAY]; + * encodes `type => BPF_MAP_TYPE_ARRAY` key/value pair completely using BTF + * type definition, while using only sizeof(void *) space in ELF data section. + */ +static bool get_map_field_int(const char *map_name, const struct btf *btf, const struct btf_type *def, - const struct btf_member *m, - const void *data, __u32 *res) { + const struct btf_member *m, __u32 *res) { const struct btf_type *t = skip_mods_and_typedefs(btf, m->type); const char *name = btf__name_by_offset(btf, m->name_off); - __u32 int_info = *(const __u32 *)(const void *)(t + 1); + const struct btf_array *arr_info; + const struct btf_type *arr_t; - if (BTF_INFO_KIND(t->info) != BTF_KIND_INT) { - pr_warning("map '%s': attr '%s': expected INT, got %u.\n", + if (BTF_INFO_KIND(t->info) != BTF_KIND_PTR) { + pr_warning("map '%s': attr '%s': expected PTR, got %u.\n", map_name, name, BTF_INFO_KIND(t->info)); return false; } - if (t->size != 4 || BTF_INT_BITS(int_info) != 32 || - BTF_INT_OFFSET(int_info)) { - pr_warning("map '%s': attr '%s': expected 32-bit non-bitfield integer, " - "got %u-byte (%d-bit) one with bit offset %d.\n", - map_name, name, t->size, BTF_INT_BITS(int_info), - BTF_INT_OFFSET(int_info)); - return false; - } - if (BTF_INFO_KFLAG(def->info) && BTF_MEMBER_BITFIELD_SIZE(m->offset)) { - pr_warning("map '%s': attr '%s': bitfield is not supported.\n", - map_name, name); + + arr_t = btf__type_by_id(btf, t->type); + if (!arr_t) { + pr_warning("map '%s': attr '%s': type [%u] not found.\n", + map_name, name, t->type); return false; } - if (m->offset % 32) { - pr_warning("map '%s': attr '%s': unaligned fields are not supported.\n", - map_name, name); + if (BTF_INFO_KIND(arr_t->info) != BTF_KIND_ARRAY) { + pr_warning("map '%s': attr '%s': expected ARRAY, got %u.\n", + map_name, name, BTF_INFO_KIND(arr_t->info)); return false; } - - *res = *(const __u32 *)(data + m->offset / 8); + arr_info = (const void *)(arr_t + 1); + *res = arr_info->nelems; return true; } @@ -1074,7 +1074,6 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj, const struct btf_var_secinfo *vi; const struct btf_var *var_extra; const struct btf_member *m; - const void *def_data; const char *map_name; struct bpf_map *map; int vlen, i; @@ -1131,7 +1130,6 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj, pr_debug("map '%s': at sec_idx %d, offset %zu.\n", map_name, map->sec_idx, map->sec_offset); - def_data = data->d_buf + vi->offset; vlen = BTF_INFO_VLEN(def->info); m = (const void *)(def + 1); for (i = 0; i < vlen; i++, m++) { @@ -1144,19 +1142,19 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj, } if (strcmp(name, "type") == 0) { if (!get_map_field_int(map_name, obj->btf, def, m, - def_data, &map->def.type)) + &map->def.type)) return -EINVAL; pr_debug("map '%s': found type = %u.\n", map_name, map->def.type); } else if (strcmp(name, "max_entries") == 0) { if (!get_map_field_int(map_name, obj->btf, def, m, - def_data, &map->def.max_entries)) + &map->def.max_entries)) return -EINVAL; pr_debug("map '%s': found max_entries = %u.\n", map_name, map->def.max_entries); } else if (strcmp(name, "map_flags") == 0) { if (!get_map_field_int(map_name, obj->btf, def, m, - def_data, &map->def.map_flags)) + &map->def.map_flags)) return -EINVAL; pr_debug("map '%s': found map_flags = %u.\n", map_name, map->def.map_flags); @@ -1164,7 +1162,7 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj, __u32 sz; if (!get_map_field_int(map_name, obj->btf, def, m, - def_data, &sz)) + &sz)) return -EINVAL; pr_debug("map '%s': found key_size = %u.\n", map_name, sz); @@ -1207,7 +1205,7 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj, __u32 sz; if (!get_map_field_int(map_name, obj->btf, def, m, - def_data, &sz)) + &sz)) return -EINVAL; pr_debug("map '%s': found value_size = %u.\n", map_name, sz); diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h index 1a5b1accf091..aa5ddf58c088 100644 --- a/tools/testing/selftests/bpf/bpf_helpers.h +++ b/tools/testing/selftests/bpf/bpf_helpers.h @@ -8,6 +8,9 @@ */ #define SEC(NAME) __attribute__((section(NAME), used)) +#define __int(name, val) int (*name)[val] +#define __type(name, val) val *name + /* helper macro to print out debug messages */ #define bpf_printk(fmt, ...) \ ({ \
Change BTF-defined map definitions to capture compile-time integer values as part of BTF type definition, to avoid split of key/value type information and actual type/size/flags initialization for maps. Signed-off-by: Andrii Nakryiko <andriin@fb.com> --- tools/lib/bpf/libbpf.c | 58 +++++++++++------------ tools/testing/selftests/bpf/bpf_helpers.h | 3 ++ 2 files changed, 31 insertions(+), 30 deletions(-)