[PATCH V3 net-next] TLP: Don't reschedule PTO when there's one outstanding TLP retransmission

If there is one TLP probe went out(TLP use the write_queue_tail
packet as TLP probe, we assume this first TLP probe named A), and
this TLP probe was not acked by receive side.

Then the transmit side sent the next two packetes out(named B,C),
but unfortunately these two packets are also not acked by receive side.

And then there is one data packet with ack_seq A arrive at transmit
side, in tcp_ack() will call tcp_schedule_loss_probe() to rearm PTO,
the handler tcp_send_loss_probe() is to check
if(tp->tlp_high_seq) then go to rearm_timer(because there is one
outstanding TLP named A), so the new TLP probe can't be sent out and
it needs to rearm the RTO timer(timeout is relative to the transmit
time of the write queue head).

After that, there is another data packet with ack_seq A is received,
if the tlp_time_stamp is greater than rto_time_stamp, it will reset
the TLP timeout, which is before previous RTO timeout, so PTO is
rearm and previous RTO is cleared. Because there is no
retransmission packet was sent or no TLP sack receive,
tp->tlp_high_seq can't be reset to zero and the next TLP probe also
can't be sent out, so there is no way(or very long time)
to retransmit the lost packet.

This fix is to check(tp->tlp_high_seq) in tcp_schedule_loss_probe()
when TLP PTO is after RTO, It is not needed to reschedule PTO when
there is one outstanding TLP retransmission, so if the TLP A is lost
RTO can retransmit lost packet, then tp->tlp_high_seq will be set to
0, and TLP will go to the normal work process.

v1->v2
	refine some words of code and patch comments.
v2->v3
	delete senseless "{" and "}" in if clause.

Signed-off-by: Mao Wenan <redacted>
---
 net/ipv4/tcp_output.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 886d874..b59975f 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c

@@ -2377,6 +2377,7 @@ bool tcp_schedule_loss_probe(struct sock *sk)
 	struct inet_connection_sock *icsk = inet_csk(sk);
 	struct tcp_sock *tp = tcp_sk(sk);
 	u32 timeout, tlp_time_stamp, rto_time_stamp;
