Re: [PATCH bpf 1/2] bpf: Check timer_off for map_in_map only when map value have timer

[Hengqi Chen <hengqi.chen@gmail.com>]

On 2022/11/28 11:14, Alexei Starovoitov wrote:
> On Sun, Nov 27, 2022 at 7:07 PM Hengqi Chen <hengqi.chen@gmail.com> wrote:
>>
>>
>>
>> On 2022/11/28 10:49, Alexei Starovoitov wrote:
>>> On Sun, Nov 27, 2022 at 6:42 PM Hengqi Chen <hengqi.chen@gmail.com> wrote:
>>>>
>>>> Hi, Alexei:
>>>>
>>>> On 2022/11/28 08:44, Alexei Starovoitov wrote:
>>>>> On Sat, Nov 26, 2022 at 2:54 AM Hengqi Chen <hengqi.chen@gmail.com> wrote:
>>>>>>
>>>>>> The timer_off value could be -EINVAL or -ENOENT when map value of
>>>>>> inner map is struct and contains no bpf_timer. The EINVAL case happens
>>>>>> when the map is created without BTF key/value info, map->timer_off
>>>>>> is set to -EINVAL in map_create(). The ENOENT case happens when
>>>>>> the map is created with BTF key/value info (e.g. from BPF skeleton),
>>>>>> map->timer_off is set to -ENOENT as what btf_find_timer() returns.
>>>>>> In bpf_map_meta_equal(), we expect timer_off to be equal even if
>>>>>> map value does not contains bpf_timer. This rejects map_in_map created
>>>>>> with BTF key/value info to be updated using inner map without BTF
>>>>>> key/value info in case inner map value is struct. This commit lifts
>>>>>> such restriction.
>>>>>
>>>>> Sorry, but I prefer to label this issue as 'wont-fix'.
>>>>> Mixing BTF enabled and non-BTF inner maps is a corner case
>>>>
>>>> We do have such usecase. The BPF progs and maps are pinned to bpffs
>>>> using BPF object file. And the map_in_map is updated by some other
>>>> process which don't have access to such BTF info.
>>>>
>>>>> that is not worth fixing.
>>>>
>>>> Is there a way to get this fixed for v5.x series only ?
>>>>
>>>>> At some point we will require all programs and maps to contain BTF.
>>>>> It's necessary for introspection.
>>>>
>>>> We don't care much about BTF for introspection. In production, we always
>>>> have a version field and some reserved fields in the map value for backward
>>>> compatibility. The interpretation of such map values are left to upper layer.
>>>
>>> That "interpretation of such map values are left to upper layer"...
>>> is exactly the reason why we will enforce BTF in the future.
>>> Production engineers and people outside of "upper layer" sw team
>>> has to be able to debug maps and progs.
>>
>> Fine.
>>
>> In libbpf, we have:
>>
>>   if (is_inner) {
>>         pr_warn("map '%s': inner def can't be pinned.\n", map_name);
>>         return -EINVAL;
>>   }
>>
>>
>> Can we lift this restriction so that we can have an easy way to access BTF info
>> via pinned map ?
> 
> Probably. Note that __uint(pinning, LIBBPF_PIN_BY_NAME)
> is the only mode libbpf understands. It's simplistic.
> but why do you want to use that mode?
> Just pin it directly with bpf_map__pin() ?
> Or even more low level bpf_obj_pin() ?

Will try. 

Currently, we use `__uint(pinning, LIBBPF_PIN_BY_NAME)` and let
libbpf and Cilium's ebpf go library handle all the pinning jobs.