On Wed, May 22, 2024 at 4:56 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
>
> On 5/10/24 12:24 PM, Amery Hung wrote:
> > +BTF_KFUNCS_START(bpf_qdisc_kfunc_ids)
> > +BTF_ID_FLAGS(func, bpf_skb_set_dev)
> > +BTF_ID_FLAGS(func, bpf_skb_get_hash)
> > +BTF_ID_FLAGS(func, bpf_skb_release, KF_RELEASE)
> > +BTF_ID_FLAGS(func, bpf_qdisc_skb_drop, KF_RELEASE)
> > +BTF_ID_FLAGS(func, bpf_qdisc_watchdog_schedule)
>
> Thanks for working on the bpf qdisc!
>
> I want to see if we can shrink the set and focus on the core pieces first.
>
> The above kfuncs look ok. bpf_skb_set_dev() will need some thoughts but my
> understanding is that it is also not needed if the patch set did not reuse the
> rb_node in the sk_buff?
Correct. I will remove this kfunc and fall back to the v7 approach
(allocating local objects to hold skb kptrs) in the next version.
Support for adding skb natively to bpf graphs can come at a later
time.
>
> > +BTF_ID_FLAGS(func, bpf_skb_tc_classify)
> > +BTF_ID_FLAGS(func, bpf_qdisc_create_child)
> > +BTF_ID_FLAGS(func, bpf_qdisc_find_class)
> > +BTF_ID_FLAGS(func, bpf_qdisc_enqueue, KF_RELEASE)
> > +BTF_ID_FLAGS(func, bpf_qdisc_dequeue, KF_ACQUIRE | KF_RET_NULL)
>
> How about starting with classless qdisc first?
>
> I also wonder if the class/hierarchy can be implemented in the
> bpf_map/bpf_rb_root/bpf_list_head alone. That aside, the patch set shows that
> classful qdisc is something tangible with kfuncs. The classless qdisc bpf
> support does not seem to depend on it. Unless there is something on the classful
> side that really needs to be finalized at this point, I would leave it out from
> the core pieces for now and focus on classless. Does it make sense?
>
Totally make sense! I will simplify the patchset in the next version
by making it classless. Like what you said, with bpf maps and graphs,
sophisticated & hierarchical queues can already be implemented in a
single bpf qdisc.
Just to sum up, to make the patchset landable, I will:
1) fix and keep the first 4 struct_ops patches that support acquiring/
returning referenced kptr
2) defer the support of adding skb to bpf graphs
3) defer Qdisc_class_ops and related kfuncs
> > +BTF_KFUNCS_END(bpf_qdisc_kfunc_ids)
> > +
> > +static const struct btf_kfunc_id_set bpf_qdisc_kfunc_set = {
> > + .owner = THIS_MODULE,
> > + .set = &bpf_qdisc_kfunc_ids,
> > +};
> > +
> > +BTF_ID_LIST(skb_kfunc_dtor_ids)
> > +BTF_ID(struct, sk_buff)
> > +BTF_ID_FLAGS(func, bpf_skb_release, KF_RELEASE)
> > +
> > static const struct bpf_verifier_ops bpf_qdisc_verifier_ops = {
> > .get_func_proto = bpf_qdisc_get_func_proto,
> > .is_valid_access = bpf_qdisc_is_valid_access,
> > @@ -558,6 +781,20 @@ static struct bpf_struct_ops bpf_Qdisc_ops = {
> >
> > static int __init bpf_qdisc_kfunc_init(void)
> > {
> > - return register_bpf_struct_ops(&bpf_Qdisc_ops, Qdisc_ops);
> > + int ret;
> > + const struct btf_id_dtor_kfunc skb_kfunc_dtors[] = {
> > + {
> > + .btf_id = skb_kfunc_dtor_ids[0],
> > + .kfunc_btf_id = skb_kfunc_dtor_ids[1]
> > + },
> > + };
> > +
> > + ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS, &bpf_qdisc_kfunc_set);
> > + ret = ret ?: register_btf_id_dtor_kfuncs(skb_kfunc_dtors,
> > + ARRAY_SIZE(skb_kfunc_dtors),
> > + THIS_MODULE);
> > + ret = ret ?: register_bpf_struct_ops(&bpf_Qdisc_ops, Qdisc_ops);
> > +
> > + return ret;
> > }
> > late_initcall(bpf_qdisc_kfunc_init);
>
