On 1/17/23 12:36 PM, Alexei Starovoitov wrote: > On Tue, Jan 17, 2023 at 9:26 AM Dave Marchevsky <davemarchevsky@xxxxxxxx> 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@xxxxxxxxxxxxx/ >>> >> >> 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@xxxxxxxx/ > > 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@xxxxxxxx/
