On Fri, Dec 30, 2022 at 4:42 PM Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote: > > On Fri, Dec 30, 2022 at 03:00:21PM -0600, David Vernet wrote: > > > > > > > > Taking bpf_get_current_task() as an example, I think it's better to have > > > > the debate be "should we keep supporting this / are users still using > > > > it?" rather than, "it's UAPI, there's nothing to even discuss". The > > > > point being that even if bpf_get_current_task() is still used, there may > > > > (and inevitably will) be other UAPI helpers that are useless and that we > > > > just can't remove. > > Sorry, missed this question in the previous reply. [...] > > Part of me was trying to find a compromise here to move forward, but > > honestly, I do agree with you that we should aggressively make > > everything a kfunc unless we have a good reason not to, dynptr functions > > included. So I'm willing to walk this suggestion back as well -- let's > > just make these kfuncs. > > Agree that any hard policy like 'only kfuncs from now on' gotta have its limits. > Maybe there will be a strong reason to add a new helper one day, > so we can keep the door open a tiny bit for an exception, > but for dynptr... > There are kfuncs with dynptr already (bpf_verify_pkcs7_signature) > So precedent is already made. bpf_verify_pkcs7_signature() is using dynptr as a pointer to memory. It's a totally valid and intended use case, to pass memory area of statically unknown size, yes. But that's very different from having basic dynptr helpers like is_null() and trim/advance as kfunc. Such helpers are stable, they manipulate generic attributes of dynptr: size, offset, underlying memory pointer. There is nothing unstable and potentially changing about them. > > > Also a reasonable point. My point above was really just a response to > > your claim in [0] that dynptrs are flawed. It wasn't related to kfuncs > > vs. helpers. > > > > [0]: https://lore.kernel.org/all/20221216173526.y3e5go6mgmjrv46l@MacBook-Pro-6.local/ > > The flawed part of dynptr I was explaining here: > https://lore.kernel.org/all/20221225215210.ekmfhyczgubx4rih@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ > > It's not that the whole concept of dynptr is flawed, > but using it as an abstraction on top of skb/xdp. >From original exchange: > > > So just because there is no perfect way to > > > handle all the SKB/XDP physical non-contiguity, doesn't mean that the > > > dynptr concept itself is flawed or not well thought out. It's just > > > > I think that's exactly what it means. dynptr concept is flawed. Must be a lot of typos in here ;) because as written it clearly states that the whole concept of dynptr is flawed. But I'm glad we are finally on the same page at least on this point now. > I don't believe that the extreme performance demands of xdp users are > compatible with 'lets verify in runtime' philosophy of dynptr. > I could be wrong. That's why I'm fine adding dynptr_on_top_of_xdp as kfuncs > and seeing it playing out, but certainly not as a stable helper. > iirc Martin and Kuba had concerns about bits of dynptr(skb | xdp) too. > With kfuncs we can iron out the issues while trying to use it whereas > with helpers we will be stuck for long time in endless mailing list arguments. > It's a win-win for everyone to switch everything to kfuncs.
