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 17:03, Andrii Nakryiko wrote:
> On Wed, Feb 28, 2024 at 4:44 PM Kui-Feng Lee <sinquersw@gmail.com> 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;
> 
> 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;
>>>>>>> +}
>>>>>>> +