+	s32 delta;
 
 	/* No consecutive loss probes. */
 	if (WARN_ON(icsk->icsk_pending == ICSK_TIME_LOSS_PROBE)) {

@@ -2423,7 +2424,12 @@ bool tcp_schedule_loss_probe(struct sock *sk)
 	tlp_time_stamp = tcp_jiffies32 + timeout;
 	rto_time_stamp = (u32)inet_csk(sk)->icsk_timeout;
 	if ((s32)(tlp_time_stamp - rto_time_stamp) > 0) {
-		s32 delta = rto_time_stamp - tcp_jiffies32;
+		/* It is not needed to reschedule PTO when there 
+		 * is one outstanding TLP retransmission. 
+		 */
+		if (tp->tlp_high_seq)
+			return false;
+		delta = rto_time_stamp - tcp_jiffies32;
 		if (delta > 0)
 			timeout = delta;
 	}

-- 
2.5.0

On Thu, Jul 27, 2017 at 8:08 AM, Mao Wenan [off-list ref] wrote:

If there is one TLP probe went out(TLP use the write_queue_tail
packet as TLP probe, we assume this first TLP probe named A), and
this TLP probe was not acked by receive side.

Then the transmit side sent the next two packetes out(named B,C),
but unfortunately these two packets are also not acked by receive side.

And then there is one data packet with ack_seq A arrive at transmit
side, in tcp_ack() will call tcp_schedule_loss_probe() to rearm PTO,
the handler tcp_send_loss_probe() is to check
if(tp->tlp_high_seq) then go to rearm_timer(because there is one
outstanding TLP named A), so the new TLP probe can't be sent out and
it needs to rearm the RTO timer(timeout is relative to the transmit
time of the write queue head).

After that, there is another data packet with ack_seq A is received,
if the tlp_time_stamp is greater than rto_time_stamp, it will reset
the TLP timeout, which is before previous RTO timeout, so PTO is
rearm and previous RTO is cleared. Because there is no
retransmission packet was sent or no TLP sack receive,
tp->tlp_high_seq can't be reset to zero and the next TLP probe also
can't be sent out, so there is no way(or very long time)
to retransmit the lost packet.

This fix is to check(tp->tlp_high_seq) in tcp_schedule_loss_probe()
when TLP PTO is after RTO, It is not needed to reschedule PTO when
there is one outstanding TLP retransmission, so if the TLP A is lost
RTO can retransmit lost packet, then tp->tlp_high_seq will be set to
0, and TLP will go to the normal work process.

v1->v2
        refine some words of code and patch comments.
v2->v3
        delete senseless "{" and "}" in if clause.

Signed-off-by: Mao Wenan <redacted>

Thanks for posting this patch with a detailed problem description, as
well as a trace in the thread for v1 of the patch. This was very
helpful!

Thinking about the problem you describe, and looking at the trace,
AFAICT I don't think this is the patch we want.

We can still have this problem of improperly/repeatedly rescheduling a
PTO even when the TLPs are new data. When the TLPs are new data
tp->tlp_high_seq is not set, and so the patch above will not help.

I think the broader problem is hinted at in this part of your commit
description:

After that, there is another data packet with ack_seq A is received,
if the tlp_time_stamp is greater than rto_time_stamp, it will reset
the TLP timeout

The broader problem here is that an incoming data packet (with no new
ACK/SACK info) affected the TLP for our outbound data. That is a
problem because such incoming data can cause us to delay the TLP when
there is no reason to.

I think this is basically the same as the TLP issue from the "TCP fast
retransmit issues" thread on netdev from July 26. Our TCP team at
Google has a proposed fix for this more general issue that we have
tested and reviewed. I will post a quick summary of the proposed patch
in the "TCP fast retransmit issues" thread. Once the patch has
undergone a little more testing we will send it to the list, hopefully
next week.

Thanks!
neal

-----Original Message-----
From: Neal Cardwell [mailto:ncardwell@google.com]
Sent: Saturday, July 29, 2017 6:48 AM
To: maowenan
Cc: Netdev; David Miller; Yuchung Cheng; Nandita Dukkipati; weiyongjun (A);
Chenweilong; Wangkefeng (Kevin)
Subject: Re: [PATCH V3 net-next] TLP: Don't reschedule PTO when there's one
outstanding TLP retransmission

On Thu, Jul 27, 2017 at 8:08 AM, Mao Wenan [off-list ref]
wrote:

quoted

If there is one TLP probe went out(TLP use the write_queue_tail packet
as TLP probe, we assume this first TLP probe named A), and this TLP
probe was not acked by receive side.

Then the transmit side sent the next two packetes out(named B,C), but
unfortunately these two packets are also not acked by receive side.

And then there is one data packet with ack_seq A arrive at transmit
side, in tcp_ack() will call tcp_schedule_loss_probe() to rearm PTO,
the handler tcp_send_loss_probe() is to check
if(tp->tlp_high_seq) then go to rearm_timer(because there is one
outstanding TLP named A), so the new TLP probe can't be sent out and
it needs to rearm the RTO timer(timeout is relative to the transmit
time of the write queue head).

After that, there is another data packet with ack_seq A is received,
if the tlp_time_stamp is greater than rto_time_stamp, it will reset
the TLP timeout, which is before previous RTO timeout, so PTO is rearm
and previous RTO is cleared. Because there is no retransmission packet
was sent or no TLP sack receive,
tp->tlp_high_seq can't be reset to zero and the next TLP probe also
can't be sent out, so there is no way(or very long time) to retransmit
the lost packet.

This fix is to check(tp->tlp_high_seq) in tcp_schedule_loss_probe()
when TLP PTO is after RTO, It is not needed to reschedule PTO when
there is one outstanding TLP retransmission, so if the TLP A is lost
RTO can retransmit lost packet, then tp->tlp_high_seq will be set to
0, and TLP will go to the normal work process.

v1->v2
        refine some words of code and patch comments.
v2->v3
        delete senseless "{" and "}" in if clause.

Signed-off-by: Mao Wenan <redacted>

Thanks for posting this patch with a detailed problem description, as well as a
trace in the thread for v1 of the patch. This was very helpful!

Thinking about the problem you describe, and looking at the trace, AFAICT I
don't think this is the patch we want.

We can still have this problem of improperly/repeatedly rescheduling a PTO
even when the TLPs are new data. When the TLPs are new data
tp->tlp_high_seq is not set, and so the patch above will not help.

[Mao Wenan]ok, We have reproduced this issue with packetdrill yesterday, 
there is no the same issue when TLP send new data packet, RTO will be fired and 
retransmit packet.

I think the broader problem is hinted at in this part of your commit
description:

quoted

After that, there is another data packet with ack_seq A is received,
if the tlp_time_stamp is greater than rto_time_stamp, it will reset
the TLP timeout

The broader problem here is that an incoming data packet (with no new
ACK/SACK info) affected the TLP for our outbound data. That is a problem
because such incoming data can cause us to delay the TLP when there is no
reason to.

I think this is basically the same as the TLP issue from the "TCP fast retransmit
issues" thread on netdev from July 26. Our TCP team at Google has a proposed
fix for this more general issue that we have tested and reviewed. I will post a
quick summary of the proposed patch in the "TCP fast retransmit issues"
thread. Once the patch has undergone a little more testing we will send it to
the list, hopefully next week.

[Mao Wenan]OK.

Thanks!
neal

On Fri, Jul 28, 2017 at 9:39 PM, maowenan [off-list ref] wrote:

[Mao Wenan]ok, We have reproduced this issue with packetdrill yesterday,
there is no the same issue when TLP send new data packet, RTO will be fired and
retransmit packet.

That's great to hear that you were able to reproduce this with
packetdrill. Would you be able to share the packetdrill scripts that
reproduce your issues? I would like to make sure our proposed patch
addresses your scenarios as well.

thanks,
neal

-----Original Message-----
From: Neal Cardwell [mailto:ncardwell@google.com]
Sent: Saturday, July 29, 2017 10:04 PM
To: maowenan
Cc: Netdev; David Miller; Yuchung Cheng; Nandita Dukkipati; weiyongjun (A);
Chenweilong; Wangkefeng (Kevin)
Subject: Re: [PATCH V3 net-next] TLP: Don't reschedule PTO when there's one
outstanding TLP retransmission

On Fri, Jul 28, 2017 at 9:39 PM, maowenan [off-list ref] wrote:

quoted

[Mao Wenan]ok, We have reproduced this issue with packetdrill
yesterday, there is no the same issue when TLP send new data packet,
RTO will be fired and retransmit packet.

That's great to hear that you were able to reproduce this with packetdrill.
Would you be able to share the packetdrill scripts that reproduce your issues? I
would like to make sure our proposed patch addresses your scenarios as well.

[Mao Wenan]please refer to the attachment, test.pkt is packetdrill script. 
In test.pcap, packet number 17 is the TLP probe, packet number 218 is the
retransmission packet because client don't send data packet to server. 
From the capture time, there are about 6 seconds the retransmission
packet can be sent, and this time can be added more as long as client 
send data packet continually.
I have reproduced this issue in Linux 4.13-rc3, 3.10, 4.1. Please check the timing 
When you use test.pkt to reproduce in your environment.

thanks,
neal

On Sun, Jul 30, 2017 at 11:29 PM, maowenan [off-list ref] wrote:

[Mao Wenan]please refer to the attachment, test.pkt is packetdrill script.
In test.pcap, packet number 17 is the TLP probe, packet number 218 is the
retransmission packet because client don't send data packet to server.
From the capture time, there are about 6 seconds the retransmission
packet can be sent, and this time can be added more as long as client
send data packet continually.
I have reproduced this issue in Linux 4.13-rc3, 3.10, 4.1. Please check the timing
When you use test.pkt to reproduce in your environment.

Thank you for your very nice packetdrill test case illustrating this
problem! And thanks for verifying that the problem shows up in those
kernel versions.

We are able to run the script in our environment and both verify that
the bug is the one we hypothesized, and verify our  proposed patch
fixes it (the RTO for the TLP happens 221ms after the TLP, instead of
~5 secs later). We will send out our proposed patches ASAP.

Thanks!
neal

On Mon, Jul 31, 2017 at 11:49 AM, Neal Cardwell [off-list ref] wrote:

On Sun, Jul 30, 2017 at 11:29 PM, maowenan [off-list ref] wrote:

quoted

[Mao Wenan]please refer to the attachment, test.pkt is packetdrill script.
In test.pcap, packet number 17 is the TLP probe, packet number 218 is the
retransmission packet because client don't send data packet to server.
From the capture time, there are about 6 seconds the retransmission
packet can be sent, and this time can be added more as long as client
send data packet continually.
I have reproduced this issue in Linux 4.13-rc3, 3.10, 4.1. Please check the timing
When you use test.pkt to reproduce in your environment.

Thank you for your very nice packetdrill test case illustrating this
problem! And thanks for verifying that the problem shows up in those
kernel versions.

We are able to run the script in our environment and both verify that
the bug is the one we hypothesized, and verify our  proposed patch
fixes it (the RTO for the TLP happens 221ms after the TLP, instead of
~5 secs later). We will send out our proposed patches ASAP.

The timer patches are upstream for review for the "net" branch:

  https://patchwork.ozlabs.org/patch/796057/
  https://patchwork.ozlabs.org/patch/796058/
  https://patchwork.ozlabs.org/patch/796059/

Again, thank you for reporting this and providing a packetdrill script
to reproduce this!

neal