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

[Kui-Feng Lee <sinquersw@gmail.com>]

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 <sinquersw@gmail.com> 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@gmail.com>
>>>>> 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;
>>>>>> +}
>>>>>> +