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

On 7/25/24 6:15 PM, Martin KaFai Lau wrote:

quoted

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 5064b6d2d1ec..9fb9375e2793 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c

@@ -1352,6 +1352,13 @@ static struct Qdisc *qdisc_create(struct net_device *dev,

          rcu_assign_pointer(sch->stab, stab);
      }
+#if defined(CONFIG_BPF_SYSCALL) && defined(CONFIG_BPF_JIT)
+    if (sch->flags & TCQ_F_BPF) {

I can see the reason why this patch is needed. It is a few line changes and they 
are not in the fast path... still weakly not excited about them but I know it 
could be a personal preference.

I think at the very least, instead of adding a new TCQ_F_BPF, let see if the 
"owner == BPF_MODULE_OWNER" test can be reused like how it is done in the 
bpf_try_module_get().


A rough direction I am spinning...

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 

typo. The kfunc should be s/qdisc_watchdog_cancel/qdisc_watchdog_init/ for the pre.

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.

quoted

+        err = bpf_qdisc_init_pre_op(sch, tca[TCA_OPTIONS], extack);
+        if (err != 0)
+            goto err_out4;
+    }