| Message ID | 20240714175130.4051012-7-amery.hung@bytedance.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | bpf qdisc | expand |
On 7/14/24 10:51 AM, Amery Hung wrote: > Add kfuncs for working on skb in qdisc. > > Both bpf_qdisc_skb_drop() and bpf_skb_release() can be used to release > a reference to an skb. However, bpf_qdisc_skb_drop() can only be called > in .enqueue where a to_free skb list is available from kernel to defer Enforcing the bpf_qdisc_skb_drop() kfunc only available to the ".enqueue" is achieved by the "struct bpf_sk_buff_ptr" pointer type only available to the ".enqueue" ops ? > the release. Otherwise, bpf_skb_release() should be used elsewhere. It > is also used in bpf_obj_free_fields() when cleaning up skb in maps and > collections. > > bpf_qdisc_schedule() can be used to schedule the execution of the qdisc. > An example use case is to throttle a qdisc if the time to dequeue the > next packet is known. > > bpf_skb_get_hash() returns the flow hash of an skb, which can be used > to build flow-based queueing algorithms. > > Signed-off-by: Amery Hung <amery.hung@bytedance.com> > --- > net/sched/bpf_qdisc.c | 74 ++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 73 insertions(+), 1 deletion(-) > > diff --git a/net/sched/bpf_qdisc.c b/net/sched/bpf_qdisc.c > index a68fc115d8f8..eff7559aa346 100644 > --- a/net/sched/bpf_qdisc.c > +++ b/net/sched/bpf_qdisc.c > @@ -148,6 +148,64 @@ static int bpf_qdisc_btf_struct_access(struct bpf_verifier_log *log, > return 0; > } > > +__bpf_kfunc_start_defs(); > + > +/* bpf_skb_get_hash - Get the flow hash of an skb. > + * @skb: The skb to get the flow hash from. > + */ > +__bpf_kfunc u32 bpf_skb_get_hash(struct sk_buff *skb) > +{ > + return skb_get_hash(skb); > +} > + > +/* bpf_skb_release - Release an skb reference acquired on an skb immediately. > + * @skb: The skb on which a reference is being released. > + */ > +__bpf_kfunc void bpf_skb_release(struct sk_buff *skb) > +{ > + consume_skb(skb); snippet from the comment of consume_skb(): * Functions identically to kfree_skb, but kfree_skb assumes that the frame * is being dropped after a failure and notes that consume_skb() has a different tracepoint from the kfree_skb also. It is better not to confuse the tracing. I think at least the Qdisc_ops.reset and the btf_id_dtor_kfunc don't fall into the consume_skb(). May be useful to add the kfree_skb[_reason?]() kfunc also? > +} > + > +/* bpf_qdisc_skb_drop - Add an skb to be dropped later to a list. > + * @skb: The skb on which a reference is being released and dropped. > + * @to_free_list: The list of skbs to be dropped. > + */ > +__bpf_kfunc void bpf_qdisc_skb_drop(struct sk_buff *skb, > + struct bpf_sk_buff_ptr *to_free_list) > +{ > + __qdisc_drop(skb, (struct sk_buff **)to_free_list); > +} > + > +/* bpf_qdisc_watchdog_schedule - Schedule a qdisc to a later time using a timer. > + * @sch: The qdisc to be scheduled. > + * @expire: The expiry time of the timer. > + * @delta_ns: The slack range of the timer. > + */ > +__bpf_kfunc void bpf_qdisc_watchdog_schedule(struct Qdisc *sch, u64 expire, u64 delta_ns) > +{ > + struct bpf_sched_data *q = qdisc_priv(sch); > + > + qdisc_watchdog_schedule_range_ns(&q->watchdog, expire, delta_ns); > +} > + > +__bpf_kfunc_end_defs(); > + > +BTF_KFUNCS_START(bpf_qdisc_kfunc_ids) > +BTF_ID_FLAGS(func, bpf_skb_get_hash) Add KF_TRUSTED_ARGS. Avoid cases like getting a skb from walking the skb->next for now. > +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) Also add KF_TRUSTED_ARGS here. > +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, > @@ -347,6 +405,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);
On Thu, Jul 25, 2024 at 3:39 PM Martin KaFai Lau <martin.lau@linux.dev> wrote: > > On 7/14/24 10:51 AM, Amery Hung wrote: > > Add kfuncs for working on skb in qdisc. > > > > Both bpf_qdisc_skb_drop() and bpf_skb_release() can be used to release > > a reference to an skb. However, bpf_qdisc_skb_drop() can only be called > > in .enqueue where a to_free skb list is available from kernel to defer > > Enforcing the bpf_qdisc_skb_drop() kfunc only available to the ".enqueue" is > achieved by the "struct bpf_sk_buff_ptr" pointer type only available to the > ".enqueue" ops ? Yes. I assume it will be better to make this availability check explicit using the .filter you mentioned. > > > the release. Otherwise, bpf_skb_release() should be used elsewhere. It > > is also used in bpf_obj_free_fields() when cleaning up skb in maps and > > collections. > > > > bpf_qdisc_schedule() can be used to schedule the execution of the qdisc. > > An example use case is to throttle a qdisc if the time to dequeue the > > next packet is known. > > > > bpf_skb_get_hash() returns the flow hash of an skb, which can be used > > to build flow-based queueing algorithms. > > > > Signed-off-by: Amery Hung <amery.hung@bytedance.com> > > --- > > net/sched/bpf_qdisc.c | 74 ++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 73 insertions(+), 1 deletion(-) > > > > diff --git a/net/sched/bpf_qdisc.c b/net/sched/bpf_qdisc.c > > index a68fc115d8f8..eff7559aa346 100644 > > --- a/net/sched/bpf_qdisc.c > > +++ b/net/sched/bpf_qdisc.c > > @@ -148,6 +148,64 @@ static int bpf_qdisc_btf_struct_access(struct bpf_verifier_log *log, > > return 0; > > } > > > > +__bpf_kfunc_start_defs(); > > + > > +/* bpf_skb_get_hash - Get the flow hash of an skb. > > + * @skb: The skb to get the flow hash from. > > + */ > > +__bpf_kfunc u32 bpf_skb_get_hash(struct sk_buff *skb) > > +{ > > + return skb_get_hash(skb); > > +} > > + > > +/* bpf_skb_release - Release an skb reference acquired on an skb immediately. > > + * @skb: The skb on which a reference is being released. > > + */ > > +__bpf_kfunc void bpf_skb_release(struct sk_buff *skb) > > +{ > > + consume_skb(skb); > > snippet from the comment of consume_skb(): > > * Functions identically to kfree_skb, but kfree_skb assumes that the frame > * is being dropped after a failure and notes that > > consume_skb() has a different tracepoint from the kfree_skb also. It is better > not to confuse the tracing. > > I think at least the Qdisc_ops.reset and the btf_id_dtor_kfunc don't fall into > the consume_skb(). May be useful to add the kfree_skb[_reason?]() kfunc also? > I see. I will change bpf_skb_release() from using consume_skb() to kfree_skb() (existing qdiscs are not using skb_drop_reason). The skb cleanup mechanism when .reset is called can use rtnl_kfree_skbs(). > > +} > > + > > +/* bpf_qdisc_skb_drop - Add an skb to be dropped later to a list. > > + * @skb: The skb on which a reference is being released and dropped. > > + * @to_free_list: The list of skbs to be dropped. > > + */ > > +__bpf_kfunc void bpf_qdisc_skb_drop(struct sk_buff *skb, > > + struct bpf_sk_buff_ptr *to_free_list) > > +{ > > + __qdisc_drop(skb, (struct sk_buff **)to_free_list); > > +} > > + > > +/* bpf_qdisc_watchdog_schedule - Schedule a qdisc to a later time using a timer. > > + * @sch: The qdisc to be scheduled. > > + * @expire: The expiry time of the timer. > > + * @delta_ns: The slack range of the timer. > > + */ > > +__bpf_kfunc void bpf_qdisc_watchdog_schedule(struct Qdisc *sch, u64 expire, u64 delta_ns) > > +{ > > + struct bpf_sched_data *q = qdisc_priv(sch); > > + > > + qdisc_watchdog_schedule_range_ns(&q->watchdog, expire, delta_ns); > > +} > > + > > +__bpf_kfunc_end_defs(); > > + > > +BTF_KFUNCS_START(bpf_qdisc_kfunc_ids) > > +BTF_ID_FLAGS(func, bpf_skb_get_hash) > > Add KF_TRUSTED_ARGS. Avoid cases like getting a skb from walking the skb->next > for now. Good point. Will do. > > > +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) > > Also add KF_TRUSTED_ARGS here. > > > +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, > > @@ -347,6 +405,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); >
diff --git a/net/sched/bpf_qdisc.c b/net/sched/bpf_qdisc.c index a68fc115d8f8..eff7559aa346 100644 --- a/net/sched/bpf_qdisc.c +++ b/net/sched/bpf_qdisc.c @@ -148,6 +148,64 @@ static int bpf_qdisc_btf_struct_access(struct bpf_verifier_log *log, return 0; } +__bpf_kfunc_start_defs(); + +/* bpf_skb_get_hash - Get the flow hash of an skb. + * @skb: The skb to get the flow hash from. + */ +__bpf_kfunc u32 bpf_skb_get_hash(struct sk_buff *skb) +{ + return skb_get_hash(skb); +} + +/* bpf_skb_release - Release an skb reference acquired on an skb immediately. + * @skb: The skb on which a reference is being released. + */ +__bpf_kfunc void bpf_skb_release(struct sk_buff *skb) +{ + consume_skb(skb); +} + +/* bpf_qdisc_skb_drop - Add an skb to be dropped later to a list. + * @skb: The skb on which a reference is being released and dropped. + * @to_free_list: The list of skbs to be dropped. + */ +__bpf_kfunc void bpf_qdisc_skb_drop(struct sk_buff *skb, + struct bpf_sk_buff_ptr *to_free_list) +{ + __qdisc_drop(skb, (struct sk_buff **)to_free_list); +} + +/* bpf_qdisc_watchdog_schedule - Schedule a qdisc to a later time using a timer. + * @sch: The qdisc to be scheduled. + * @expire: The expiry time of the timer. + * @delta_ns: The slack range of the timer. + */ +__bpf_kfunc void bpf_qdisc_watchdog_schedule(struct Qdisc *sch, u64 expire, u64 delta_ns) +{ + struct bpf_sched_data *q = qdisc_priv(sch); + + qdisc_watchdog_schedule_range_ns(&q->watchdog, expire, delta_ns); +} + +__bpf_kfunc_end_defs(); + +BTF_KFUNCS_START(bpf_qdisc_kfunc_ids) +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) +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, @@ -347,6 +405,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);
Add kfuncs for working on skb in qdisc. Both bpf_qdisc_skb_drop() and bpf_skb_release() can be used to release a reference to an skb. However, bpf_qdisc_skb_drop() can only be called in .enqueue where a to_free skb list is available from kernel to defer the release. Otherwise, bpf_skb_release() should be used elsewhere. It is also used in bpf_obj_free_fields() when cleaning up skb in maps and collections. bpf_qdisc_schedule() can be used to schedule the execution of the qdisc. An example use case is to throttle a qdisc if the time to dequeue the next packet is known. bpf_skb_get_hash() returns the flow hash of an skb, which can be used to build flow-based queueing algorithms. Signed-off-by: Amery Hung <amery.hung@bytedance.com> --- net/sched/bpf_qdisc.c | 74 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 73 insertions(+), 1 deletion(-)