Re: [RFC PATCH v9 07/11] bpf: net_sched: Allow more optional operators in Qdisc_ops

[Martin KaFai Lau <martin.lau@linux.dev>]

On 7/26/24 3:30 PM, Amery Hung wrote:
>> The pre/post is mainly to initialize and cleanup the "struct bpf_sched_data"
>> before/after calling the bpf prog.
>>
>> For the pre (init), there is a ".gen_prologue(...., const struct bpf_prog
>> *prog)" in the "bpf_verifier_ops". Take a look at the tc_cls_act_prologue().
>> It calls a BPF_FUNC_skb_pull_data helper. It potentially can call a kfunc
>> bpf_qdisc_watchdog_cancel. However, the gen_prologue is invoked too late in the
>> verifier for kfunc calling now. This will need some thoughts and works.
>>
>> For the post (destroy,reset), there is no "gen_epilogue" now. If
>> bpf_qdisc_watchdog_schedule() is not allowed to be called in the ".reset" and
>> ".destroy" bpf prog. I think it can be changed to pre also? There is a ".filter"
>> function in the "struct btf_kfunc_id_set" during the kfunc register.
>>
> I can see how that would work. The ability to add prologue, epilogue
> to struct_ops operators is one thing on my wish list.
> 
> Meanwhile, I am not sure whether that should be written in the kernel
> or rewritten by the verifier. An argument for keeping it in the kernel
> is that the prologue or epilogue can get quite complex and involves
> many kernel structures not exposed to the bpf program (pre-defined ops
> in Qdisc_ops in v8).

Can the v8 pre-defined ops be called as a kfunc? The qdisc_watchdog_cancel/init 
in v9 could be a kfunc and called by pro/epilogue.

For checking and/or resetting skb->dev, it should be simple enough without 
kfunc. e.g. when reusing the skb->rbnode in the future followup effort.

[ Unrelated to qdisc. bpf_tcp_ca can also use help to ensure the cwnd is sane. ]

> 
> Maybe we can keep the current approach in the initial version as they
> are not in the fast path, and then move to (gen_prologue,
> gen_epilogue) once the plumbing is done?

Sure. It can be improved when things are ready.

I am trying some ideas on how to do gen_epilogue and will share when I get some 
tests working.