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.
