On Fri, Feb 14, 2025 at 6:09 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote: > > On 2/14/25 8:45 AM, Amery Hung wrote: > > Hi, > > > > I am splitting the bpf qdisc patchset into smaller landable sets and > > this is the first part. > > > > This patchset supports struct_ops operators that acquire kptrs through > > arguments and operators that return a kptr. A coming new struct_ops use > > case, bpf qdisc [0], has two operators that are not yet supported by > > current struct_ops infrastructure. Qdisc_ops::enqueue requires getting > > referenced skb kptr from the argument; Qdisc_ops::dequeue needs to return > > a referenced skb kptr. This patchset will allow bpf qdisc and other > > potential struct_ops implementers to do so. > > > > For struct_ops implementers: > > > > - To get a kptr from an argument, a struct_ops implementer needs to > > annotate the argument name in the stub function with "__ref" suffix. > > > > - The kptr return will automatically work as we now allow operators that > > return a struct pointer. > > > > - The verifier allows returning a null pointer. More control can be > > added later if there is a future struct_ops implementer only expecting > > valid pointers. > > > > For struct_ops users: > > > > - The referenced kptr acquired through the argument needs to be released > > or xchged into maps just like ones acquired via kfuncs. > > > > - To return a referenced kptr in struct_ops, > > 1) The type of the pointer must matches the return type > > 2) The pointer must comes from the kernel (not locally allocated), and > > 3) The pointer must be in its unmodified form > > I only left some minor comments in the patches. > > A few other thoughts: > > I think https://lore.kernel.org/bpf/20250210174336.2024258-11-ameryhung@xxxxxxxxx/ > should be a good addition also. A new subtest in the prog_tests/pro_epilogue.c > should do for testing it. > > Another thing is disabling tail call in the bpf_qdisc_get_func_proto in > https://lore.kernel.org/bpf/20250210174336.2024258-9-ameryhung@xxxxxxxxx/ > I am wondering if it can be done in a generic way for all struct_ops in > check_struct_ops_btf_id(). At that point, we should know all the "has_tail_call". > I meant only for ops with __ref arg such that it won't break the existing > struct_ops. > > I think both of them could be a followup. Your suggestions make sense. I will follow-up on these two items with patches. Thanks, Amery
