Re: [PATCH v2 bpf-next 01/13] bpf: Support multiple arg regs w/ ref_obj_id for kfuncs

[Dave Marchevsky <davemarchevsky@meta.com>]

On 1/17/23 12:36 PM, Alexei Starovoitov wrote:
> On Tue, Jan 17, 2023 at 9:26 AM Dave Marchevsky <davemarchevsky@meta.com> wrote:
>>
>> In another thread you also mentioned that hypothetical "kfunc writer" persona
>> shouldn't have to understand kfunc flags in order to add their simple kfunc, and
>> I think your comments here are also presupposing a "kfunc writer" persona that
>> doesn't look at the verifier. Having such a person able to add kfuncs without
>> understanding the verifier is a good goal, but doesn't reflect current
>> reality when the kfunc needs to have any special semantics.
> 
> agree on that goal.
> 
>> Regardless, I'd expect that anyone adding further new-style Graph
>> datastructures, old-style maps, or new datastructures unrelated to either,
>> will be closer to "verifier expert" than "random person adding a few kfuncs".
> 
> also agree, since it's a reality right now.
> 
>>>> Here we're paving the way for graph (aka new gen data structs)
>>>> and so far not only kfuncs, but their arg types have to have
>>>> special handling inside the verifier.
>>>> There is not much yet to generalize and expose as generic KF_
>>>> flag or as a name suffix.
>>>> Therefore I think it's more appropriate to implement them
>>>> with minimal verifier changes and minimal complexity.
>>>
>>> Agreed
>>>
>>
>> 'Generalize' was addressed in Patch 2's thread.
>>
>>>> There is no 3rd graph algorithm on the horizon after link list
>>>> and rbtree. Instead there is a big todo list for
>>>> 'multi owner graph node' and 'bpf_refcount_t'.
>>>
>>> In this case my point in [0] of the only option for generalizing being
>>> to have something like KF_GRAPH_INSERT / KF_GRAPH_REMOVE is just not the
>>> way forward (which I also said was my opinion when I pointed it out as
>>> an option). Let's just special-case these kfuncs. There's already a
>>> precedence for doing that in the verifier anyways. Minimal complexity,
>>> minimal API changes. It's a win-win.
>>>
>>> [0]: https://lore.kernel.org/all/Y63GLqZil9l1NzY4@maniforge.lan/
>>>
>>
>> There's certainly precedent for adding special-case "kfunc_id == KFUNC_whatever"
>> all over the verifier. It's a bad precedent, though, for reasons discussed in
>> [0].
>>
>> To specifically address your points here, I don't buy the argument that
>> special-casing based on func id is "minimal complexity, minimal API changes".
>> Re: 'complexity': the logic implementing the complicated semantic will be
>> added regardless, it just won't have a name that's easily referenced in docs
>> and mailing list discussions.
>>
>> Similarly, re: 'API changes': if by 'API' here you mean "API that's exposed
>> to folks adding kfuncs" - see my comments about "kfunc writer" persona above.
>> We can think of the verifier itself as an API too - with a single bpf_check
>> function. That API's behavior is indeed changed here, regardless of whether
>> the added semantics are gated by a kfunc flag or special-case checks. I don't
>> think that hiding complexity behind special-case checks when there could be
>> a named flag simplifies anything. The complexity is added regardless, question
>> is how many breadcrumbs and pointers we want to leave for folks trying to make
>> sense of it in the future.
>>
>>   [0]: https://lore.kernel.org/bpf/9763aed7-0284-e400-b4dc-ed01718d8e1e@meta.com/
> 
> I could have agreed to this as well if I didn't go and remove
> all the new KF_*OWN* flags.
> imo the resulting diff of mine vs your initial patch is easier to
> follow and reason about.
> So for this case "kfunc_id == KFUNC_whatever" is cleaner.
> It doesn't mean that it will be the case in other situations.

In the alternate "bpf: Migrate release_on_unlock logic to non-owning ref
semantics" series you submitted, you mean?

It's certainly a smaller diff and easier to reason about as an individual
change. IMO "smaller diff" is largely due to my version moving
convert_owning_non_owning semantics to function-level while yours keeps it at
arg-level. I think moving to function-level is necessary, elaborated on
why in the other deep side-thread [0].

  [0]: https://lore.kernel.org/bpf/9763aed7-0284-e400-b4dc-ed01718d8e1e@meta.com/