* [PATCH mptcp-next 0/2] BPF redundant scheduler, part 3 @ 2022-12-05 12:35 Geliang Tang 2022-12-05 12:35 ` [PATCH mptcp-next 1/2] Squash to "mptcp: use get_send wrapper, v22" Geliang Tang 2022-12-05 12:35 ` [PATCH mptcp-next 2/2] mptcp: retrans for redundant sends Geliang Tang 0 siblings, 2 replies; 5+ messages in thread From: Geliang Tang @ 2022-12-05 12:35 UTC (permalink / raw) To: mptcp; +Cc: Geliang Tang - The DSS issue has been fixed in this version, and all tests (mptcp_connect.sh, mptcp_join.sh, simult_flows.sh and BPF test_progs) passed. - No need to set already_sent to 0, drop this. - Add retrans_redundant flag. - depends on "BPF redundant scheduler, part 2" v22. Geliang Tang (2): Squash to "mptcp: use get_send wrapper, v22" mptcp: retrans for redundant sends net/mptcp/protocol.c | 45 ++++++++++++++++++++++++++++++++++++++++---- net/mptcp/protocol.h | 1 + 2 files changed, 42 insertions(+), 4 deletions(-) -- 2.35.3 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH mptcp-next 1/2] Squash to "mptcp: use get_send wrapper, v22" 2022-12-05 12:35 [PATCH mptcp-next 0/2] BPF redundant scheduler, part 3 Geliang Tang @ 2022-12-05 12:35 ` Geliang Tang 2022-12-06 1:22 ` Mat Martineau 2022-12-05 12:35 ` [PATCH mptcp-next 2/2] mptcp: retrans for redundant sends Geliang Tang 1 sibling, 1 reply; 5+ messages in thread From: Geliang Tang @ 2022-12-05 12:35 UTC (permalink / raw) To: mptcp; +Cc: Geliang Tang Cleanup. Signed-off-by: Geliang Tang <geliang.tang@suse.com> --- net/mptcp/protocol.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index 712b183fd80e..ea2e48a74772 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -1565,7 +1565,7 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags) int ret = 0; if (mptcp_sched_get_send(msk)) - break; + goto out; push_count = 0; @@ -1605,6 +1605,7 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags) } } +out: /* at this point we held the socket lock for the last subflow we used */ if (ssk) mptcp_push_release(ssk, &info); -- 2.35.3 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH mptcp-next 1/2] Squash to "mptcp: use get_send wrapper, v22" 2022-12-05 12:35 ` [PATCH mptcp-next 1/2] Squash to "mptcp: use get_send wrapper, v22" Geliang Tang @ 2022-12-06 1:22 ` Mat Martineau 0 siblings, 0 replies; 5+ messages in thread From: Mat Martineau @ 2022-12-06 1:22 UTC (permalink / raw) To: Geliang Tang; +Cc: mptcp On Mon, 5 Dec 2022, Geliang Tang wrote: > Cleanup. > > Signed-off-by: Geliang Tang <geliang.tang@suse.com> > --- > net/mptcp/protocol.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > index 712b183fd80e..ea2e48a74772 100644 > --- a/net/mptcp/protocol.c > +++ b/net/mptcp/protocol.c > @@ -1565,7 +1565,7 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags) > int ret = 0; > > if (mptcp_sched_get_send(msk)) > - break; > + goto out; I prefer the original code here. It looks like you were making this code look more similar to __mptcp_subflow_push_pending(), but I'm not sure that the changes to __mptcp_push_pending() in patch 2 are needed. (See my reply to patch 2 for more details) > > push_count = 0; > > @@ -1605,6 +1605,7 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags) > } > } > > +out: > /* at this point we held the socket lock for the last subflow we used */ > if (ssk) > mptcp_push_release(ssk, &info); > -- > 2.35.3 > > > -- Mat Martineau Intel ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH mptcp-next 2/2] mptcp: retrans for redundant sends 2022-12-05 12:35 [PATCH mptcp-next 0/2] BPF redundant scheduler, part 3 Geliang Tang 2022-12-05 12:35 ` [PATCH mptcp-next 1/2] Squash to "mptcp: use get_send wrapper, v22" Geliang Tang @ 2022-12-05 12:35 ` Geliang Tang 2022-12-06 1:37 ` Mat Martineau 1 sibling, 1 reply; 5+ messages in thread From: Geliang Tang @ 2022-12-05 12:35 UTC (permalink / raw) To: mptcp; +Cc: Geliang Tang Redundant sends need to work more like the MPTCP retransmit code path. When the scheduler selects multiple subflows, the first subflow to send is a "normal" transmit, and any other subflows would act like a retransmit when accessing the dfrags. Signed-off-by: Geliang Tang <geliang.tang@suse.com> --- net/mptcp/protocol.c | 42 +++++++++++++++++++++++++++++++++++++++--- net/mptcp/protocol.h | 1 + 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index ea2e48a74772..545b29c0e01c 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -997,6 +997,8 @@ static void __mptcp_clean_una(struct sock *sk) break; if (unlikely(dfrag == msk->first_pending)) { + if (msk->retrans_redundant) + break; /* in recovery mode can see ack after the current snd head */ if (WARN_ON_ONCE(!msk->recovery)) break; @@ -1013,6 +1015,8 @@ static void __mptcp_clean_una(struct sock *sk) /* prevent wrap around in recovery mode */ if (unlikely(delta > dfrag->already_sent)) { + if (msk->retrans_redundant) + goto out; if (WARN_ON_ONCE(!msk->recovery)) goto out; if (WARN_ON_ONCE(delta > dfrag->data_len)) @@ -1473,7 +1477,8 @@ struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk) static void mptcp_push_release(struct sock *ssk, struct mptcp_sendmsg_info *info) { - tcp_push(ssk, 0, info->mss_now, tcp_sk(ssk)->nonagle, info->size_goal); + if (info->mss_now) + tcp_push(ssk, 0, info->mss_now, tcp_sk(ssk)->nonagle, info->size_goal); release_sock(ssk); } @@ -1555,14 +1560,19 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags) struct sock *prev_ssk = NULL, *ssk = NULL; struct mptcp_sock *msk = mptcp_sk(sk); struct mptcp_subflow_context *subflow; + struct mptcp_data_frag *head = NULL; struct mptcp_sendmsg_info info = { .flags = flags, }; bool do_check_data_fin = false; int push_count = 1; + head = mptcp_send_head(sk); + if (!head) + goto out; + while (mptcp_send_head(sk) && (push_count > 0)) { - int ret = 0; + int ret = 0, i = 0; if (mptcp_sched_get_send(msk)) goto out; @@ -1571,6 +1581,14 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags) mptcp_for_each_subflow(msk, subflow) { if (READ_ONCE(subflow->scheduled)) { + if (i > 0) { + if (!test_and_set_bit(MPTCP_WORK_RTX, &msk->flags)) + mptcp_schedule_work(sk); + WRITE_ONCE(msk->first_pending, head); + msk->retrans_redundant = true; + goto out; + } + prev_ssk = ssk; ssk = mptcp_subflow_tcp_sock(subflow); @@ -1598,6 +1616,7 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags) push_count--; continue; } + i++; do_check_data_fin = true; msk->last_snd = ssk; mptcp_subflow_set_scheduled(subflow, false); @@ -1621,16 +1640,21 @@ static void __mptcp_subflow_push_pending(struct sock *sk, struct sock *ssk, bool { struct mptcp_sock *msk = mptcp_sk(sk); struct mptcp_subflow_context *subflow; + struct mptcp_data_frag *head = NULL; struct mptcp_sendmsg_info info = { .data_lock_held = true, }; bool push = true; int copied = 0; + head = mptcp_send_head(sk); + if (!head) + goto out; + info.flags = 0; while (mptcp_send_head(sk) && push) { bool delegate = false; - int ret = 0; + int ret = 0, i = 0; /* check for a different subflow usage only after * spooling the first chunk of data @@ -1652,6 +1676,14 @@ static void __mptcp_subflow_push_pending(struct sock *sk, struct sock *ssk, bool if (READ_ONCE(subflow->scheduled)) { struct sock *xmit_ssk = mptcp_subflow_tcp_sock(subflow); + if (i > 0) { + if (!test_and_set_bit(MPTCP_WORK_RTX, &msk->flags)) + mptcp_schedule_work(sk); + WRITE_ONCE(msk->first_pending, head); + msk->retrans_redundant = true; + goto out; + } + if (xmit_ssk != ssk) { /* Only delegate to one subflow recently, * TODO: chain delegated calls for more subflows. @@ -1660,6 +1692,7 @@ static void __mptcp_subflow_push_pending(struct sock *sk, struct sock *ssk, bool goto out; mptcp_subflow_delegate(subflow, MPTCP_DELEGATE_SEND); + i++; msk->last_snd = ssk; mptcp_subflow_set_scheduled(subflow, false); delegate = true; @@ -1672,6 +1705,7 @@ static void __mptcp_subflow_push_pending(struct sock *sk, struct sock *ssk, bool push = false; continue; } + i++; copied += ret; msk->last_snd = ssk; mptcp_subflow_set_scheduled(subflow, false); @@ -2593,6 +2627,7 @@ static void __mptcp_retrans(struct sock *sk) } } dfrag->already_sent = max(dfrag->already_sent, len); + msk->retrans_redundant = false; reset_timer: mptcp_check_and_set_pending(sk); @@ -2724,6 +2759,7 @@ static int __mptcp_init_sock(struct sock *sk) WRITE_ONCE(msk->csum_enabled, mptcp_is_checksum_enabled(sock_net(sk))); WRITE_ONCE(msk->allow_infinite_fallback, true); msk->recovery = false; + msk->retrans_redundant = false; mptcp_pm_data_init(msk); diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index adfb758a842f..e6bebde2f3ab 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -290,6 +290,7 @@ struct mptcp_sock { bool use_64bit_ack; /* Set when we received a 64-bit DSN */ bool csum_enabled; bool allow_infinite_fallback; + bool retrans_redundant; u8 mpc_endpoint_id; u8 recvmsg_inq:1, cork:1, -- 2.35.3 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH mptcp-next 2/2] mptcp: retrans for redundant sends 2022-12-05 12:35 ` [PATCH mptcp-next 2/2] mptcp: retrans for redundant sends Geliang Tang @ 2022-12-06 1:37 ` Mat Martineau 0 siblings, 0 replies; 5+ messages in thread From: Mat Martineau @ 2022-12-06 1:37 UTC (permalink / raw) To: Geliang Tang; +Cc: mptcp On Mon, 5 Dec 2022, Geliang Tang wrote: > Redundant sends need to work more like the MPTCP retransmit code path. > When the scheduler selects multiple subflows, the first subflow to send > is a "normal" transmit, and any other subflows would act like a retransmit > when accessing the dfrags. > > Signed-off-by: Geliang Tang <geliang.tang@suse.com> > --- > net/mptcp/protocol.c | 42 +++++++++++++++++++++++++++++++++++++++--- > net/mptcp/protocol.h | 1 + > 2 files changed, 40 insertions(+), 3 deletions(-) > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > index ea2e48a74772..545b29c0e01c 100644 > --- a/net/mptcp/protocol.c > +++ b/net/mptcp/protocol.c > @@ -997,6 +997,8 @@ static void __mptcp_clean_una(struct sock *sk) > break; > > if (unlikely(dfrag == msk->first_pending)) { > + if (msk->retrans_redundant) > + break; > /* in recovery mode can see ack after the current snd head */ > if (WARN_ON_ONCE(!msk->recovery)) > break; > @@ -1013,6 +1015,8 @@ static void __mptcp_clean_una(struct sock *sk) > > /* prevent wrap around in recovery mode */ > if (unlikely(delta > dfrag->already_sent)) { > + if (msk->retrans_redundant) > + goto out; > if (WARN_ON_ONCE(!msk->recovery)) > goto out; > if (WARN_ON_ONCE(delta > dfrag->data_len)) > @@ -1473,7 +1477,8 @@ struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk) > > static void mptcp_push_release(struct sock *ssk, struct mptcp_sendmsg_info *info) > { > - tcp_push(ssk, 0, info->mss_now, tcp_sk(ssk)->nonagle, info->size_goal); > + if (info->mss_now) > + tcp_push(ssk, 0, info->mss_now, tcp_sk(ssk)->nonagle, info->size_goal); > release_sock(ssk); > } > > @@ -1555,14 +1560,19 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags) > struct sock *prev_ssk = NULL, *ssk = NULL; > struct mptcp_sock *msk = mptcp_sk(sk); > struct mptcp_subflow_context *subflow; > + struct mptcp_data_frag *head = NULL; > struct mptcp_sendmsg_info info = { > .flags = flags, > }; > bool do_check_data_fin = false; > int push_count = 1; > > + head = mptcp_send_head(sk); > + if (!head) > + goto out; > + > while (mptcp_send_head(sk) && (push_count > 0)) { > - int ret = 0; > + int ret = 0, i = 0; > > if (mptcp_sched_get_send(msk)) > goto out; > @@ -1571,6 +1581,14 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags) > > mptcp_for_each_subflow(msk, subflow) { > if (READ_ONCE(subflow->scheduled)) { > + if (i > 0) { > + if (!test_and_set_bit(MPTCP_WORK_RTX, &msk->flags)) > + mptcp_schedule_work(sk); > + WRITE_ONCE(msk->first_pending, head); > + msk->retrans_redundant = true; > + goto out; > + } > + Hi Geliang - On this code path, the msk lock is not released between subflow sends, so I don't think it's necessary to use the retrans_redundant approach for __mptcp_push_pending(). Did you find that the retrans_redundant path was needed in both __mptcp_push_pending() and __mptcp_subflow_push_pending()? > prev_ssk = ssk; > ssk = mptcp_subflow_tcp_sock(subflow); > > @@ -1598,6 +1616,7 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags) > push_count--; > continue; > } > + i++; > do_check_data_fin = true; > msk->last_snd = ssk; > mptcp_subflow_set_scheduled(subflow, false); > @@ -1621,16 +1640,21 @@ static void __mptcp_subflow_push_pending(struct sock *sk, struct sock *ssk, bool > { > struct mptcp_sock *msk = mptcp_sk(sk); > struct mptcp_subflow_context *subflow; > + struct mptcp_data_frag *head = NULL; > struct mptcp_sendmsg_info info = { > .data_lock_held = true, > }; > bool push = true; > int copied = 0; > > + head = mptcp_send_head(sk); > + if (!head) > + goto out; > + > info.flags = 0; > while (mptcp_send_head(sk) && push) { > bool delegate = false; > - int ret = 0; > + int ret = 0, i = 0; > > /* check for a different subflow usage only after > * spooling the first chunk of data > @@ -1652,6 +1676,14 @@ static void __mptcp_subflow_push_pending(struct sock *sk, struct sock *ssk, bool > if (READ_ONCE(subflow->scheduled)) { > struct sock *xmit_ssk = mptcp_subflow_tcp_sock(subflow); > > + if (i > 0) { > + if (!test_and_set_bit(MPTCP_WORK_RTX, &msk->flags)) > + mptcp_schedule_work(sk); > + WRITE_ONCE(msk->first_pending, head); > + msk->retrans_redundant = true; > + goto out; > + } > + I need to look at this in some more detail, but my initial impression is that redundant sends will need to avoid the worker thread and instead add calls to the retrans code through MPTCP_DELEGATE_SEND. - Mat > if (xmit_ssk != ssk) { > /* Only delegate to one subflow recently, > * TODO: chain delegated calls for more subflows. > @@ -1660,6 +1692,7 @@ static void __mptcp_subflow_push_pending(struct sock *sk, struct sock *ssk, bool > goto out; > mptcp_subflow_delegate(subflow, > MPTCP_DELEGATE_SEND); > + i++; > msk->last_snd = ssk; > mptcp_subflow_set_scheduled(subflow, false); > delegate = true; > @@ -1672,6 +1705,7 @@ static void __mptcp_subflow_push_pending(struct sock *sk, struct sock *ssk, bool > push = false; > continue; > } > + i++; > copied += ret; > msk->last_snd = ssk; > mptcp_subflow_set_scheduled(subflow, false); > @@ -2593,6 +2627,7 @@ static void __mptcp_retrans(struct sock *sk) > } > } > dfrag->already_sent = max(dfrag->already_sent, len); > + msk->retrans_redundant = false; > > reset_timer: > mptcp_check_and_set_pending(sk); > @@ -2724,6 +2759,7 @@ static int __mptcp_init_sock(struct sock *sk) > WRITE_ONCE(msk->csum_enabled, mptcp_is_checksum_enabled(sock_net(sk))); > WRITE_ONCE(msk->allow_infinite_fallback, true); > msk->recovery = false; > + msk->retrans_redundant = false; > > mptcp_pm_data_init(msk); > > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h > index adfb758a842f..e6bebde2f3ab 100644 > --- a/net/mptcp/protocol.h > +++ b/net/mptcp/protocol.h > @@ -290,6 +290,7 @@ struct mptcp_sock { > bool use_64bit_ack; /* Set when we received a 64-bit DSN */ > bool csum_enabled; > bool allow_infinite_fallback; > + bool retrans_redundant; > u8 mpc_endpoint_id; > u8 recvmsg_inq:1, > cork:1, > -- > 2.35.3 > > > -- Mat Martineau Intel ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-12-06 1:37 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-12-05 12:35 [PATCH mptcp-next 0/2] BPF redundant scheduler, part 3 Geliang Tang 2022-12-05 12:35 ` [PATCH mptcp-next 1/2] Squash to "mptcp: use get_send wrapper, v22" Geliang Tang 2022-12-06 1:22 ` Mat Martineau 2022-12-05 12:35 ` [PATCH mptcp-next 2/2] mptcp: retrans for redundant sends Geliang Tang 2022-12-06 1:37 ` Mat Martineau
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.