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/25/24 6:15 PM, Martin KaFai Lau wrote:
>> 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.
> 
>> +        err = bpf_qdisc_init_pre_op(sch, tca[TCA_OPTIONS], extack);
>> +        if (err != 0)
>> +            goto err_out4;
>> +    }