* [PATCH mptcp-next 0/8] split get_subflow interface into two
@ 2024-11-21 2:45 Geliang Tang
2024-11-21 2:45 ` [PATCH mptcp-next 1/8] mptcp: " Geliang Tang
` (8 more replies)
0 siblings, 9 replies; 13+ messages in thread
From: Geliang Tang @ 2024-11-21 2:45 UTC (permalink / raw)
To: mptcp; +Cc: baohua, Geliang Tang
From: Geliang Tang <tanggeliang@kylinos.cn>
I got the following error when running bpf burst sched selftests in auto-btf-debug mode:
[ 167.258293][ T49] BUG: sleeping function called from invalid context at net/core/sock.c:3652
[ 167.258981][ T49] in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid: 49, name: kworker/0:2
[ 167.259547][ T49] preempt_count: 0, expected: 0
[ 167.259826][ T49] RCU nest depth: 1, expected: 0
[ 167.259975][ T49] 5 locks held by kworker/0:2/49:
[ 167.260137][ T49] #0: ffff888001130948 ((wq_completion)events){+.+.}-{0:0}, at: process_one_work+0x7e4/0x16b0
[ 167.260741][ T49] #1: ffffc90000347d90 ((work_completion)(&msk->work)){+.+.}-{0:0}, at: process_one_work+0xdf9/0x16b0
[ 167.261206][ T49] #2: ffff88800c620e58 (sk_lock-AF_INET){+.+.}-{0:0}, at: mptcp_worker+0x7b/0xad0
[ 167.261476][ T49] #3: ffffffff8977e620 (rcu_read_lock){....}-{1:2}, at: __bpf_prog_enter+0x1f/0x170
[ 167.261742][ T49] #4: ffff88800b6f8258 (k-sk_lock-AF_INET#2){+.+.}-{0:0}, at: bpf_prog_87e3c1bd65224f1c_bpf_burst_get_subflow+0x12c/0x58b
[ 167.262309][ T49] CPU: 0 UID: 0 PID: 49 Comm: kworker/0:2 Tainted: G OE 6.12.0-rc6+ #9
[ 167.262591][ T49] Tainted: [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
[ 167.262774][ T49] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[ 167.262950][ T49] Workqueue: events mptcp_worker
[ 167.263097][ T49] Call Trace:
[ 167.263211][ T49] <TASK>
[ 167.263301][ T49] dump_stack_lvl+0x9e/0xe0
[ 167.263453][ T49] __might_resched+0x35d/0x590
[ 167.263599][ T49] ? __pfx___might_resched+0x10/0x10
[ 167.263829][ T49] __lock_sock_fast+0x2f/0xd0
[ 167.264035][ T49] mptcp_pm_nl_subflow_chk_stale+0x1e2/0x3c0
[ 167.264304][ T49] ? bpf_prog_87e3c1bd65224f1c_bpf_burst_get_subflow+0x12c/0x58b
[ 167.264604][ T49] bpf_prog_87e3c1bd65224f1c_bpf_burst_get_subflow+0x12c/0x58b
[ 167.264995][ T49] ? mptcp_sched_get_retrans+0x1f8/0x330
[ 167.265215][ T49] ? __pfx_mptcp_sched_get_retrans+0x10/0x10
[ 167.265432][ T49] ? mark_lock+0x371/0x3c0
[ 167.265678][ T49] ? __mptcp_retrans+0xf4/0x9a0
[ 167.265973][ T49] ? mptcp_worker+0x7b/0xad0
mptcp_pm_subflow_chk_stale is a sleeping function, it shouldn't be
invoked under BPF rcu_read_lock.
One solution is to set mptcp_pm_subflow_chk_stale with KF_SLEEPABLE
flag:
BTF_ID_FLAGS(func, mptcp_pm_subflow_chk_stale, KF_SLEEPABLE)
Then set bpf_burst_get_subflow with BPF_F_SLEEPABLE:
err = bpf_program__set_flags(skel->progs.bpf_burst_get_subflow,
BPF_F_SLEEPABLE);
But another error ocurrs:
[ 101.732098][ C0] BUG: sleeping function called from invalid context at kernel/bpf/trampoline.c:968
[ 101.732781][ C0] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 1640, name: test_progs-no_a
[ 101.733477][ C0] preempt_count: 303, expected: 0
[ 101.733793][ C0] RCU nest depth: 2, expected: 0
[ 101.734043][ C0] 6 locks held by test_progs-no_a/1640:
[ 101.734187][ C0] #0: ffff888007526e58 (sk_lock-AF_INET){+.+.}-{0:0}, at: sk_wait_data (net/core/sock.c:3131 (discriminator 2))
[ 101.734440][ C0] #1: ffffffff95b7e620 (rcu_read_lock){....}-{1:2}, at: process_backlog (include/linux/local_lock_internal.h:38 (discriminator 1) net/core/dev.c:6115 (discriminator 1))
[ 101.734728][ C0] #2: ffffffff95b7e620 (rcu_read_lock){....}-{1:2}, at: ip_local_deliver_finish (include/linux/rcupdate.h:337 (discriminator 1) include/linux/rcupdate.h:849 (discriminator 1) net/ipv4/ip_input.c:232 (discriminator 1))
[ 101.735103][ C0] #3: ffff888008eb8e58 (k-slock-AF_INET/1){+.-.}-{2:2}, at: tcp_v4_rcv (include/linux/skbuff.h:1670 include/net/tcp.h:2530 net/ipv4/tcp_ipv4.c:2348)
[ 101.735560][ C0] #4: ffff8880075261d8 (slock-AF_INET){+.-.}-{2:2}, at: mptcp_incoming_options (net/mptcp/options.c:1053 net/mptcp/options.c:1203)
[ 101.736023][ C0] #5: ffffffff95b7d980 (rcu_read_lock_trace){....}-{0:0}, at: __bpf_prog_enter_sleepable (include/linux/rcupdate.h:337 (discriminator 1) include/linux/rcupdate_trace.h:57 (discriminator 1) kernel/bpf/trampoline.c:966 (discriminator 1))
[ 101.736325][ C0] CPU: 0 UID: 0 PID: 1640 Comm: test_progs-no_a Tainted: G OE 6.12.0-rc7+ #5
[ 101.736676][ C0] Tainted: [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
[ 101.736940][ C0] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[ 101.737146][ C0] Call Trace:
[ 101.737259][ C0] <IRQ>
[ 101.737331][ C0] dump_stack_lvl (lib/dump_stack.c:123)
[ 101.737489][ C0] __might_resched (kernel/sched/core.c:8657)
[ 101.737625][ C0] ? __lock_acquire (kernel/locking/lockdep.c:5202 (discriminator 1))
[ 101.737772][ C0] ? __pfx___might_resched (kernel/sched/core.c:8611)
[ 101.737919][ C0] __might_fault (mm/memory.c:6715 (discriminator 1))
[ 101.738051][ C0] __bpf_prog_enter_sleepable (include/linux/bpf.h:2089 (discriminator 1) kernel/bpf/trampoline.c:970 (discriminator 1))
[ 101.738195][ C0] ? __bpf_prog_enter_sleepable (include/linux/rcupdate.h:337 (discriminator 1) include/linux/rcupdate_trace.h:57 (discriminator 1) kernel/bpf/trampoline.c:966 (discriminator 1))
[ 101.738357][ C0] ? mptcp_sched_get_send (net/mptcp/sched.c:183)
[ 101.738511][ C0] ? __pfx_mptcp_sched_get_send (net/mptcp/sched.c:158)
[ 101.738641][ C0] ? __bpf_tramp_enter (include/linux/rcupdate.h:347 (discriminator 1) include/linux/rcupdate.h:880 (discriminator 1) include/linux/percpu-refcount.h:209 (discriminator 1) include/linux/percpu-refcount.h:222 (discriminator 1) kernel/bpf/trampoline.c:1010 (discriminator 1))
[ 101.738774][ C0] ? bpf_trampoline_6442561300+0x31/0xd7
[ 101.738911][ C0] ? mptcp_sched_get_send (net/mptcp/sched.c:158)
[ 101.739067][ C0] ? __mptcp_subflow_push_pending (net/mptcp/protocol.c:1682 (discriminator 1))
[ 101.739231][ C0] ? __pfx___mptcp_subflow_push_pending (net/mptcp/protocol.c:1655)
[ 101.739378][ C0] ? do_raw_spin_lock (arch/x86/include/asm/atomic.h:107 (discriminator 1) include/linux/atomic/atomic-arch-fallback.h:2170 (discriminator 1) include/linux/atomic/atomic-instrumented.h:1302 (discriminator 1) include/asm-generic/qspinlock.h:111 (discriminator 1) kernel/locking/spinlock_debug.c:116 (discriminator 1))
[ 101.739556][ C0] ? __pfx_do_raw_spin_lock (kernel/locking/spinlock_debug.c:114)
[ 101.739743][ C0] ? mptcp_incoming_options (net/mptcp/options.c:1067 net/mptcp/options.c:1203)
This means get_send() interface of this scheduler can't be set with
BPF_F_SLEEPABLE flag since it's invoked in ack_update_msk() under mptcp
data lock.
So this patch has to split get_subflow() interface of packet scheduer into
two interfaces: get_send() and get_retrans(). Then we can set get_retrans()
interface alone with BPF_F_SLEEPABLE flag.
Thanks to Barry for helping me solve this issue.
Cc: Barry Song <baohua@kernel.org>
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/529
Geliang Tang (8):
mptcp: split get_subflow interface into two
Squash to "selftests/bpf: Add bpf_first scheduler & test"
Squash to "selftests/bpf: Add bpf_bkup scheduler & test"
Squash to "selftests/bpf: Add bpf_rr scheduler & test"
Squash to "selftests/bpf: Add bpf_red scheduler & test"
Squash to "selftests/bpf: Add bpf_burst scheduler & test"
Squash to "bpf: Export mptcp packet scheduler helpers"
Squash to "selftests/bpf: Add bpf_burst scheduler & test"
include/net/mptcp.h | 5 +--
net/mptcp/bpf.c | 13 ++++++--
net/mptcp/sched.c | 33 ++++++++++++-------
.../testing/selftests/bpf/prog_tests/mptcp.c | 14 ++++++--
.../selftests/bpf/progs/mptcp_bpf_bkup.c | 24 +++++++++++---
.../selftests/bpf/progs/mptcp_bpf_burst.c | 32 ++++++++----------
.../selftests/bpf/progs/mptcp_bpf_first.c | 24 +++++++++++---
.../selftests/bpf/progs/mptcp_bpf_red.c | 24 +++++++++++---
.../selftests/bpf/progs/mptcp_bpf_rr.c | 24 +++++++++++---
9 files changed, 136 insertions(+), 57 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH mptcp-next 1/8] mptcp: split get_subflow interface into two
2024-11-21 2:45 [PATCH mptcp-next 0/8] split get_subflow interface into two Geliang Tang
@ 2024-11-21 2:45 ` Geliang Tang
2024-12-21 1:23 ` Mat Martineau
2024-11-21 2:45 ` [PATCH mptcp-next 2/8] Squash to "selftests/bpf: Add bpf_first scheduler & test" Geliang Tang
` (7 subsequent siblings)
8 siblings, 1 reply; 13+ messages in thread
From: Geliang Tang @ 2024-11-21 2:45 UTC (permalink / raw)
To: mptcp; +Cc: baohua, Geliang Tang
From: Geliang Tang <tanggeliang@kylinos.cn>
get_retrans() interface of the burst packet scheduler invokes a sleeping
function mptcp_pm_subflow_chk_stale(), which calls __lock_sock_fast().
So get_retrans() interface should be set with BPF_F_SLEEPABLE flag in
BPF. But get_send() interface of this scheduler can't be set with
BPF_F_SLEEPABLE flag since it's invoked in ack_update_msk() under mptcp
data lock.
So this patch has to split get_subflow() interface of packet scheduer into
two interfaces: get_send() and get_retrans(). Then we can set get_retrans()
interface alone with BPF_F_SLEEPABLE flag.
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
include/net/mptcp.h | 5 +++--
net/mptcp/bpf.c | 11 +++++++++--
net/mptcp/sched.c | 33 ++++++++++++++++++++++-----------
3 files changed, 34 insertions(+), 15 deletions(-)
diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 814b5f2e3ed5..2c85ca92bb1c 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -103,13 +103,14 @@ struct mptcp_out_options {
#define MPTCP_SUBFLOWS_MAX 8
struct mptcp_sched_data {
- bool reinject;
u8 subflows;
struct mptcp_subflow_context *contexts[MPTCP_SUBFLOWS_MAX];
};
struct mptcp_sched_ops {
- int (*get_subflow)(struct mptcp_sock *msk,
+ int (*get_send)(struct mptcp_sock *msk,
+ struct mptcp_sched_data *data);
+ int (*get_retrans)(struct mptcp_sock *msk,
struct mptcp_sched_data *data);
char name[MPTCP_SCHED_NAME_MAX];
diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
index e9db856972cb..ab1de189ba4b 100644
--- a/net/mptcp/bpf.c
+++ b/net/mptcp/bpf.c
@@ -156,7 +156,13 @@ static int bpf_mptcp_sched_init(struct btf *btf)
return 0;
}
-static int __bpf_mptcp_sched_get_subflow(struct mptcp_sock *msk,
+static int __bpf_mptcp_sched_get_send(struct mptcp_sock *msk,
+ struct mptcp_sched_data *data)
+{
+ return 0;
+}
+
+static int __bpf_mptcp_sched_get_retrans(struct mptcp_sock *msk,
struct mptcp_sched_data *data)
{
return 0;
@@ -171,7 +177,8 @@ static void __bpf_mptcp_sched_release(struct mptcp_sock *msk)
}
static struct mptcp_sched_ops __bpf_mptcp_sched_ops = {
- .get_subflow = __bpf_mptcp_sched_get_subflow,
+ .get_send = __bpf_mptcp_sched_get_send,
+ .get_retrans = __bpf_mptcp_sched_get_retrans,
.init = __bpf_mptcp_sched_init,
.release = __bpf_mptcp_sched_release,
};
diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c
index 6da60e35932f..7560693e5b7a 100644
--- a/net/mptcp/sched.c
+++ b/net/mptcp/sched.c
@@ -16,13 +16,25 @@
static DEFINE_SPINLOCK(mptcp_sched_list_lock);
static LIST_HEAD(mptcp_sched_list);
-static int mptcp_sched_default_get_subflow(struct mptcp_sock *msk,
+static int mptcp_sched_default_get_send(struct mptcp_sock *msk,
+ struct mptcp_sched_data *data)
+{
+ struct sock *ssk;
+
+ ssk = mptcp_subflow_get_send(msk);
+ if (!ssk)
+ return -EINVAL;
+
+ mptcp_subflow_set_scheduled(mptcp_subflow_ctx(ssk), true);
+ return 0;
+}
+
+static int mptcp_sched_default_get_retrans(struct mptcp_sock *msk,
struct mptcp_sched_data *data)
{
struct sock *ssk;
- ssk = data->reinject ? mptcp_subflow_get_retrans(msk) :
- mptcp_subflow_get_send(msk);
+ ssk = mptcp_subflow_get_retrans(msk);
if (!ssk)
return -EINVAL;
@@ -31,7 +43,8 @@ static int mptcp_sched_default_get_subflow(struct mptcp_sock *msk,
}
static struct mptcp_sched_ops mptcp_sched_default = {
- .get_subflow = mptcp_sched_default_get_subflow,
+ .get_send = mptcp_sched_default_get_send,
+ .get_retrans = mptcp_sched_default_get_retrans,
.name = "default",
.owner = THIS_MODULE,
};
@@ -73,7 +86,7 @@ void mptcp_get_available_schedulers(char *buf, size_t maxlen)
int mptcp_register_scheduler(struct mptcp_sched_ops *sched)
{
- if (!sched->get_subflow)
+ if (!sched->get_send || !sched->get_retrans)
return -EINVAL;
spin_lock(&mptcp_sched_list_lock);
@@ -184,11 +197,10 @@ int mptcp_sched_get_send(struct mptcp_sock *msk)
return 0;
}
- data.reinject = false;
if (msk->sched == &mptcp_sched_default || !msk->sched)
- return mptcp_sched_default_get_subflow(msk, &data);
+ return mptcp_sched_default_get_send(msk, &data);
mptcp_sched_data_set_contexts(msk, &data);
- return msk->sched->get_subflow(msk, &data);
+ return msk->sched->get_send(msk, &data);
}
int mptcp_sched_get_retrans(struct mptcp_sock *msk)
@@ -207,9 +219,8 @@ int mptcp_sched_get_retrans(struct mptcp_sock *msk)
return 0;
}
- data.reinject = true;
if (msk->sched == &mptcp_sched_default || !msk->sched)
- return mptcp_sched_default_get_subflow(msk, &data);
+ return mptcp_sched_default_get_retrans(msk, &data);
mptcp_sched_data_set_contexts(msk, &data);
- return msk->sched->get_subflow(msk, &data);
+ return msk->sched->get_retrans(msk, &data);
}
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH mptcp-next 2/8] Squash to "selftests/bpf: Add bpf_first scheduler & test"
2024-11-21 2:45 [PATCH mptcp-next 0/8] split get_subflow interface into two Geliang Tang
2024-11-21 2:45 ` [PATCH mptcp-next 1/8] mptcp: " Geliang Tang
@ 2024-11-21 2:45 ` Geliang Tang
2024-12-21 1:28 ` Mat Martineau
2024-11-21 2:45 ` [PATCH mptcp-next 3/8] Squash to "selftests/bpf: Add bpf_bkup " Geliang Tang
` (6 subsequent siblings)
8 siblings, 1 reply; 13+ messages in thread
From: Geliang Tang @ 2024-11-21 2:45 UTC (permalink / raw)
To: mptcp; +Cc: baohua, Geliang Tang
From: Geliang Tang <tanggeliang@kylinos.cn>
Split get_subflow() interface into two: get_send() and get_retrans().
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
.../selftests/bpf/progs/mptcp_bpf_first.c | 24 +++++++++++++++----
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_first.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_first.c
index d57399b407a7..36bf9adffb2c 100644
--- a/tools/testing/selftests/bpf/progs/mptcp_bpf_first.c
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_first.c
@@ -16,18 +16,32 @@ void BPF_PROG(mptcp_sched_first_release, struct mptcp_sock *msk)
{
}
-SEC("struct_ops")
-int BPF_PROG(bpf_first_get_subflow, struct mptcp_sock *msk,
- struct mptcp_sched_data *data)
+static int bpf_first_get_subflow(struct mptcp_sock *msk,
+ struct mptcp_sched_data *data)
{
mptcp_subflow_set_scheduled(bpf_mptcp_subflow_ctx_by_pos(data, 0), true);
return 0;
}
-SEC(".struct_ops")
+SEC("struct_ops")
+int BPF_PROG(bpf_first_get_send, struct mptcp_sock *msk,
+ struct mptcp_sched_data *data)
+{
+ return bpf_first_get_subflow(msk, data);
+}
+
+SEC("struct_ops")
+int BPF_PROG(bpf_first_get_retrans, struct mptcp_sock *msk,
+ struct mptcp_sched_data *data)
+{
+ return bpf_first_get_subflow(msk, data);
+}
+
+SEC(".struct_ops.link")
struct mptcp_sched_ops first = {
.init = (void *)mptcp_sched_first_init,
.release = (void *)mptcp_sched_first_release,
- .get_subflow = (void *)bpf_first_get_subflow,
+ .get_send = (void *)bpf_first_get_send,
+ .get_retrans = (void *)bpf_first_get_retrans,
.name = "bpf_first",
};
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH mptcp-next 3/8] Squash to "selftests/bpf: Add bpf_bkup scheduler & test"
2024-11-21 2:45 [PATCH mptcp-next 0/8] split get_subflow interface into two Geliang Tang
2024-11-21 2:45 ` [PATCH mptcp-next 1/8] mptcp: " Geliang Tang
2024-11-21 2:45 ` [PATCH mptcp-next 2/8] Squash to "selftests/bpf: Add bpf_first scheduler & test" Geliang Tang
@ 2024-11-21 2:45 ` Geliang Tang
2024-11-21 2:45 ` [PATCH mptcp-next 4/8] Squash to "selftests/bpf: Add bpf_rr " Geliang Tang
` (5 subsequent siblings)
8 siblings, 0 replies; 13+ messages in thread
From: Geliang Tang @ 2024-11-21 2:45 UTC (permalink / raw)
To: mptcp; +Cc: baohua, Geliang Tang
From: Geliang Tang <tanggeliang@kylinos.cn>
Split get_subflow() interface into two: get_send() and get_retrans().
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
.../selftests/bpf/progs/mptcp_bpf_bkup.c | 24 +++++++++++++++----
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_bkup.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_bkup.c
index 296f0318d843..41e6639253a4 100644
--- a/tools/testing/selftests/bpf/progs/mptcp_bpf_bkup.c
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_bkup.c
@@ -16,9 +16,8 @@ void BPF_PROG(mptcp_sched_bkup_release, struct mptcp_sock *msk)
{
}
-SEC("struct_ops")
-int BPF_PROG(bpf_bkup_get_subflow, struct mptcp_sock *msk,
- struct mptcp_sched_data *data)
+static int bpf_bkup_get_subflow(struct mptcp_sock *msk,
+ struct mptcp_sched_data *data)
{
int nr = -1;
@@ -43,10 +42,25 @@ int BPF_PROG(bpf_bkup_get_subflow, struct mptcp_sock *msk,
return 0;
}
-SEC(".struct_ops")
+SEC("struct_ops")
+int BPF_PROG(bpf_bkup_get_send, struct mptcp_sock *msk,
+ struct mptcp_sched_data *data)
+{
+ return bpf_bkup_get_subflow(msk, data);
+}
+
+SEC("struct_ops")
+int BPF_PROG(bpf_bkup_get_retrans, struct mptcp_sock *msk,
+ struct mptcp_sched_data *data)
+{
+ return bpf_bkup_get_subflow(msk, data);
+}
+
+SEC(".struct_ops.link")
struct mptcp_sched_ops bkup = {
.init = (void *)mptcp_sched_bkup_init,
.release = (void *)mptcp_sched_bkup_release,
- .get_subflow = (void *)bpf_bkup_get_subflow,
+ .get_send = (void *)bpf_bkup_get_send,
+ .get_retrans = (void *)bpf_bkup_get_retrans,
.name = "bpf_bkup",
};
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH mptcp-next 4/8] Squash to "selftests/bpf: Add bpf_rr scheduler & test"
2024-11-21 2:45 [PATCH mptcp-next 0/8] split get_subflow interface into two Geliang Tang
` (2 preceding siblings ...)
2024-11-21 2:45 ` [PATCH mptcp-next 3/8] Squash to "selftests/bpf: Add bpf_bkup " Geliang Tang
@ 2024-11-21 2:45 ` Geliang Tang
2024-11-21 2:45 ` [PATCH mptcp-next 5/8] Squash to "selftests/bpf: Add bpf_red " Geliang Tang
` (4 subsequent siblings)
8 siblings, 0 replies; 13+ messages in thread
From: Geliang Tang @ 2024-11-21 2:45 UTC (permalink / raw)
To: mptcp; +Cc: baohua, Geliang Tang
From: Geliang Tang <tanggeliang@kylinos.cn>
Split get_subflow() interface into two: get_send() and get_retrans().
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
.../selftests/bpf/progs/mptcp_bpf_rr.c | 24 +++++++++++++++----
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_rr.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_rr.c
index 638ea6aa63b7..ec7853a9716c 100644
--- a/tools/testing/selftests/bpf/progs/mptcp_bpf_rr.c
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_rr.c
@@ -30,9 +30,8 @@ void BPF_PROG(mptcp_sched_rr_release, struct mptcp_sock *msk)
bpf_sk_storage_delete(&mptcp_rr_map, msk);
}
-SEC("struct_ops")
-int BPF_PROG(bpf_rr_get_subflow, struct mptcp_sock *msk,
- struct mptcp_sched_data *data)
+static int bpf_rr_get_subflow(struct mptcp_sock *msk,
+ struct mptcp_sched_data *data)
{
struct mptcp_subflow_context *subflow;
struct mptcp_rr_storage *ptr;
@@ -69,10 +68,25 @@ int BPF_PROG(bpf_rr_get_subflow, struct mptcp_sock *msk,
return 0;
}
-SEC(".struct_ops")
+SEC("struct_ops")
+int BPF_PROG(bpf_rr_get_send, struct mptcp_sock *msk,
+ struct mptcp_sched_data *data)
+{
+ return bpf_rr_get_subflow(msk, data);
+}
+
+SEC("struct_ops")
+int BPF_PROG(bpf_rr_get_retrans, struct mptcp_sock *msk,
+ struct mptcp_sched_data *data)
+{
+ return bpf_rr_get_subflow(msk, data);
+}
+
+SEC(".struct_ops.link")
struct mptcp_sched_ops rr = {
.init = (void *)mptcp_sched_rr_init,
.release = (void *)mptcp_sched_rr_release,
- .get_subflow = (void *)bpf_rr_get_subflow,
+ .get_send = (void *)bpf_rr_get_send,
+ .get_retrans = (void *)bpf_rr_get_retrans,
.name = "bpf_rr",
};
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH mptcp-next 5/8] Squash to "selftests/bpf: Add bpf_red scheduler & test"
2024-11-21 2:45 [PATCH mptcp-next 0/8] split get_subflow interface into two Geliang Tang
` (3 preceding siblings ...)
2024-11-21 2:45 ` [PATCH mptcp-next 4/8] Squash to "selftests/bpf: Add bpf_rr " Geliang Tang
@ 2024-11-21 2:45 ` Geliang Tang
2024-11-21 2:45 ` [PATCH mptcp-next 6/8] Squash to "selftests/bpf: Add bpf_burst " Geliang Tang
` (3 subsequent siblings)
8 siblings, 0 replies; 13+ messages in thread
From: Geliang Tang @ 2024-11-21 2:45 UTC (permalink / raw)
To: mptcp; +Cc: baohua, Geliang Tang
From: Geliang Tang <tanggeliang@kylinos.cn>
Split get_subflow() interface into two: get_send() and get_retrans().
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
.../selftests/bpf/progs/mptcp_bpf_red.c | 24 +++++++++++++++----
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_red.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_red.c
index cc0aab732fc4..acd0b03378f2 100644
--- a/tools/testing/selftests/bpf/progs/mptcp_bpf_red.c
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_red.c
@@ -16,9 +16,8 @@ void BPF_PROG(mptcp_sched_red_release, struct mptcp_sock *msk)
{
}
-SEC("struct_ops")
-int BPF_PROG(bpf_red_get_subflow, struct mptcp_sock *msk,
- struct mptcp_sched_data *data)
+static int bpf_red_get_subflow(struct mptcp_sock *msk,
+ struct mptcp_sched_data *data)
{
for (int i = 0; i < data->subflows && i < MPTCP_SUBFLOWS_MAX; i++) {
if (!bpf_mptcp_subflow_ctx_by_pos(data, i))
@@ -30,10 +29,25 @@ int BPF_PROG(bpf_red_get_subflow, struct mptcp_sock *msk,
return 0;
}
-SEC(".struct_ops")
+SEC("struct_ops")
+int BPF_PROG(bpf_red_get_send, struct mptcp_sock *msk,
+ struct mptcp_sched_data *data)
+{
+ return bpf_red_get_subflow(msk, data);
+}
+
+SEC("struct_ops")
+int BPF_PROG(bpf_red_get_retrans, struct mptcp_sock *msk,
+ struct mptcp_sched_data *data)
+{
+ return bpf_red_get_subflow(msk, data);
+}
+
+SEC(".struct_ops.link")
struct mptcp_sched_ops red = {
.init = (void *)mptcp_sched_red_init,
.release = (void *)mptcp_sched_red_release,
- .get_subflow = (void *)bpf_red_get_subflow,
+ .get_send = (void *)bpf_red_get_send,
+ .get_retrans = (void *)bpf_red_get_retrans,
.name = "bpf_red",
};
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH mptcp-next 6/8] Squash to "selftests/bpf: Add bpf_burst scheduler & test"
2024-11-21 2:45 [PATCH mptcp-next 0/8] split get_subflow interface into two Geliang Tang
` (4 preceding siblings ...)
2024-11-21 2:45 ` [PATCH mptcp-next 5/8] Squash to "selftests/bpf: Add bpf_red " Geliang Tang
@ 2024-11-21 2:45 ` Geliang Tang
2024-11-21 2:45 ` [PATCH mptcp-next 7/8] Squash to "bpf: Export mptcp packet scheduler helpers" Geliang Tang
` (2 subsequent siblings)
8 siblings, 0 replies; 13+ messages in thread
From: Geliang Tang @ 2024-11-21 2:45 UTC (permalink / raw)
To: mptcp; +Cc: baohua, Geliang Tang
From: Geliang Tang <tanggeliang@kylinos.cn>
Split get_subflow() interface into two: get_send() and get_retrans().
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
.../selftests/bpf/progs/mptcp_bpf_burst.c | 32 ++++++++-----------
1 file changed, 13 insertions(+), 19 deletions(-)
diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_burst.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_burst.c
index eb21119aa8f7..2785ccbc3807 100644
--- a/tools/testing/selftests/bpf/progs/mptcp_bpf_burst.c
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_burst.c
@@ -9,6 +9,10 @@ char _license[] SEC("license") = "GPL";
#define MPTCP_SEND_BURST_SIZE 65428
+#define SSK_MODE_ACTIVE 0
+#define SSK_MODE_BACKUP 1
+#define SSK_MODE_MAX 2
+
#define min(a, b) ((a) < (b) ? (a) : (b))
struct bpf_subflow_send_info {
@@ -23,10 +27,6 @@ extern bool tcp_stream_memory_free(const struct sock *sk, int wake) __ksym;
extern bool bpf_mptcp_subflow_queues_empty(struct sock *sk) __ksym;
extern void mptcp_pm_subflow_chk_stale(const struct mptcp_sock *msk, struct sock *ssk) __ksym;
-#define SSK_MODE_ACTIVE 0
-#define SSK_MODE_BACKUP 1
-#define SSK_MODE_MAX 2
-
static __always_inline __u64 div_u64(__u64 dividend, __u32 divisor)
{
return dividend / divisor;
@@ -67,8 +67,9 @@ void BPF_PROG(mptcp_sched_burst_release, struct mptcp_sock *msk)
{
}
-static int bpf_burst_get_send(struct mptcp_sock *msk,
- struct mptcp_sched_data *data)
+SEC("struct_ops")
+int BPF_PROG(bpf_burst_get_send, struct mptcp_sock *msk,
+ struct mptcp_sched_data *data)
{
struct bpf_subflow_send_info send_info[SSK_MODE_MAX];
struct mptcp_subflow_context *subflow;
@@ -141,8 +142,9 @@ static int bpf_burst_get_send(struct mptcp_sock *msk,
return 0;
}
-static int bpf_burst_get_retrans(struct mptcp_sock *msk,
- struct mptcp_sched_data *data)
+SEC("struct_ops")
+int BPF_PROG(bpf_burst_get_retrans, struct mptcp_sock *msk,
+ struct mptcp_sched_data *data)
{
int backup = MPTCP_SUBFLOWS_MAX, pick = MPTCP_SUBFLOWS_MAX, subflow_id;
struct mptcp_subflow_context *subflow;
@@ -189,19 +191,11 @@ static int bpf_burst_get_retrans(struct mptcp_sock *msk,
return 0;
}
-SEC("struct_ops")
-int BPF_PROG(bpf_burst_get_subflow, struct mptcp_sock *msk,
- struct mptcp_sched_data *data)
-{
- if (data->reinject)
- return bpf_burst_get_retrans(msk, data);
- return bpf_burst_get_send(msk, data);
-}
-
-SEC(".struct_ops")
+SEC(".struct_ops.link")
struct mptcp_sched_ops burst = {
.init = (void *)mptcp_sched_burst_init,
.release = (void *)mptcp_sched_burst_release,
- .get_subflow = (void *)bpf_burst_get_subflow,
+ .get_send = (void *)bpf_burst_get_send,
+ .get_retrans = (void *)bpf_burst_get_retrans,
.name = "bpf_burst",
};
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH mptcp-next 7/8] Squash to "bpf: Export mptcp packet scheduler helpers"
2024-11-21 2:45 [PATCH mptcp-next 0/8] split get_subflow interface into two Geliang Tang
` (5 preceding siblings ...)
2024-11-21 2:45 ` [PATCH mptcp-next 6/8] Squash to "selftests/bpf: Add bpf_burst " Geliang Tang
@ 2024-11-21 2:45 ` Geliang Tang
2024-11-21 2:45 ` [PATCH mptcp-next 8/8] Squash to "selftests/bpf: Add bpf_burst scheduler & test" Geliang Tang
2024-11-21 3:55 ` [PATCH mptcp-next 0/8] split get_subflow interface into two MPTCP CI
8 siblings, 0 replies; 13+ messages in thread
From: Geliang Tang @ 2024-11-21 2:45 UTC (permalink / raw)
To: mptcp; +Cc: baohua, Geliang Tang
From: Geliang Tang <tanggeliang@kylinos.cn>
Set KF_SLEEPABLE flag for mptcp_pm_subflow_chk_stale.
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
net/mptcp/bpf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
index ab1de189ba4b..a6cf4d1a48a9 100644
--- a/net/mptcp/bpf.c
+++ b/net/mptcp/bpf.c
@@ -329,7 +329,7 @@ BTF_ID_FLAGS(func, mptcp_set_timeout)
BTF_ID_FLAGS(func, mptcp_wnd_end)
BTF_ID_FLAGS(func, tcp_stream_memory_free)
BTF_ID_FLAGS(func, bpf_mptcp_subflow_queues_empty)
-BTF_ID_FLAGS(func, mptcp_pm_subflow_chk_stale)
+BTF_ID_FLAGS(func, mptcp_pm_subflow_chk_stale, KF_SLEEPABLE)
BTF_KFUNCS_END(bpf_mptcp_sched_kfunc_ids)
static const struct btf_kfunc_id_set bpf_mptcp_sched_kfunc_set = {
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH mptcp-next 8/8] Squash to "selftests/bpf: Add bpf_burst scheduler & test"
2024-11-21 2:45 [PATCH mptcp-next 0/8] split get_subflow interface into two Geliang Tang
` (6 preceding siblings ...)
2024-11-21 2:45 ` [PATCH mptcp-next 7/8] Squash to "bpf: Export mptcp packet scheduler helpers" Geliang Tang
@ 2024-11-21 2:45 ` Geliang Tang
2024-11-21 3:55 ` [PATCH mptcp-next 0/8] split get_subflow interface into two MPTCP CI
8 siblings, 0 replies; 13+ messages in thread
From: Geliang Tang @ 2024-11-21 2:45 UTC (permalink / raw)
To: mptcp; +Cc: baohua, Geliang Tang
From: Geliang Tang <tanggeliang@kylinos.cn>
Set BPF_F_SLEEPABLE flags for bpf_burst_get_retrans.
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
tools/testing/selftests/bpf/prog_tests/mptcp.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index 1d68bbda3370..d7a640dce7da 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -726,12 +726,22 @@ static void test_red(void)
static void test_burst(void)
{
struct mptcp_bpf_burst *skel;
+ int err;
- skel = mptcp_bpf_burst__open_and_load();
- if (!ASSERT_OK_PTR(skel, "open_and_load: burst"))
+ skel = mptcp_bpf_burst__open();
+ if (!ASSERT_OK_PTR(skel, "open: burst"))
return;
+ err = bpf_program__set_flags(skel->progs.bpf_burst_get_retrans,
+ BPF_F_SLEEPABLE);
+ if (!ASSERT_OK(err, "set sleepable flags"))
+ goto skel_destroy;
+
+ if (!ASSERT_OK(mptcp_bpf_burst__load(skel), "load: burst"))
+ goto skel_destroy;
+
test_bpf_sched(skel->obj, "burst", WITH_DATA, WITH_DATA);
+skel_destroy:
mptcp_bpf_burst__destroy(skel);
}
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH mptcp-next 0/8] split get_subflow interface into two
2024-11-21 2:45 [PATCH mptcp-next 0/8] split get_subflow interface into two Geliang Tang
` (7 preceding siblings ...)
2024-11-21 2:45 ` [PATCH mptcp-next 8/8] Squash to "selftests/bpf: Add bpf_burst scheduler & test" Geliang Tang
@ 2024-11-21 3:55 ` MPTCP CI
8 siblings, 0 replies; 13+ messages in thread
From: MPTCP CI @ 2024-11-21 3:55 UTC (permalink / raw)
To: Geliang Tang; +Cc: mptcp
Hi Geliang,
Thank you for your modifications, that's great!
Our CI did some validations and here is its report:
- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/11945771301
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/86555cf65164
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=911397
If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:
$ cd [kernel source code]
$ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
--pull always mptcp/mptcp-upstream-virtme-docker:latest \
auto-normal
For more details:
https://github.com/multipath-tcp/mptcp-upstream-virtme-docker
Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)
Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH mptcp-next 1/8] mptcp: split get_subflow interface into two
2024-11-21 2:45 ` [PATCH mptcp-next 1/8] mptcp: " Geliang Tang
@ 2024-12-21 1:23 ` Mat Martineau
0 siblings, 0 replies; 13+ messages in thread
From: Mat Martineau @ 2024-12-21 1:23 UTC (permalink / raw)
To: Geliang Tang; +Cc: mptcp, baohua, Geliang Tang
On Thu, 21 Nov 2024, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
>
> get_retrans() interface of the burst packet scheduler invokes a sleeping
> function mptcp_pm_subflow_chk_stale(), which calls __lock_sock_fast().
> So get_retrans() interface should be set with BPF_F_SLEEPABLE flag in
> BPF. But get_send() interface of this scheduler can't be set with
> BPF_F_SLEEPABLE flag since it's invoked in ack_update_msk() under mptcp
> data lock.
>
> So this patch has to split get_subflow() interface of packet scheduer into
> two interfaces: get_send() and get_retrans(). Then we can set get_retrans()
> interface alone with BPF_F_SLEEPABLE flag.
>
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
> include/net/mptcp.h | 5 +++--
> net/mptcp/bpf.c | 11 +++++++++--
> net/mptcp/sched.c | 33 ++++++++++++++++++++++-----------
> 3 files changed, 34 insertions(+), 15 deletions(-)
>
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 814b5f2e3ed5..2c85ca92bb1c 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -103,13 +103,14 @@ struct mptcp_out_options {
> #define MPTCP_SUBFLOWS_MAX 8
>
> struct mptcp_sched_data {
> - bool reinject;
> u8 subflows;
> struct mptcp_subflow_context *contexts[MPTCP_SUBFLOWS_MAX];
> };
>
> struct mptcp_sched_ops {
> - int (*get_subflow)(struct mptcp_sock *msk,
> + int (*get_send)(struct mptcp_sock *msk,
> + struct mptcp_sched_data *data);
> + int (*get_retrans)(struct mptcp_sock *msk,
> struct mptcp_sched_data *data);
>
> char name[MPTCP_SCHED_NAME_MAX];
> diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
> index e9db856972cb..ab1de189ba4b 100644
> --- a/net/mptcp/bpf.c
> +++ b/net/mptcp/bpf.c
> @@ -156,7 +156,13 @@ static int bpf_mptcp_sched_init(struct btf *btf)
> return 0;
> }
>
> -static int __bpf_mptcp_sched_get_subflow(struct mptcp_sock *msk,
> +static int __bpf_mptcp_sched_get_send(struct mptcp_sock *msk,
> + struct mptcp_sched_data *data)
> +{
> + return 0;
> +}
> +
> +static int __bpf_mptcp_sched_get_retrans(struct mptcp_sock *msk,
> struct mptcp_sched_data *data)
> {
> return 0;
> @@ -171,7 +177,8 @@ static void __bpf_mptcp_sched_release(struct mptcp_sock *msk)
> }
>
> static struct mptcp_sched_ops __bpf_mptcp_sched_ops = {
> - .get_subflow = __bpf_mptcp_sched_get_subflow,
> + .get_send = __bpf_mptcp_sched_get_send,
> + .get_retrans = __bpf_mptcp_sched_get_retrans,
> .init = __bpf_mptcp_sched_init,
> .release = __bpf_mptcp_sched_release,
> };
> diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c
> index 6da60e35932f..7560693e5b7a 100644
> --- a/net/mptcp/sched.c
> +++ b/net/mptcp/sched.c
> @@ -16,13 +16,25 @@
> static DEFINE_SPINLOCK(mptcp_sched_list_lock);
> static LIST_HEAD(mptcp_sched_list);
>
> -static int mptcp_sched_default_get_subflow(struct mptcp_sock *msk,
> +static int mptcp_sched_default_get_send(struct mptcp_sock *msk,
> + struct mptcp_sched_data *data)
> +{
> + struct sock *ssk;
> +
> + ssk = mptcp_subflow_get_send(msk);
> + if (!ssk)
> + return -EINVAL;
> +
> + mptcp_subflow_set_scheduled(mptcp_subflow_ctx(ssk), true);
> + return 0;
> +}
> +
> +static int mptcp_sched_default_get_retrans(struct mptcp_sock *msk,
> struct mptcp_sched_data *data)
> {
> struct sock *ssk;
>
> - ssk = data->reinject ? mptcp_subflow_get_retrans(msk) :
> - mptcp_subflow_get_send(msk);
> + ssk = mptcp_subflow_get_retrans(msk);
> if (!ssk)
> return -EINVAL;
>
> @@ -31,7 +43,8 @@ static int mptcp_sched_default_get_subflow(struct mptcp_sock *msk,
> }
>
> static struct mptcp_sched_ops mptcp_sched_default = {
> - .get_subflow = mptcp_sched_default_get_subflow,
> + .get_send = mptcp_sched_default_get_send,
> + .get_retrans = mptcp_sched_default_get_retrans,
> .name = "default",
> .owner = THIS_MODULE,
> };
> @@ -73,7 +86,7 @@ void mptcp_get_available_schedulers(char *buf, size_t maxlen)
>
> int mptcp_register_scheduler(struct mptcp_sched_ops *sched)
> {
> - if (!sched->get_subflow)
> + if (!sched->get_send || !sched->get_retrans)
Hi Geliang -
Looking at patches 2-5, they all implement identical code for get_send and
get_retrans. It might be helpful for simpler schedulers to not have to
define two handlers if they do the same thing. My suggestion is to only
require get_send, and if get_retrans is NULL in mptcp_sched_ops then use
get_send for retransmits too.
> return -EINVAL;
>
> spin_lock(&mptcp_sched_list_lock);
> @@ -184,11 +197,10 @@ int mptcp_sched_get_send(struct mptcp_sock *msk)
> return 0;
> }
>
> - data.reinject = false;
> if (msk->sched == &mptcp_sched_default || !msk->sched)
> - return mptcp_sched_default_get_subflow(msk, &data);
> + return mptcp_sched_default_get_send(msk, &data);
> mptcp_sched_data_set_contexts(msk, &data);
> - return msk->sched->get_subflow(msk, &data);
> + return msk->sched->get_send(msk, &data);
> }
>
> int mptcp_sched_get_retrans(struct mptcp_sock *msk)
> @@ -207,9 +219,8 @@ int mptcp_sched_get_retrans(struct mptcp_sock *msk)
> return 0;
> }
>
> - data.reinject = true;
> if (msk->sched == &mptcp_sched_default || !msk->sched)
> - return mptcp_sched_default_get_subflow(msk, &data);
> + return mptcp_sched_default_get_retrans(msk, &data);
> mptcp_sched_data_set_contexts(msk, &data);
> - return msk->sched->get_subflow(msk, &data);
> + return msk->sched->get_retrans(msk, &data);
As I mentioned above, this could revert to get_send() if get_retrans is
NULL.
- Mat
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH mptcp-next 2/8] Squash to "selftests/bpf: Add bpf_first scheduler & test"
2024-11-21 2:45 ` [PATCH mptcp-next 2/8] Squash to "selftests/bpf: Add bpf_first scheduler & test" Geliang Tang
@ 2024-12-21 1:28 ` Mat Martineau
2024-12-23 9:49 ` Geliang Tang
0 siblings, 1 reply; 13+ messages in thread
From: Mat Martineau @ 2024-12-21 1:28 UTC (permalink / raw)
To: Geliang Tang; +Cc: mptcp, baohua, Geliang Tang
On Thu, 21 Nov 2024, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
>
> Split get_subflow() interface into two: get_send() and get_retrans().
>
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
> .../selftests/bpf/progs/mptcp_bpf_first.c | 24 +++++++++++++++----
> 1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_first.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_first.c
> index d57399b407a7..36bf9adffb2c 100644
> --- a/tools/testing/selftests/bpf/progs/mptcp_bpf_first.c
> +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_first.c
> @@ -16,18 +16,32 @@ void BPF_PROG(mptcp_sched_first_release, struct mptcp_sock *msk)
> {
> }
>
> -SEC("struct_ops")
> -int BPF_PROG(bpf_first_get_subflow, struct mptcp_sock *msk,
> - struct mptcp_sched_data *data)
> +static int bpf_first_get_subflow(struct mptcp_sock *msk,
> + struct mptcp_sched_data *data)
> {
> mptcp_subflow_set_scheduled(bpf_mptcp_subflow_ctx_by_pos(data, 0), true);
> return 0;
> }
>
> -SEC(".struct_ops")
> +SEC("struct_ops")
> +int BPF_PROG(bpf_first_get_send, struct mptcp_sock *msk,
> + struct mptcp_sched_data *data)
> +{
> + return bpf_first_get_subflow(msk, data);
> +}
> +
> +SEC("struct_ops")
> +int BPF_PROG(bpf_first_get_retrans, struct mptcp_sock *msk,
> + struct mptcp_sched_data *data)
> +{
> + return bpf_first_get_subflow(msk, data);
> +}
Hi Geliang -
If my suggestion in patch 1 (use get_send for retransmit scheduling if
get_retrans is NULL) doesn't seem like clear enough behavior for BPF
scheduler authors, this code and the following patches could be simplified
by removing the duplicate _retrans function above...
> +
> +SEC(".struct_ops.link")
> struct mptcp_sched_ops first = {
> .init = (void *)mptcp_sched_first_init,
> .release = (void *)mptcp_sched_first_release,
> - .get_subflow = (void *)bpf_first_get_subflow,
> + .get_send = (void *)bpf_first_get_send,
> + .get_retrans = (void *)bpf_first_get_retrans,
... and setting get_retrans like this:
.get_retrans = (void *)bpf_first_get_send,
Then there would be example code for BPF scheduler programmers to see that
it's ok to use the same function pointer in both fields.
- Mat
> .name = "bpf_first",
> };
> --
> 2.43.0
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH mptcp-next 2/8] Squash to "selftests/bpf: Add bpf_first scheduler & test"
2024-12-21 1:28 ` Mat Martineau
@ 2024-12-23 9:49 ` Geliang Tang
0 siblings, 0 replies; 13+ messages in thread
From: Geliang Tang @ 2024-12-23 9:49 UTC (permalink / raw)
To: Mat Martineau; +Cc: mptcp, baohua, Geliang Tang
Hi Mat,
Thanks for your review.
On Fri, 2024-12-20 at 17:28 -0800, Mat Martineau wrote:
> On Thu, 21 Nov 2024, Geliang Tang wrote:
>
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> >
> > Split get_subflow() interface into two: get_send() and
> > get_retrans().
> >
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> > .../selftests/bpf/progs/mptcp_bpf_first.c | 24 +++++++++++++++-
> > ---
> > 1 file changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_first.c
> > b/tools/testing/selftests/bpf/progs/mptcp_bpf_first.c
> > index d57399b407a7..36bf9adffb2c 100644
> > --- a/tools/testing/selftests/bpf/progs/mptcp_bpf_first.c
> > +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_first.c
> > @@ -16,18 +16,32 @@ void BPF_PROG(mptcp_sched_first_release, struct
> > mptcp_sock *msk)
> > {
> > }
> >
> > -SEC("struct_ops")
> > -int BPF_PROG(bpf_first_get_subflow, struct mptcp_sock *msk,
> > - struct mptcp_sched_data *data)
> > +static int bpf_first_get_subflow(struct mptcp_sock *msk,
> > + struct mptcp_sched_data *data)
> > {
> > mptcp_subflow_set_scheduled(bpf_mptcp_subflow_ctx_by_pos(d
> > ata, 0), true);
> > return 0;
> > }
> >
> > -SEC(".struct_ops")
> > +SEC("struct_ops")
> > +int BPF_PROG(bpf_first_get_send, struct mptcp_sock *msk,
> > + struct mptcp_sched_data *data)
> > +{
> > + return bpf_first_get_subflow(msk, data);
> > +}
> > +
> > +SEC("struct_ops")
> > +int BPF_PROG(bpf_first_get_retrans, struct mptcp_sock *msk,
> > + struct mptcp_sched_data *data)
> > +{
> > + return bpf_first_get_subflow(msk, data);
> > +}
>
> Hi Geliang -
>
> If my suggestion in patch 1 (use get_send for retransmit scheduling
> if
> get_retrans is NULL) doesn't seem like clear enough behavior for BPF
The suggestion in patch 1 works well, I updated it in v2.
>
> scheduler authors, this code and the following patches could be
> simplified
> by removing the duplicate _retrans function above...
>
> > +
> > +SEC(".struct_ops.link")
> > struct mptcp_sched_ops first = {
> > .init = (void *)mptcp_sched_first_init,
> > .release = (void *)mptcp_sched_first_release,
> > - .get_subflow = (void *)bpf_first_get_subflow,
> > + .get_send = (void *)bpf_first_get_send,
> > + .get_retrans = (void *)bpf_first_get_retrans,
>
> ... and setting get_retrans like this:
> .get_retrans = (void *)bpf_first_get_send,
If we do this, libbpf will report the following error:
# libbpf: struct_ops init_kern first func ptr get_retrans: invalid
reuse of prog bpf_first_get_send in sec struct_ops with type 27:
expected_attach_type 0 != kern_member_idx 1
# libbpf: failed to load object 'mptcp_bpf_first'
# libbpf: failed to load BPF skeleton 'mptcp_bpf_first': -EINVAL
# test_first:FAIL:open_and_load: first unexpected error: -22
So in v2, I didn't set this get_retrans ptr, but let it be NULL.
Thanks,
-Geliang
>
> Then there would be example code for BPF scheduler programmers to see
> that
> it's ok to use the same function pointer in both fields.
>
> - Mat
>
> > .name = "bpf_first",
> > };
> > --
> > 2.43.0
> >
> >
> >
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2024-12-23 9:49 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-21 2:45 [PATCH mptcp-next 0/8] split get_subflow interface into two Geliang Tang
2024-11-21 2:45 ` [PATCH mptcp-next 1/8] mptcp: " Geliang Tang
2024-12-21 1:23 ` Mat Martineau
2024-11-21 2:45 ` [PATCH mptcp-next 2/8] Squash to "selftests/bpf: Add bpf_first scheduler & test" Geliang Tang
2024-12-21 1:28 ` Mat Martineau
2024-12-23 9:49 ` Geliang Tang
2024-11-21 2:45 ` [PATCH mptcp-next 3/8] Squash to "selftests/bpf: Add bpf_bkup " Geliang Tang
2024-11-21 2:45 ` [PATCH mptcp-next 4/8] Squash to "selftests/bpf: Add bpf_rr " Geliang Tang
2024-11-21 2:45 ` [PATCH mptcp-next 5/8] Squash to "selftests/bpf: Add bpf_red " Geliang Tang
2024-11-21 2:45 ` [PATCH mptcp-next 6/8] Squash to "selftests/bpf: Add bpf_burst " Geliang Tang
2024-11-21 2:45 ` [PATCH mptcp-next 7/8] Squash to "bpf: Export mptcp packet scheduler helpers" Geliang Tang
2024-11-21 2:45 ` [PATCH mptcp-next 8/8] Squash to "selftests/bpf: Add bpf_burst scheduler & test" Geliang Tang
2024-11-21 3:55 ` [PATCH mptcp-next 0/8] split get_subflow interface into two MPTCP CI
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox