[bpf-next,v5,4/6] bpftool: generated shadow variables for struct_ops maps.

Commit Message

Kui-Feng Lee Feb. 27, 2024, 1:04 a.m. UTC

Declares and defines a pointer of the shadow type for each struct_ops map.

The code generator will create an anonymous struct type as the shadow type
for each struct_ops map. The shadow type is translated from the original
struct type of the map. The user of the skeleton use pointers of them to
access the values of struct_ops maps.

However, shadow types only supports certain types of fields, including
scalar types and function pointers. Any fields of unsupported types are
translated into an array of characters to occupy the space of the original
field. Function pointers are translated into pointers of the struct
bpf_program. Additionally, padding fields are generated to occupy the space
between two consecutive fields.

The pointers of shadow types of struct_osp maps are initialized when
*__open_opts() in skeletons are called. For a map called FOO, the user can
access it through the pointer at skel->struct_ops.FOO.

Reviewed-by: Quentin Monnet <[e-mail]>
Signed-off-by: Kui-Feng Lee <[e-mail]>
---
 tools/bpf/bpftool/gen.c | 235 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 234 insertions(+), 1 deletion(-)

Comments

Andrii Nakryiko Feb. 28, 2024, 6:25 p.m. UTC | #1

On Mon, Feb 26, 2024 at 5:04 PM Kui-Feng Lee <[e-mail]> wrote:
>
> Declares and defines a pointer of the shadow type for each struct_ops map.
>
> The code generator will create an anonymous struct type as the shadow type
> for each struct_ops map. The shadow type is translated from the original
> struct type of the map. The user of the skeleton use pointers of them to
> access the values of struct_ops maps.
>
> However, shadow types only supports certain types of fields, including
> scalar types and function pointers. Any fields of unsupported types are
> translated into an array of characters to occupy the space of the original
> field. Function pointers are translated into pointers of the struct
> bpf_program. Additionally, padding fields are generated to occupy the space
> between two consecutive fields.
>
> The pointers of shadow types of struct_osp maps are initialized when
> *__open_opts() in skeletons are called. For a map called FOO, the user can
> access it through the pointer at skel->struct_ops.FOO.
>
> Reviewed-by: Quentin Monnet <[e-mail]>
> Signed-off-by: Kui-Feng Lee <[e-mail]>
> ---
>  tools/bpf/bpftool/gen.c | 235 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 234 insertions(+), 1 deletion(-)
>

Looks pretty good overall, but I have a few stylistical nits, see below.

> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> index a9334c57e859..a21c92d95401 100644
> --- a/tools/bpf/bpftool/gen.c
> +++ b/tools/bpf/bpftool/gen.c
> @@ -909,6 +909,207 @@ codegen_progs_skeleton(struct bpf_object *obj, size_t prog_cnt, bool populate_li
>         }
>  }
>
> +static int walk_st_ops_shadow_vars(struct btf *btf,
> +                                  const char *ident,
> +                                  const struct bpf_map *map)
> +{
> +       DECLARE_LIBBPF_OPTS(btf_dump_emit_type_decl_opts, opts,
> +                           .indent_level = 3,
> +                           );

DECLARE_LIBBPF_OPTS is legacy, please use shorter (but equivalent)
LIBBPF_OPTS() macro. Also formatting is a bit weird, let's do just

LIBBPF_OPTS(btf_dump_emit_type_decl_opts, opts, .indent_level = 3);

> +       const struct btf_type *map_type, *member_type;
> +       __u32 map_type_id, member_type_id;
> +       __u32 offset, next_offset = 0;
> +       const struct btf_member *m;
> +       const char *member_name;
> +       struct btf_dump *d = NULL;
> +       int i, err = 0;
> +       int size, map_size;
> +
> +       map_type_id = bpf_map__btf_value_type_id(map);
> +       if (map_type_id == 0)
> +               return -EINVAL;
> +       map_type = btf__type_by_id(btf, map_type_id);
> +       if (!map_type)
> +               return -EINVAL;
> +
> +       d = btf_dump__new(btf, codegen_btf_dump_printf, NULL, NULL);
> +       if (!d)
> +               return -errno;
> +
> +       for (i = 0, m = btf_members(map_type);
> +            i < btf_vlen(map_type);
> +            i++, m++) {

let's move `n = btf_vlen(map_type)` out of for loop and keep for()
itself as a single-line

> +               member_type = skip_mods_and_typedefs(btf, m->type,
> +                                                    &member_type_id);

the line length limit is 100, try to keep single-lines if possible

> +               if (!member_type) {
> +                       err = -EINVAL;
> +                       goto out;
> +               }

this can't happen, no need to add so much error handling

> +
> +               member_name = btf__name_by_offset(btf, m->name_off);
> +               if (!member_name) {
> +                       err = -EINVAL;
> +                       goto out;
> +               }

same here, let's assume BTF is correct, no need to double-check these things

> +
> +               offset = m->offset / 8;
> +               if (next_offset != offset) {
> +                       printf("\t\t\tchar __padding_%d[%d];\n",
> +                              i - 1, offset - next_offset);

why i-1? that will give us `__padding_-1[...]` if the very first field
is not aligned (I know, it's unlikely and hypothetical, but still).
Let's just use i as a number for simplicity.

> +               }
> +
> +               switch (btf_kind(member_type)) {
> +               case BTF_KIND_INT:
> +               case BTF_KIND_FLOAT:
> +               case BTF_KIND_ENUM:
> +               case BTF_KIND_ENUM64:
> +                       /* scalar type */
> +                       printf("\t\t\t");
> +                       opts.field_name = member_name;
> +                       err = btf_dump__emit_type_decl(d, member_type_id,
> +                                                      &opts);

same nit about single lines up to 100 characters (I didn't measure if
this fits, but I hope it does)

> +                       if (err)
> +                               goto out;
> +                       printf(";\n");
> +
> +                       size = btf__resolve_size(btf, member_type_id);
> +                       if (size < 0) {
> +                               err = size;
> +                               goto out;
> +                       }
> +
> +                       next_offset = offset + size;
> +                       break;
> +
> +               case BTF_KIND_PTR:
> +                       if (resolve_func_ptr(btf, m->type, NULL)) {
> +                               /* Function pointer */
> +                               printf("\t\t\tconst struct bpf_program *%s;\n",

Why const? technically, user can call libbpf bpf_program___*() APIs on
this and adjust them before the skeleton is loaded, right? Not sure if
const buys us anything, so I'd drop it.

> +                                      member_name);
> +
> +                               next_offset = offset + sizeof(void *);
> +                               break;
> +                       }
> +                       /* All pointer types are unsupported except for
> +                        * function pointers.
> +                        */
> +                       fallthrough;
> +
> +               default:
> +                       /* Unsupported types
> +                        *
> +                        * Types other than scalar types and function
> +                        * pointers are currently not supported in order to
> +                        * prevent conflicts in the generated code caused
> +                        * by multiple definitions. For instance, if the
> +                        * struct type FOO is used in a struct_ops map,
> +                        * bpftool has to generate definitions for FOO,
> +                        * which may result in conflicts if FOO is defined
> +                        * in different skeleton files.
> +                        */
> +                       if (i == btf_vlen(map_type) - 1) {
> +                               map_size = btf__resolve_size(btf, map_type_id);
> +                               if (map_size < 0)
> +                                       return -EINVAL;
> +                               size = map_size - offset;
> +                       } else {
> +                               size = (m[1].offset - m->offset) / 8;
> +                       }

You are trying to support both unsupported field and any necessary
padding with the same field. I think it's just confuses things. Let's
keep this part just for unsupported field (and thus use field's size),
and then any extra padding between fields or at the end of a struct
should be handled just as pure padding.

> +
> +                       printf("\t\t\tchar __padding_%d[%d];\n", i, size);

should we name this `__unsupported_%d[%d]` to distinguish from
padding? It would be easier to realize that some portions are actually
not-yet-supported things and call it out, if it should be supported

> +
> +                       next_offset = offset + size;
> +                       break;
> +               }
> +       }

With the above remark about splitting unsupported and padding, you
need to handle remaining padding at the end of a struct here after the
loop

> +
> +out:
> +       btf_dump__free(d);
> +
> +       return err;
> +}
> +
> +/* Generate the pointer of the shadow type for a struct_ops map.
> + *
> + * This function adds a pointer of the shadow type for a struct_ops map.
> + * The members of a struct_ops map can be exported through a pointer to a
> + * shadow type. The user can access these members through the pointer.
> + *
> + * A shadow type includes not all members, only members of some types.
> + * They are scalar types and function pointers. The function pointers are
> + * translated to the pointer of the struct bpf_program. The scalar types
> + * are translated to the original type without any modifiers.
> + *
> + * Unsupported types will be translated to a char array to occupy the same
> + * space as the original field. However, the user should not access them
> + * directly. These unsupported fields are also renamed as __padding_*

as mentioned above, I think it's useful to have them named
__unsupported_* so that users can realize that there is something that
is unsupported

> + * . They may be reordered or shifted due to changes in the original struct

nit: dot at the beginning of the line. Also the comment about "may be
reordered or shifted" is confusing, I'm not sure what the problem is.
I'd drop this part of the comment completely, I don't think anything
can be shifted/shuffled, we just don't expose some fields and replace
them with opaque bytes.

> + * type. Accessing them through the generated names may unintentionally
> + * corrupt data.
> + */
> +static int gen_st_ops_shadow_type(struct btf *btf, const char *ident,
> +                                 const struct bpf_map *map)
> +{
> +       int err;
> +
> +       printf("\t\tstruct {\n");

would it be useful to still name this type? E.g., if it is `struct
bpf_struct_ops_tcp_congestion_ops in vmlinux BTF` we can name this one
as <skeleton-name>__bpf_struct_ops_tcp_congestion_ops. We have a
similar pattern for bss/data/rodata sections, having names is useful.

> +
> +       err = walk_st_ops_shadow_vars(btf, ident, map);
> +       if (err)
> +               return err;
> +
> +       printf("\t\t} *%s;\n", ident);
> +
> +       return 0;
> +}
> +
> +static int gen_st_ops_shadow(struct btf *btf, struct bpf_object *obj)
> +{
> +       struct bpf_map *map;
> +       char ident[256];
> +       int err;
> +
> +       /* Generate the pointers to shadow types of
> +        * struct_ops maps.
> +        */
> +       printf("\tstruct {\n");
> +       bpf_object__for_each_map(map, obj) {
> +               if (bpf_map__type(map) != BPF_MAP_TYPE_STRUCT_OPS)
> +                       continue;
> +               if (!get_map_ident(map, ident, sizeof(ident)))
> +                       continue;
> +               err = gen_st_ops_shadow_type(btf, ident, map);
> +               if (err)
> +                       return err;
> +       }
> +       printf("\t} struct_ops;\n");
> +
> +       return 0;
> +}
> +
> +/* Generate the code to initialize the pointers of shadow types. */
> +static void gen_st_ops_shadow_init(struct btf *btf, struct bpf_object *obj)
> +{
> +       struct bpf_map *map;
> +       char ident[256];
> +
> +       /* Initialize the pointers to_ops shadow types of
> +        * struct_ops maps.
> +        */
> +       bpf_object__for_each_map(map, obj) {
> +               if (bpf_map__type(map) != BPF_MAP_TYPE_STRUCT_OPS)
> +                       continue;
> +               if (!get_map_ident(map, ident, sizeof(ident)))
> +                       continue;
> +               codegen("\
> +                       \n\
> +                               obj->struct_ops.%1$s =                      \n\
> +                                       bpf_map__initial_value(obj->maps.%1$s, NULL);\n\

nit: keep on single line?

> +                       \n\
> +                       ", ident);
> +       }
> +}
> +
>  static int do_skeleton(int argc, char **argv)
>  {
>         char header_guard[MAX_OBJ_NAME_LEN + sizeof("__SKEL_H__")];
> @@ -923,6 +1124,7 @@ static int do_skeleton(int argc, char **argv)
>         struct bpf_map *map;
>         struct btf *btf;
>         struct stat st;
> +       int st_ops_cnt = 0;
>
>         if (!REQ_ARGS(1)) {
>                 usage();
> @@ -1039,6 +1241,8 @@ static int do_skeleton(int argc, char **argv)
>                 );
>         }
>
> +       btf = bpf_object__btf(obj);
> +
>         if (map_cnt) {
>                 printf("\tstruct {\n");
>                 bpf_object__for_each_map(map, obj) {
> @@ -1048,8 +1252,15 @@ static int do_skeleton(int argc, char **argv)
>                                 printf("\t\tstruct bpf_map_desc %s;\n", ident);
>                         else
>                                 printf("\t\tstruct bpf_map *%s;\n", ident);
> +                       if (bpf_map__type(map) == BPF_MAP_TYPE_STRUCT_OPS)
> +                               st_ops_cnt++;
>                 }
>                 printf("\t} maps;\n");
> +               if (st_ops_cnt && btf) {
> +                       err = gen_st_ops_shadow(btf, obj);
> +                       if (err)
> +                               goto out;
> +               }

move it out of `if (map_cnt) { ... }` block, just like you do it in
do_subskeleton?

>         }
>
>         if (prog_cnt) {
> @@ -1075,7 +1286,6 @@ static int do_skeleton(int argc, char **argv)
>                 printf("\t} links;\n");
>         }
>
> -       btf = bpf_object__btf(obj);
>         if (btf) {
>                 err = codegen_datasecs(obj, obj_name);
>                 if (err)
> @@ -1133,6 +1343,13 @@ static int do_skeleton(int argc, char **argv)
>                         if (err)                                            \n\
>                                 goto err_out;                               \n\
>                                                                             \n\
> +               ", obj_name);
> +
> +       if (st_ops_cnt && btf)
> +               gen_st_ops_shadow_init(btf, obj);

gen_st_ops_shadow_init() and gen_st_ops_shadow() can do st_ops_cnt
calculation inside, keeping this high-level logic a bit cleaner. Just
call `gen_st_ops_shadow_init()` unconditionally and let it do nothing
if there are no struct_ops maps (or no BTF).

> +
> +       codegen("\
> +               \n\
>                         return obj;                                         \n\
>                 err_out:                                                    \n\
>                         %1$s__destroy(obj);                                 \n\
> @@ -1296,6 +1513,7 @@ static int do_subskeleton(int argc, char **argv)
>         struct btf *btf;
>         const struct btf_type *map_type, *var_type;
>         const struct btf_var_secinfo *var;
> +       int st_ops_cnt = 0;
>         struct stat st;
>
>         if (!REQ_ARGS(1)) {
> @@ -1438,10 +1656,18 @@ static int do_subskeleton(int argc, char **argv)
>                         if (!get_map_ident(map, ident, sizeof(ident)))
>                                 continue;
>                         printf("\t\tstruct bpf_map *%s;\n", ident);
> +                       if (bpf_map__type(map) == BPF_MAP_TYPE_STRUCT_OPS)
> +                               st_ops_cnt++;

see above, I think it can be hidden inside gen_st_ops_shadow[_init]()
functions to keep things cleaner


>                 }
>                 printf("\t} maps;\n");
>         }
>
> +       if (st_ops_cnt && btf) {
> +               err = gen_st_ops_shadow(btf, obj);
> +               if (err)
> +                       goto out;
> +       }
> +
>         if (prog_cnt) {
>                 printf("\tstruct {\n");
>                 bpf_object__for_each_program(prog, obj) {
> @@ -1553,6 +1779,13 @@ static int do_subskeleton(int argc, char **argv)
>                         if (err)                                            \n\
>                                 goto err;                                   \n\
>                                                                             \n\
> +               ");
> +
> +       if (st_ops_cnt && btf)
> +               gen_st_ops_shadow_init(btf, obj);
> +
> +       codegen("\
> +               \n\
>                         return obj;                                         \n\
>                 err:                                                        \n\
>                         %1$s__destroy(obj);                                 \n\
> --
> 2.34.1
>

Kui-Feng Lee Feb. 28, 2024, 9:21 p.m. UTC | #2

Will fix most of issues.

On 2/28/24 10:25, Andrii Nakryiko wrote:
> On Mon, Feb 26, 2024 at 5:04 PM Kui-Feng Lee <[e-mail]> wrote:
>>
>> + * type. Accessing them through the generated names may unintentionally
>> + * corrupt data.
>> + */
>> +static int gen_st_ops_shadow_type(struct btf *btf, const char *ident,
>> +                                 const struct bpf_map *map)
>> +{
>> +       int err;
>> +
>> +       printf("\t\tstruct {\n");
> 
> would it be useful to still name this type? E.g., if it is `struct
> bpf_struct_ops_tcp_congestion_ops in vmlinux BTF` we can name this one
> as <skeleton-name>__bpf_struct_ops_tcp_congestion_ops. We have a
> similar pattern for bss/data/rodata sections, having names is useful.

If a user defines several struct_ops maps with the same name and type in
different files, it can cause name conflicts. Unless we also prefix the
name with the name of the skeleton. I am not sure if it is a good idea
to generate such long names. If a user want to refer to the type, he
still can use typeof(). WDYT?

> 
>> +
>> +       err = walk_st_ops_shadow_vars(btf, ident, map);
>> +       if (err)
>> +               return err;
>> +
>> +       printf("\t\t} *%s;\n", ident);
>> +
>> +       return 0;
>> +}
>> +

Kui-Feng Lee Feb. 28, 2024, 10:28 p.m. UTC | #3

On 2/28/24 13:21, Kui-Feng Lee wrote:
> Will fix most of issues.
> 
> On 2/28/24 10:25, Andrii Nakryiko wrote:
>> On Mon, Feb 26, 2024 at 5:04 PM Kui-Feng Lee <[e-mail]> 
>> wrote:
>>>
>>> + * type. Accessing them through the generated names may unintentionally
>>> + * corrupt data.
>>> + */
>>> +static int gen_st_ops_shadow_type(struct btf *btf, const char *ident,
>>> +                                 const struct bpf_map *map)
>>> +{
>>> +       int err;
>>> +
>>> +       printf("\t\tstruct {\n");
>>
>> would it be useful to still name this type? E.g., if it is `struct
>> bpf_struct_ops_tcp_congestion_ops in vmlinux BTF` we can name this one
>> as <skeleton-name>__bpf_struct_ops_tcp_congestion_ops. We have a
>> similar pattern for bss/data/rodata sections, having names is useful.
> 
> If a user defines several struct_ops maps with the same name and type in
> different files, it can cause name conflicts. Unless we also prefix the
> name with the name of the skeleton. I am not sure if it is a good idea
> to generate such long names. If a user want to refer to the type, he
> still can use typeof(). WDYT?

I misread your words. So, you were saying to prefix the skeleton name, 
not map names. It is doable.

> 
>>
>>> +
>>> +       err = walk_st_ops_shadow_vars(btf, ident, map);
>>> +       if (err)
>>> +               return err;
>>> +
>>> +       printf("\t\t} *%s;\n", ident);
>>> +
>>> +       return 0;
>>> +}
>>> +

Andrii Nakryiko Feb. 29, 2024, 12:09 a.m. UTC | #4

On Wed, Feb 28, 2024 at 2:28 PM Kui-Feng Lee <[e-mail]> wrote:
>
>
>
> On 2/28/24 13:21, Kui-Feng Lee wrote:
> > Will fix most of issues.
> >
> > On 2/28/24 10:25, Andrii Nakryiko wrote:
> >> On Mon, Feb 26, 2024 at 5:04 PM Kui-Feng Lee <[e-mail]>
> >> wrote:
> >>>
> >>> + * type. Accessing them through the generated names may unintentionally
> >>> + * corrupt data.
> >>> + */
> >>> +static int gen_st_ops_shadow_type(struct btf *btf, const char *ident,
> >>> +                                 const struct bpf_map *map)
> >>> +{
> >>> +       int err;
> >>> +
> >>> +       printf("\t\tstruct {\n");
> >>
> >> would it be useful to still name this type? E.g., if it is `struct
> >> bpf_struct_ops_tcp_congestion_ops in vmlinux BTF` we can name this one
> >> as <skeleton-name>__bpf_struct_ops_tcp_congestion_ops. We have a
> >> similar pattern for bss/data/rodata sections, having names is useful.
> >
> > If a user defines several struct_ops maps with the same name and type in
> > different files, it can cause name conflicts. Unless we also prefix the
> > name with the name of the skeleton. I am not sure if it is a good idea
> > to generate such long names. If a user want to refer to the type, he
> > still can use typeof(). WDYT?
>
> I misread your words. So, you were saying to prefix the skeleton name,
> not map names. It is doable.

I did say to prefix with skeleton name, but *that* actually can lead
to a conflict if you have two struct_ops maps that use the same BTF
type. On the other hand, map names are unique, they are forced to be
global symbols, so there is no way for them to conflict (it would be
link-time error).

How about we append both skeleton name, map name, and map's underlying
BTF type? So:

<skel>__<map>__bpf_struct_ops_tcp_congestion_ops

?

Is there any problem with having a long name?

>
> >
> >>
> >>> +
> >>> +       err = walk_st_ops_shadow_vars(btf, ident, map);
> >>> +       if (err)
> >>> +               return err;
> >>> +
> >>> +       printf("\t\t} *%s;\n", ident);
> >>> +
> >>> +       return 0;
> >>> +}
> >>> +

Kui-Feng Lee Feb. 29, 2024, 12:44 a.m. UTC | #5

On 2/28/24 16:09, Andrii Nakryiko wrote:
> On Wed, Feb 28, 2024 at 2:28 PM Kui-Feng Lee <[e-mail]> wrote:
>>
>>
>>
>> On 2/28/24 13:21, Kui-Feng Lee wrote:
>>> Will fix most of issues.
>>>
>>> On 2/28/24 10:25, Andrii Nakryiko wrote:
>>>> On Mon, Feb 26, 2024 at 5:04 PM Kui-Feng Lee <[e-mail]>
>>>> wrote:
>>>>>
>>>>> + * type. Accessing them through the generated names may unintentionally
>>>>> + * corrupt data.
>>>>> + */
>>>>> +static int gen_st_ops_shadow_type(struct btf *btf, const char *ident,
>>>>> +                                 const struct bpf_map *map)
>>>>> +{
>>>>> +       int err;
>>>>> +
>>>>> +       printf("\t\tstruct {\n");
>>>>
>>>> would it be useful to still name this type? E.g., if it is `struct
>>>> bpf_struct_ops_tcp_congestion_ops in vmlinux BTF` we can name this one
>>>> as <skeleton-name>__bpf_struct_ops_tcp_congestion_ops. We have a
>>>> similar pattern for bss/data/rodata sections, having names is useful.
>>>
>>> If a user defines several struct_ops maps with the same name and type in
>>> different files, it can cause name conflicts. Unless we also prefix the
>>> name with the name of the skeleton. I am not sure if it is a good idea
>>> to generate such long names. If a user want to refer to the type, he
>>> still can use typeof(). WDYT?
>>
>> I misread your words. So, you were saying to prefix the skeleton name,
>> not map names. It is doable.
> 
> I did say to prefix with skeleton name, but *that* actually can lead
> to a conflict if you have two struct_ops maps that use the same BTF
> type. On the other hand, map names are unique, they are forced to be
> global symbols, so there is no way for them to conflict (it would be
> link-time error).

I avoided conflicts by checking if the definition of a type is already
generated.

For example, if there are two maps with the same type, they would looks 
like.
struct XXXSekelton {
     ...
     struct {
         struct struct_ops_type {
              ....
         } *map1;
         struct struct_ops_type *map2;
     } struct_ops;
   ...
};

WDYT?

> 
> How about we append both skeleton name, map name, and map's underlying
> BTF type? So:
> 
> <skel>__<map>__bpf_struct_ops_tcp_congestion_ops
> 
> ?
> 
> Is there any problem with having a long name?

No a big problem! Just not convenient to use.

> 
>>
>>>
>>>>
>>>>> +
>>>>> +       err = walk_st_ops_shadow_vars(btf, ident, map);
>>>>> +       if (err)
>>>>> +               return err;
>>>>> +
>>>>> +       printf("\t\t} *%s;\n", ident);
>>>>> +
>>>>> +       return 0;
>>>>> +}
>>>>> +

Kui-Feng Lee Feb. 29, 2024, 12:51 a.m. UTC | #6

On 2/28/24 16:44, Kui-Feng Lee wrote:
> 
> 
> On 2/28/24 16:09, Andrii Nakryiko wrote:
>> On Wed, Feb 28, 2024 at 2:28 PM Kui-Feng Lee <[e-mail]> wrote:
>>>
>>>
>>>
>>> On 2/28/24 13:21, Kui-Feng Lee wrote:
>>>> Will fix most of issues.
>>>>
>>>> On 2/28/24 10:25, Andrii Nakryiko wrote:
>>>>> On Mon, Feb 26, 2024 at 5:04 PM Kui-Feng Lee <[e-mail]>
>>>>> wrote:
>>>>>>
>>>>>> + * type. Accessing them through the generated names may 
>>>>>> unintentionally
>>>>>> + * corrupt data.
>>>>>> + */
>>>>>> +static int gen_st_ops_shadow_type(struct btf *btf, const char 
>>>>>> *ident,
>>>>>> +                                 const struct bpf_map *map)
>>>>>> +{
>>>>>> +       int err;
>>>>>> +
>>>>>> +       printf("\t\tstruct {\n");
>>>>>
>>>>> would it be useful to still name this type? E.g., if it is `struct
>>>>> bpf_struct_ops_tcp_congestion_ops in vmlinux BTF` we can name this one
>>>>> as <skeleton-name>__bpf_struct_ops_tcp_congestion_ops. We have a
>>>>> similar pattern for bss/data/rodata sections, having names is useful.
>>>>
>>>> If a user defines several struct_ops maps with the same name and 
>>>> type in
>>>> different files, it can cause name conflicts. Unless we also prefix the
>>>> name with the name of the skeleton. I am not sure if it is a good idea
>>>> to generate such long names. If a user want to refer to the type, he
>>>> still can use typeof(). WDYT?
>>>
>>> I misread your words. So, you were saying to prefix the skeleton name,
>>> not map names. It is doable.
>>
>> I did say to prefix with skeleton name, but *that* actually can lead
>> to a conflict if you have two struct_ops maps that use the same BTF
>> type. On the other hand, map names are unique, they are forced to be
>> global symbols, so there is no way for them to conflict (it would be
>> link-time error).
> 
> I avoided conflicts by checking if the definition of a type is already
> generated.
> 
> For example, if there are two maps with the same type, they would looks 
> like.
> struct XXXSekelton {
>      ...
>      struct {
>          struct struct_ops_type {
>               ....
>          } *map1;
>          struct struct_ops_type *map2;
>      } struct_ops;
>    ...
> };
> 
> WDYT?

Sorry! It should be "<skeleton-type>_bpf_struct_ops_tcp_congestion_ops"
instead of "struct_ops_type".

> 
>>
>> How about we append both skeleton name, map name, and map's underlying
>> BTF type? So:
>>
>> <skel>__<map>__bpf_struct_ops_tcp_congestion_ops
>>
>> ?
>>
>> Is there any problem with having a long name?
> 
> No a big problem! Just not convenient to use.
> 
>>
>>>
>>>>
>>>>>
>>>>>> +
>>>>>> +       err = walk_st_ops_shadow_vars(btf, ident, map);
>>>>>> +       if (err)
>>>>>> +               return err;
>>>>>> +
>>>>>> +       printf("\t\t} *%s;\n", ident);
>>>>>> +
>>>>>> +       return 0;
>>>>>> +}
>>>>>> +

Andrii Nakryiko Feb. 29, 2024, 1:03 a.m. UTC | #7

On Wed, Feb 28, 2024 at 4:44 PM Kui-Feng Lee <[e-mail]> wrote:
>
>
>
> On 2/28/24 16:09, Andrii Nakryiko wrote:
> > On Wed, Feb 28, 2024 at 2:28 PM Kui-Feng Lee <[e-mail]> wrote:
> >>
> >>
> >>
> >> On 2/28/24 13:21, Kui-Feng Lee wrote:
> >>> Will fix most of issues.
> >>>
> >>> On 2/28/24 10:25, Andrii Nakryiko wrote:
> >>>> On Mon, Feb 26, 2024 at 5:04 PM Kui-Feng Lee <[e-mail]>
> >>>> wrote:
> >>>>>
> >>>>> + * type. Accessing them through the generated names may unintentionally
> >>>>> + * corrupt data.
> >>>>> + */
> >>>>> +static int gen_st_ops_shadow_type(struct btf *btf, const char *ident,
> >>>>> +                                 const struct bpf_map *map)
> >>>>> +{
> >>>>> +       int err;
> >>>>> +
> >>>>> +       printf("\t\tstruct {\n");
> >>>>
> >>>> would it be useful to still name this type? E.g., if it is `struct
> >>>> bpf_struct_ops_tcp_congestion_ops in vmlinux BTF` we can name this one
> >>>> as <skeleton-name>__bpf_struct_ops_tcp_congestion_ops. We have a
> >>>> similar pattern for bss/data/rodata sections, having names is useful.
> >>>
> >>> If a user defines several struct_ops maps with the same name and type in
> >>> different files, it can cause name conflicts. Unless we also prefix the
> >>> name with the name of the skeleton. I am not sure if it is a good idea
> >>> to generate such long names. If a user want to refer to the type, he
> >>> still can use typeof(). WDYT?
> >>
> >> I misread your words. So, you were saying to prefix the skeleton name,
> >> not map names. It is doable.
> >
> > I did say to prefix with skeleton name, but *that* actually can lead
> > to a conflict if you have two struct_ops maps that use the same BTF
> > type. On the other hand, map names are unique, they are forced to be
> > global symbols, so there is no way for them to conflict (it would be
> > link-time error).
>
> I avoided conflicts by checking if the definition of a type is already
> generated.
>
> For example, if there are two maps with the same type, they would looks
> like.
> struct XXXSekelton {
>      ...
>      struct {
>          struct struct_ops_type {
>               ....
>          } *map1;
>          struct struct_ops_type *map2;

It's kind of non-uniform. I think we are overengineering this, let's
just do <skel>__<map>__<type> and see how it goes? No checks, no
nothing, pure string generation.

>      } struct_ops;
>    ...
> };
>
> WDYT?
>
> >
> > How about we append both skeleton name, map name, and map's underlying
> > BTF type? So:
> >
> > <skel>__<map>__bpf_struct_ops_tcp_congestion_ops
> >
> > ?
> >
> > Is there any problem with having a long name?
>
> No a big problem! Just not convenient to use.
>
> >
> >>
> >>>
> >>>>
> >>>>> +
> >>>>> +       err = walk_st_ops_shadow_vars(btf, ident, map);
> >>>>> +       if (err)
> >>>>> +               return err;
> >>>>> +
> >>>>> +       printf("\t\t} *%s;\n", ident);
> >>>>> +
> >>>>> +       return 0;
> >>>>> +}
> >>>>> +

Kui-Feng Lee Feb. 29, 2024, 1:14 a.m. UTC | #8

On 2/28/24 17:03, Andrii Nakryiko wrote:
> On Wed, Feb 28, 2024 at 4:44 PM Kui-Feng Lee <[e-mail]> wrote:
>>
>>
>>
>> On 2/28/24 16:09, Andrii Nakryiko wrote:
>>> On Wed, Feb 28, 2024 at 2:28 PM Kui-Feng Lee <[e-mail]> wrote:
>>>>
>>>>
>>>>
>>>> On 2/28/24 13:21, Kui-Feng Lee wrote:
>>>>> Will fix most of issues.
>>>>>
>>>>> On 2/28/24 10:25, Andrii Nakryiko wrote:
>>>>>> On Mon, Feb 26, 2024 at 5:04 PM Kui-Feng Lee <[e-mail]>
>>>>>> wrote:
>>>>>>>
>>>>>>> + * type. Accessing them through the generated names may unintentionally
>>>>>>> + * corrupt data.
>>>>>>> + */
>>>>>>> +static int gen_st_ops_shadow_type(struct btf *btf, const char *ident,
>>>>>>> +                                 const struct bpf_map *map)
>>>>>>> +{
>>>>>>> +       int err;
>>>>>>> +
>>>>>>> +       printf("\t\tstruct {\n");
>>>>>>
>>>>>> would it be useful to still name this type? E.g., if it is `struct
>>>>>> bpf_struct_ops_tcp_congestion_ops in vmlinux BTF` we can name this one
>>>>>> as <skeleton-name>__bpf_struct_ops_tcp_congestion_ops. We have a
>>>>>> similar pattern for bss/data/rodata sections, having names is useful.
>>>>>
>>>>> If a user defines several struct_ops maps with the same name and type in
>>>>> different files, it can cause name conflicts. Unless we also prefix the
>>>>> name with the name of the skeleton. I am not sure if it is a good idea
>>>>> to generate such long names. If a user want to refer to the type, he
>>>>> still can use typeof(). WDYT?
>>>>
>>>> I misread your words. So, you were saying to prefix the skeleton name,
>>>> not map names. It is doable.
>>>
>>> I did say to prefix with skeleton name, but *that* actually can lead
>>> to a conflict if you have two struct_ops maps that use the same BTF
>>> type. On the other hand, map names are unique, they are forced to be
>>> global symbols, so there is no way for them to conflict (it would be
>>> link-time error).
>>
>> I avoided conflicts by checking if the definition of a type is already
>> generated.
>>
>> For example, if there are two maps with the same type, they would looks
>> like.
>> struct XXXSekelton {
>>       ...
>>       struct {
>>           struct struct_ops_type {
>>                ....
>>           } *map1;
>>           struct struct_ops_type *map2;
> 
> It's kind of non-uniform. I think we are overengineering this, let's
> just do <skel>__<map>__<type> and see how it goes? No checks, no
> nothing, pure string generation.

Got it

> 
>>       } struct_ops;
>>     ...
>> };
>>
>> WDYT?
>>
>>>
>>> How about we append both skeleton name, map name, and map's underlying
>>> BTF type? So:
>>>
>>> <skel>__<map>__bpf_struct_ops_tcp_congestion_ops
>>>
>>> ?
>>>
>>> Is there any problem with having a long name?
>>
>> No a big problem! Just not convenient to use.
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>>> +
>>>>>>> +       err = walk_st_ops_shadow_vars(btf, ident, map);
>>>>>>> +       if (err)
>>>>>>> +               return err;
>>>>>>> +
>>>>>>> +       printf("\t\t} *%s;\n", ident);
>>>>>>> +
>>>>>>> +       return 0;
>>>>>>> +}
>>>>>>> +

Patch

diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
index a9334c57e859..a21c92d95401 100644
--- a/tools/bpf/bpftool/gen.c
+++ b/tools/bpf/bpftool/gen.c
@@ -909,6 +909,207 @@  codegen_progs_skeleton(struct bpf_object *obj, size_t prog_cnt, bool populate_li
 	}
 }
 
+static int walk_st_ops_shadow_vars(struct btf *btf,
+				   const char *ident,
+				   const struct bpf_map *map)
+{
+	DECLARE_LIBBPF_OPTS(btf_dump_emit_type_decl_opts, opts,
+			    .indent_level = 3,
+			    );
+	const struct btf_type *map_type, *member_type;
+	__u32 map_type_id, member_type_id;
+	__u32 offset, next_offset = 0;
+	const struct btf_member *m;
+	const char *member_name;
+	struct btf_dump *d = NULL;
+	int i, err = 0;
+	int size, map_size;
+
+	map_type_id = bpf_map__btf_value_type_id(map);
+	if (map_type_id == 0)
+		return -EINVAL;
+	map_type = btf__type_by_id(btf, map_type_id);
+	if (!map_type)
+		return -EINVAL;
+
+	d = btf_dump__new(btf, codegen_btf_dump_printf, NULL, NULL);
+	if (!d)
+		return -errno;
+
+	for (i = 0, m = btf_members(map_type);
+	     i < btf_vlen(map_type);
+	     i++, m++) {
+		member_type = skip_mods_and_typedefs(btf, m->type,
+						     &member_type_id);
+		if (!member_type) {
+			err = -EINVAL;
+			goto out;
+		}
+
+		member_name = btf__name_by_offset(btf, m->name_off);
+		if (!member_name) {
+			err = -EINVAL;
+			goto out;
+		}
+
+		offset = m->offset / 8;
+		if (next_offset != offset) {
+			printf("\t\t\tchar __padding_%d[%d];\n",
+			       i - 1, offset - next_offset);
+		}
+
+		switch (btf_kind(member_type)) {
+		case BTF_KIND_INT:
+		case BTF_KIND_FLOAT:
+		case BTF_KIND_ENUM:
+		case BTF_KIND_ENUM64:
+			/* scalar type */
+			printf("\t\t\t");
+			opts.field_name = member_name;
+			err = btf_dump__emit_type_decl(d, member_type_id,
+						       &opts);
+			if (err)
+				goto out;
+			printf(";\n");
+
+			size = btf__resolve_size(btf, member_type_id);
+			if (size < 0) {
+				err = size;
+				goto out;
+			}
+
+			next_offset = offset + size;
+			break;
+
+		case BTF_KIND_PTR:
+			if (resolve_func_ptr(btf, m->type, NULL)) {
+				/* Function pointer */
+				printf("\t\t\tconst struct bpf_program *%s;\n",
+				       member_name);
+
+				next_offset = offset + sizeof(void *);
+				break;
+			}
+			/* All pointer types are unsupported except for
+			 * function pointers.
+			 */
+			fallthrough;
+
+		default:
+			/* Unsupported types
+			 *
+			 * Types other than scalar types and function
+			 * pointers are currently not supported in order to
+			 * prevent conflicts in the generated code caused
+			 * by multiple definitions. For instance, if the
+			 * struct type FOO is used in a struct_ops map,
+			 * bpftool has to generate definitions for FOO,
+			 * which may result in conflicts if FOO is defined
+			 * in different skeleton files.
+			 */
+			if (i == btf_vlen(map_type) - 1) {
+				map_size = btf__resolve_size(btf, map_type_id);
+				if (map_size < 0)
+					return -EINVAL;
+				size = map_size - offset;
+			} else {
+				size = (m[1].offset - m->offset) / 8;
+			}
+
+			printf("\t\t\tchar __padding_%d[%d];\n", i, size);
+
+			next_offset = offset + size;
+			break;
+		}
+	}
+
+out:
+	btf_dump__free(d);
+
+	return err;
+}
+
+/* Generate the pointer of the shadow type for a struct_ops map.
+ *
+ * This function adds a pointer of the shadow type for a struct_ops map.
+ * The members of a struct_ops map can be exported through a pointer to a
+ * shadow type. The user can access these members through the pointer.
+ *
+ * A shadow type includes not all members, only members of some types.
+ * They are scalar types and function pointers. The function pointers are
+ * translated to the pointer of the struct bpf_program. The scalar types
+ * are translated to the original type without any modifiers.
+ *
+ * Unsupported types will be translated to a char array to occupy the same
+ * space as the original field. However, the user should not access them
+ * directly. These unsupported fields are also renamed as __padding_*
+ * . They may be reordered or shifted due to changes in the original struct
+ * type. Accessing them through the generated names may unintentionally
+ * corrupt data.
+ */
+static int gen_st_ops_shadow_type(struct btf *btf, const char *ident,
+				  const struct bpf_map *map)
+{
+	int err;
+
+	printf("\t\tstruct {\n");
+
+	err = walk_st_ops_shadow_vars(btf, ident, map);
+	if (err)
+		return err;
+
+	printf("\t\t} *%s;\n", ident);
+
+	return 0;
+}
+
+static int gen_st_ops_shadow(struct btf *btf, struct bpf_object *obj)
+{
+	struct bpf_map *map;
+	char ident[256];
+	int err;
+
+	/* Generate the pointers to shadow types of
+	 * struct_ops maps.
+	 */
+	printf("\tstruct {\n");
+	bpf_object__for_each_map(map, obj) {
+		if (bpf_map__type(map) != BPF_MAP_TYPE_STRUCT_OPS)
+			continue;
+		if (!get_map_ident(map, ident, sizeof(ident)))
+			continue;
+		err = gen_st_ops_shadow_type(btf, ident, map);
+		if (err)
+			return err;
+	}
+	printf("\t} struct_ops;\n");
+
+	return 0;
+}
+
+/* Generate the code to initialize the pointers of shadow types. */
+static void gen_st_ops_shadow_init(struct btf *btf, struct bpf_object *obj)
+{
+	struct bpf_map *map;
+	char ident[256];
+
+	/* Initialize the pointers to_ops shadow types of
+	 * struct_ops maps.
+	 */
+	bpf_object__for_each_map(map, obj) {
+		if (bpf_map__type(map) != BPF_MAP_TYPE_STRUCT_OPS)
+			continue;
+		if (!get_map_ident(map, ident, sizeof(ident)))
+			continue;
+		codegen("\
+			\n\
+				obj->struct_ops.%1$s =			    \n\
+					bpf_map__initial_value(obj->maps.%1$s, NULL);\n\
+			\n\
+			", ident);
+	}
+}
+
 static int do_skeleton(int argc, char **argv)
 {
 	char header_guard[MAX_OBJ_NAME_LEN + sizeof("__SKEL_H__")];
@@ -923,6 +1124,7 @@  static int do_skeleton(int argc, char **argv)
 	struct bpf_map *map;
 	struct btf *btf;
 	struct stat st;
+	int st_ops_cnt = 0;
 
 	if (!REQ_ARGS(1)) {
 		usage();
@@ -1039,6 +1241,8 @@  static int do_skeleton(int argc, char **argv)
 		);
 	}
 
+	btf = bpf_object__btf(obj);
+
 	if (map_cnt) {
 		printf("\tstruct {\n");
 		bpf_object__for_each_map(map, obj) {
@@ -1048,8 +1252,15 @@  static int do_skeleton(int argc, char **argv)
 				printf("\t\tstruct bpf_map_desc %s;\n", ident);
 			else
 				printf("\t\tstruct bpf_map *%s;\n", ident);
+			if (bpf_map__type(map) == BPF_MAP_TYPE_STRUCT_OPS)
+				st_ops_cnt++;
 		}
 		printf("\t} maps;\n");
+		if (st_ops_cnt && btf) {
+			err = gen_st_ops_shadow(btf, obj);
+			if (err)
+				goto out;
+		}
 	}
 
 	if (prog_cnt) {
@@ -1075,7 +1286,6 @@  static int do_skeleton(int argc, char **argv)
 		printf("\t} links;\n");
 	}
 
-	btf = bpf_object__btf(obj);
 	if (btf) {
 		err = codegen_datasecs(obj, obj_name);
 		if (err)
@@ -1133,6 +1343,13 @@  static int do_skeleton(int argc, char **argv)
 			if (err)					    \n\
 				goto err_out;				    \n\
 									    \n\
+		", obj_name);
+
+	if (st_ops_cnt && btf)
+		gen_st_ops_shadow_init(btf, obj);
+
+	codegen("\
+		\n\
 			return obj;					    \n\
 		err_out:						    \n\
 			%1$s__destroy(obj);				    \n\
@@ -1296,6 +1513,7 @@  static int do_subskeleton(int argc, char **argv)
 	struct btf *btf;
 	const struct btf_type *map_type, *var_type;
 	const struct btf_var_secinfo *var;
+	int st_ops_cnt = 0;
 	struct stat st;
 
 	if (!REQ_ARGS(1)) {
@@ -1438,10 +1656,18 @@  static int do_subskeleton(int argc, char **argv)
 			if (!get_map_ident(map, ident, sizeof(ident)))
 				continue;
 			printf("\t\tstruct bpf_map *%s;\n", ident);
+			if (bpf_map__type(map) == BPF_MAP_TYPE_STRUCT_OPS)
+				st_ops_cnt++;
 		}
 		printf("\t} maps;\n");
 	}
 
+	if (st_ops_cnt && btf) {
+		err = gen_st_ops_shadow(btf, obj);
+		if (err)
+			goto out;
+	}
+
 	if (prog_cnt) {
 		printf("\tstruct {\n");
 		bpf_object__for_each_program(prog, obj) {
@@ -1553,6 +1779,13 @@  static int do_subskeleton(int argc, char **argv)
 			if (err)					    \n\
 				goto err;				    \n\
 									    \n\
+		");
+
+	if (st_ops_cnt && btf)
+		gen_st_ops_shadow_init(btf, obj);
+
+	codegen("\
+		\n\
 			return obj;					    \n\
 		err:							    \n\
 			%1$s__destroy(obj);				    \n\