Message ID | 20201001081406.4548-1-fw@strlen.de |
---|---|
State | Accepted, archived |
Commit | 15321713bc1f110e7c4ce5fd5bd3766901efbabd |
Delegated to: | Matthieu Baerts |
Headers | show |
Series | [mptcp-next] mptcp: skip to next candidate if subflow has unacked data | expand |
On Thu, 1 Oct 2020, Florian Westphal wrote: > In case a subflow path is blocked, MPTCP-level retransmit may not take > place anymore because such subflow is likely to have unacked data left > in its write queue. > > Ignore subflows that have experienced loss and test next candidate. > > Fixes: 3b1d6210a95773691 ("mptcp: implement and use MPTCP-level retransmission") > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > net/mptcp/protocol.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > index 7aa52a3bc5e2..0266d8ac24d2 100644 > --- a/net/mptcp/protocol.c > +++ b/net/mptcp/protocol.c > @@ -1732,8 +1732,11 @@ static struct sock *mptcp_subflow_get_retrans(const struct mptcp_sock *msk) > continue; > > /* still data outstanding at TCP level? Don't retransmit. */ > - if (!tcp_write_queue_empty(ssk)) > + if (!tcp_write_queue_empty(ssk)) { > + if (inet_csk(ssk)->icsk_ca_state >= TCP_CA_Loss) > + continue; > return NULL; > + } > > if (subflow->backup) { > if (!backup) > -- > 2.26.2 Checking for the loss state is an improvement when the first subflow in the list is blocking retransmissions, looks like a good idea to me. We can still get inconsistent retransmission behavior depending on the order of subflows in the list - if the first non-loss subflow has an empty write queue then retransmission will happen even if other subflows have queued data. I suppose this causes the opposite problem, retransmitting when it should wait. Better to handle a fix for that separately? -- Mat Martineau Intel
Mat Martineau <mathew.j.martineau@linux.intel.com> wrote: > We can still get inconsistent retransmission behavior depending on the order > of subflows in the list - if the first non-loss subflow has an empty write > queue then retransmission will happen even if other subflows have queued > data. I suppose this causes the opposite problem, retransmitting when it > should wait. Better to handle a fix for that separately? Yes, if we observe too many bogus retransmits then the mptcp-level retransmit timer is firing too early, i'd say thats a different bug.
Hi Florian, Mat, On 01/10/2020 10:14, Florian Westphal wrote: > In case a subflow path is blocked, MPTCP-level retransmit may not take > place anymore because such subflow is likely to have unacked data left > in its write queue. > > Ignore subflows that have experienced loss and test next candidate. Thank you for the patch and the review! Just applied with Mat's Reviewed-by. - 15321713bc1f: mptcp: skip to next candidate if subflow has unacked data - Results: 3fd01a5ab17b..0e70ef7be280 Tests and export will be started soon! Cheers, Matt
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index 7aa52a3bc5e2..0266d8ac24d2 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -1732,8 +1732,11 @@ static struct sock *mptcp_subflow_get_retrans(const struct mptcp_sock *msk) continue; /* still data outstanding at TCP level? Don't retransmit. */ - if (!tcp_write_queue_empty(ssk)) + if (!tcp_write_queue_empty(ssk)) { + if (inet_csk(ssk)->icsk_ca_state >= TCP_CA_Loss) + continue; return NULL; + } if (subflow->backup) { if (!backup)
In case a subflow path is blocked, MPTCP-level retransmit may not take place anymore because such subflow is likely to have unacked data left in its write queue. Ignore subflows that have experienced loss and test next candidate. Fixes: 3b1d6210a95773691 ("mptcp: implement and use MPTCP-level retransmission") Signed-off-by: Florian Westphal <fw@strlen.de> --- net/mptcp/protocol.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)