On Wed, Feb 28, 2024 at 4:44 PM Kui-Feng Lee <sinquersw@xxxxxxxxx> wrote:
>
>
>
> On 2/28/24 16:09, Andrii Nakryiko wrote:
> > On Wed, Feb 28, 2024 at 2:28 PM Kui-Feng Lee <sinquersw@xxxxxxxxx> 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 <thinker.li@xxxxxxxxx>
> >>>> 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;
> >>>>> +}
> >>>>> +
