[PATCH] tcp: TCP_USER_TIMEOUT can not work in tcp_probe_timer()

[PATCH] tcp: TCP_USER_TIMEOUT can not work in tcp_probe_timer()

[liujian56]

From: liujian <liujian56@huawei.com>

After the tcp socket go to ESTABLISHED stat, change IP address (server
side),
then the tcp socket will go tcp_probe_timer process.

[root@localhost net]# netstat -toe
Active Internet connections (w/o servers)
Proto Recv-Q Send-Q Local Address           Foreign Address         State       User       Inode      Timer
tcp        0   1104 9.81.254:personal-agent 9.84.201.213:23597      ESTABLISHED root       12819      probe (4.36/0/7)
[root@localhost net]# cat /proc/net/tcp
  sl  local_address rem_address   st tx_queue rx_queue tr tm->when retrnsmt   uid  timeout inode                                                     
   3: B1FE5109:15B3 D5C95409:5C2D 01 00000495:00000000 04:0000005E 00000000     0        7 12819 2 ffff95cdcf45a000 20 4 1 10 -1 

In my test case, tcp_write_queue_head(sk) and tcp_send_head(sk) is  same
SKB.
And ((s32)(tcp_time_stamp(tp) - start_ts) >
jiffies_to_msecs(icsk->icsk_user_timeout))
 always is false.
 Here use keepalive_time_elapsed(tp) to do the compare as
tcp_keepalive_timer do.

Signed-off-by: liujian <liujian56@huawei.com>
---
 net/ipv4/tcp_timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index 655dd8d..2a28a03 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -325,7 +325,7 @@ static void tcp_probe_timer(struct sock *sk)
 	if (!start_ts)
 		tcp_send_head(sk)->skb_mstamp = tp->tcp_mstamp;
 	else if (icsk->icsk_user_timeout &&
-		 (s32)(tcp_time_stamp(tp) - start_ts) >
+		 keepalive_time_elapsed(tp) >=
 		 jiffies_to_msecs(icsk->icsk_user_timeout))
 		goto abort;
 
-- 
1.8.3.1

Re: [PATCH] tcp: TCP_USER_TIMEOUT can not work in tcp_probe_timer()

[Eric Dumazet]

On Mon, 2017-09-11 at 14:27 +0800, liujian56@huawei.com wrote:
> From: liujian <liujian56@huawei.com>
> 
> After the tcp socket go to ESTABLISHED stat, change IP address (server
> side),
> then the tcp socket will go tcp_probe_timer process.
> 
> [root@localhost net]# netstat -toe
> Active Internet connections (w/o servers)
> Proto Recv-Q Send-Q Local Address           Foreign Address         State       User       Inode      Timer
> tcp        0   1104 9.81.254:personal-agent 9.84.201.213:23597      ESTABLISHED root       12819      probe (4.36/0/7)
> [root@localhost net]# cat /proc/net/tcp
>   sl  local_address rem_address   st tx_queue rx_queue tr tm->when retrnsmt   uid  timeout inode                                                     
>    3: B1FE5109:15B3 D5C95409:5C2D 01 00000495:00000000 04:0000005E 00000000     0        7 12819 2 ffff95cdcf45a000 20 4 1 10 -1 
> 
> In my test case, tcp_write_queue_head(sk) and tcp_send_head(sk) is  same
> SKB.
> And ((s32)(tcp_time_stamp(tp) - start_ts) >
> jiffies_to_msecs(icsk->icsk_user_timeout))
>  always is false.

Interesting.

>  Here use keepalive_time_elapsed(tp) to do the compare as
> tcp_keepalive_timer do.


But zero window probe and TCP_USER_TIMEOUT can be used without
keepalives...


A packetdrill test would help, I will write one.

Thanks.

Re: [PATCH] tcp: TCP_USER_TIMEOUT can not work in tcp_probe_timer()

[Eric Dumazet]

On Mon, 2017-09-11 at 07:49 -0700, Eric Dumazet wrote:
> On Mon, 2017-09-11 at 14:27 +0800, liujian56@huawei.com wrote:
> > From: liujian <liujian56@huawei.com>
> > 
> > After the tcp socket go to ESTABLISHED stat, change IP address (server
> > side),
> > then the tcp socket will go tcp_probe_timer process.
> > 
> > [root@localhost net]# netstat -toe
> > Active Internet connections (w/o servers)
> > Proto Recv-Q Send-Q Local Address           Foreign Address         State       User       Inode      Timer
> > tcp        0   1104 9.81.254:personal-agent 9.84.201.213:23597      ESTABLISHED root       12819      probe (4.36/0/7)
> > [root@localhost net]# cat /proc/net/tcp
> >   sl  local_address rem_address   st tx_queue rx_queue tr tm->when retrnsmt   uid  timeout inode                                                     
> >    3: B1FE5109:15B3 D5C95409:5C2D 01 00000495:00000000 04:0000005E 00000000     0        7 12819 2 ffff95cdcf45a000 20 4 1 10 -1 
> > 
> > In my test case, tcp_write_queue_head(sk) and tcp_send_head(sk) is  same
> > SKB.
> > And ((s32)(tcp_time_stamp(tp) - start_ts) >
> > jiffies_to_msecs(icsk->icsk_user_timeout))
> >  always is false.
> 
> Interesting.
> 
> >  Here use keepalive_time_elapsed(tp) to do the compare as
> > tcp_keepalive_timer do.
> 
> 
> But zero window probe and TCP_USER_TIMEOUT can be used without
> keepalives...
> 
> 
> A packetdrill test would help, I will write one.

So existing code seems to work :

# cat window-probe-without-data-user-timeout.pkt

    0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
   +0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
   +0 bind(3, ..., ...) = 0
   +0 listen(3, 1) = 0

   +0 < S 0:0(0) win 0 <mss 1460,sackOK,nop,nop,nop,wscale 7>
   +0 > S. 0:0(0) ack 1 <mss 1460,nop,nop,sackOK,nop,wscale 8>

// Client advertises a zero receive window, so we can't send.
  +.1 < . 1:1(0) ack 1 win 0
   +0 accept(3, ..., ...) = 4

   +0 setsockopt(4, SOL_TCP, TCP_USER_TIMEOUT, [3000], 4) = 0
   +0 write(4, ..., 2920) = 2920

// Window probes are scheduled just like RTOs.
  +.3~+.31 > . 0:0(0) ack 1
  +.6~+.62 > . 0:0(0) ack 1
 +1.2~+1.24 > . 0:0(0) ack 1
 +2.4~+2.48 > . 0:0(0) ack 1

# ./packetdrill window-probe-without-data-user_timeout.pkt
08:10:39.306137 IP 192.0.2.1.58149 > 192.168.79.31.8080: Flags [S], seq 0, win 0, options [mss 1460,sackOK,nop,nop,nop,wscale 7], length 0
08:10:39.306166 IP 192.168.79.31.8080 > 192.0.2.1.58149: Flags [S.], seq 3982794529, ack 1, win 29200, options [mss 1460,nop,nop,sackOK,nop,wscale 8], length 0
08:10:39.406296 IP 192.0.2.1.58149 > 192.168.79.31.8080: Flags [.], ack 1, win 0, length 0
08:10:39.716004 IP 192.168.79.31.8080 > 192.0.2.1.58149: Flags [.], ack 1, win 115, length 0
08:10:40.327133 IP 192.168.79.31.8080 > 192.0.2.1.58149: Flags [.], ack 1, win 115, length 0
08:10:41.540243 IP 192.168.79.31.8080 > 192.0.2.1.58149: Flags [.], ack 1, win 115, length 0

You can see we got only 3 probes, not 4.

Re: [PATCH] tcp: TCP_USER_TIMEOUT can not work in tcp_probe_timer()

[Eric Dumazet]

On Mon, 2017-09-11 at 08:13 -0700, Eric Dumazet wrote:

> You can see we got only 3 probes, not 4.

Here is complete packetdrill test showing that code behaves as expected.

    0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
   +0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
   +0 bind(3, ..., ...) = 0
   +0 listen(3, 1) = 0

   +0 < S 0:0(0) win 0 <mss 1460,sackOK,nop,nop,nop,wscale 7>
   +0 > S. 0:0(0) ack 1 <mss 1460,nop,nop,sackOK,nop,wscale 8>

// Client advertises a zero receive window, so we can't send.
  +.1 < . 1:1(0) ack 1 win 0
   +0 accept(3, ..., ...) = 4

   +0 setsockopt(4, SOL_TCP, TCP_USER_TIMEOUT, [3000], 4) = 0
   +0 write(4, ..., 2920) = 2920

// Window probes are scheduled just like RTOs.
  +.3~+.31 > . 0:0(0) ack 1
  +.6~+.62 > . 0:0(0) ack 1
 +1.2~+1.24 > . 0:0(0) ack 1

// Peer opens its window too late !
   +3 < . 1:1(0) ack 1 win 1000
   +0 > R 1:1(0)

Re: [PATCH] tcp: TCP_USER_TIMEOUT can not work in tcp_probe_timer()

[liujian]

Hi,

In the scenario, tcp server side IP changed, and at that memont,
userspace application still send data continuously;
tcp_send_head(sk)'s timestamp always be refreshed.

Here is the packetdrill script:

   0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
   +0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
   +0 bind(3, ..., ...) = 0
   +0 listen(3, 1) = 0

   +0 < S 0:0(0) win 0 <mss 1460,sackOK,nop,nop,nop,wscale 7>
   +0 > S. 0:0(0) ack 1 <mss 1460,nop,nop,sackOK,nop,wscale 7>

  +.1 < . 1:1(0) ack 1 win 65530
   +0 accept(3, ..., ...) = 4

   +0 setsockopt(4, SOL_TCP, TCP_USER_TIMEOUT, [3000], 4) = 0
   +0 write(4, ..., 24) = 24
   +0 > P. 1:25(24) ack 1 win 229
   +.1 < . 1:1(0) ack 25 win 65530

//change the ipaddress
   +1 `ifconfig tun0 192.168.0.10/16`

   +1 write(4, ..., 24) = 24
   +1 write(4, ..., 24) = 24
   +1 write(4, ..., 24) = 24
   +1 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24

   +0 `ifconfig tun0 192.168.0.1/16`
   +0 < . 1:1(0) ack 1 win 1000
   +0 write(4, ..., 24) = -1


[root@localhost ~]# time ./gtests/net/packetdrill/packetdrill test.pkt
test.pkt:50: runtime error in write call: Expected result -1 but got 24 with errno 2 (No such file or directory)

real	1m11.364s
user	0m0.028s
sys	0m0.106s

[root@localhost ~]# netstat -toen
Active Internet connections (w/o servers)
Proto Recv-Q Send-Q Local Address           Foreign Address         State       User       Inode      Timer
tcp        0    504 192.168.0.1:8080        192.0.2.1:33993         ESTABLISHED 0          45453      probe (22.38/0/7)

since the script didn't wait for enough time, here only got 7 probes.

在 2017/9/11 23:22, Eric Dumazet 写道:
> On Mon, 2017-09-11 at 08:13 -0700, Eric Dumazet wrote:
> 
>> You can see we got only 3 probes, not 4.
> 
> Here is complete packetdrill test showing that code behaves as expected.
> 
>     0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
>    +0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
>    +0 bind(3, ..., ...) = 0
>    +0 listen(3, 1) = 0
> 
>    +0 < S 0:0(0) win 0 <mss 1460,sackOK,nop,nop,nop,wscale 7>
>    +0 > S. 0:0(0) ack 1 <mss 1460,nop,nop,sackOK,nop,wscale 8>
> 
> // Client advertises a zero receive window, so we can't send.
>   +.1 < . 1:1(0) ack 1 win 0
>    +0 accept(3, ..., ...) = 4
> 
>    +0 setsockopt(4, SOL_TCP, TCP_USER_TIMEOUT, [3000], 4) = 0
>    +0 write(4, ..., 2920) = 2920
> 
> // Window probes are scheduled just like RTOs.
>   +.3~+.31 > . 0:0(0) ack 1
>   +.6~+.62 > . 0:0(0) ack 1
>  +1.2~+1.24 > . 0:0(0) ack 1
> 
> // Peer opens its window too late !
>    +3 < . 1:1(0) ack 1 win 1000
>    +0 > R 1:1(0)
> 
> 
> 
> .
> 

Re: [PATCH] tcp: TCP_USER_TIMEOUT can not work in tcp_probe_timer()

[Eric Dumazet]

On Tue, 2017-09-12 at 14:08 +0800, liujian wrote:
> Hi,
> 
> In the scenario, tcp server side IP changed, and at that memont,
> userspace application still send data continuously;
> tcp_send_head(sk)'s timestamp always be refreshed.
> 
> Here is the packetdrill script:
> 
>    0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
>    +0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
>    +0 bind(3, ..., ...) = 0
>    +0 listen(3, 1) = 0
> 
>    +0 < S 0:0(0) win 0 <mss 1460,sackOK,nop,nop,nop,wscale 7>
>    +0 > S. 0:0(0) ack 1 <mss 1460,nop,nop,sackOK,nop,wscale 7>
> 
>   +.1 < . 1:1(0) ack 1 win 65530
>    +0 accept(3, ..., ...) = 4
> 
>    +0 setsockopt(4, SOL_TCP, TCP_USER_TIMEOUT, [3000], 4) = 0
>    +0 write(4, ..., 24) = 24
>    +0 > P. 1:25(24) ack 1 win 229
>    +.1 < . 1:1(0) ack 25 win 65530
> 
> //change the ipaddress
>    +1 `ifconfig tun0 192.168.0.10/16`
> 
>    +1 write(4, ..., 24) = 24
>    +1 write(4, ..., 24) = 24
>    +1 write(4, ..., 24) = 24
>    +1 write(4, ..., 24) = 24
>    +3 write(4, ..., 24) = 24
>    +3 write(4, ..., 24) = 24
>    +3 write(4, ..., 24) = 24
>    +3 write(4, ..., 24) = 24
>    +3 write(4, ..., 24) = 24
>    +3 write(4, ..., 24) = 24
>    +3 write(4, ..., 24) = 24
>    +3 write(4, ..., 24) = 24
>    +3 write(4, ..., 24) = 24
>    +3 write(4, ..., 24) = 24
>    +3 write(4, ..., 24) = 24
>    +3 write(4, ..., 24) = 24
>    +3 write(4, ..., 24) = 24
>    +3 write(4, ..., 24) = 24
>    +3 write(4, ..., 24) = 24
>    +3 write(4, ..., 24) = 24
>    +3 write(4, ..., 24) = 24
>    +3 write(4, ..., 24) = 24
>    +3 write(4, ..., 24) = 24
>    +3 write(4, ..., 24) = 24
>    +3 write(4, ..., 24) = 24
>    +3 write(4, ..., 24) = 24
> 
>    +0 `ifconfig tun0 192.168.0.1/16`
>    +0 < . 1:1(0) ack 1 win 1000
>    +0 write(4, ..., 24) = -1
> 
> 

This has nothing to do with the code patch you have changed.

How have you tested your patch exactly ?


> [root@localhost ~]# time ./gtests/net/packetdrill/packetdrill test.pkt
> test.pkt:50: runtime error in write call: Expected result -1 but got 24 with errno 2 (No such file or directory)
> 
> real	1m11.364s
> user	0m0.028s
> sys	0m0.106s
> 
> [root@localhost ~]# netstat -toen
> Active Internet connections (w/o servers)
> Proto Recv-Q Send-Q Local Address           Foreign Address         State       User       Inode      Timer
> tcp        0    504 192.168.0.1:8080        192.0.2.1:33993         ESTABLISHED 0          45453      probe (22.38/0/7)
> 
> since the script didn't wait for enough time, here only got 7 probes.
> 
> 在 2017/9/11 23:22, Eric Dumazet 写道:
> > On Mon, 2017-09-11 at 08:13 -0700, Eric Dumazet wrote:
> > 
> >> You can see we got only 3 probes, not 4.
> > 
> > Here is complete packetdrill test showing that code behaves as expected.
> > 
> >     0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
> >    +0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
> >    +0 bind(3, ..., ...) = 0
> >    +0 listen(3, 1) = 0
> > 
> >    +0 < S 0:0(0) win 0 <mss 1460,sackOK,nop,nop,nop,wscale 7>
> >    +0 > S. 0:0(0) ack 1 <mss 1460,nop,nop,sackOK,nop,wscale 8>
> > 
> > // Client advertises a zero receive window, so we can't send.
> >   +.1 < . 1:1(0) ack 1 win 0
> >    +0 accept(3, ..., ...) = 4
> > 
> >    +0 setsockopt(4, SOL_TCP, TCP_USER_TIMEOUT, [3000], 4) = 0
> >    +0 write(4, ..., 2920) = 2920
> > 
> > // Window probes are scheduled just like RTOs.
> >   +.3~+.31 > . 0:0(0) ack 1
> >   +.6~+.62 > . 0:0(0) ack 1
> >  +1.2~+1.24 > . 0:0(0) ack 1
> > 
> > // Peer opens its window too late !
> >    +3 < . 1:1(0) ack 1 win 1000
> >    +0 > R 1:1(0)
> > 
> > 
> > 
> > .
> > 
> 

Re: [PATCH] tcp: TCP_USER_TIMEOUT can not work in tcp_probe_timer()

[Eric Dumazet]

On Tue, 2017-09-12 at 08:05 -0700, Eric Dumazet wrote:
> On Tue, 2017-09-12 at 14:08 +0800, liujian wrote:
> > Hi,
> > 
> > In the scenario, tcp server side IP changed, and at that memont,
> > userspace application still send data continuously;
> > tcp_send_head(sk)'s timestamp always be refreshed.
> > 
> > Here is the packetdrill script:
> > 
> >    0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
> >    +0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
> >    +0 bind(3, ..., ...) = 0
> >    +0 listen(3, 1) = 0
> > 
> >    +0 < S 0:0(0) win 0 <mss 1460,sackOK,nop,nop,nop,wscale 7>
> >    +0 > S. 0:0(0) ack 1 <mss 1460,nop,nop,sackOK,nop,wscale 7>
> > 
> >   +.1 < . 1:1(0) ack 1 win 65530
> >    +0 accept(3, ..., ...) = 4
> > 
> >    +0 setsockopt(4, SOL_TCP, TCP_USER_TIMEOUT, [3000], 4) = 0
> >    +0 write(4, ..., 24) = 24
> >    +0 > P. 1:25(24) ack 1 win 229
> >    +.1 < . 1:1(0) ack 25 win 65530
> > 
> > //change the ipaddress
> >    +1 `ifconfig tun0 192.168.0.10/16`
> > 
> >    +1 write(4, ..., 24) = 24
> >    +1 write(4, ..., 24) = 24
> >    +1 write(4, ..., 24) = 24
> >    +1 write(4, ..., 24) = 24
> >    +3 write(4, ..., 24) = 24
> >    +3 write(4, ..., 24) = 24
> >    +3 write(4, ..., 24) = 24
> >    +3 write(4, ..., 24) = 24
> >    +3 write(4, ..., 24) = 24
> >    +3 write(4, ..., 24) = 24
> >    +3 write(4, ..., 24) = 24
> >    +3 write(4, ..., 24) = 24
> >    +3 write(4, ..., 24) = 24
> >    +3 write(4, ..., 24) = 24
> >    +3 write(4, ..., 24) = 24
> >    +3 write(4, ..., 24) = 24
> >    +3 write(4, ..., 24) = 24
> >    +3 write(4, ..., 24) = 24
> >    +3 write(4, ..., 24) = 24
> >    +3 write(4, ..., 24) = 24
> >    +3 write(4, ..., 24) = 24
> >    +3 write(4, ..., 24) = 24
> >    +3 write(4, ..., 24) = 24
> >    +3 write(4, ..., 24) = 24
> >    +3 write(4, ..., 24) = 24
> >    +3 write(4, ..., 24) = 24
> > 
> >    +0 `ifconfig tun0 192.168.0.1/16`
> >    +0 < . 1:1(0) ack 1 win 1000
> >    +0 write(4, ..., 24) = -1
> > 
> > 
> 
> This has nothing to do with the code patch you have changed.
> 
> How have you tested your patch exactly ?
> 


lpaa23:~# ss -toenmi src :8080
State      Recv-Q Send-Q Local Address:Port               Peer
Address:Port              
ESTAB      0      144    192.168.134.161:8080
192.0.2.1:51165               timer:(persist,8.262ms,5) ino:1
82083 sk:3 <->
	 skmem:(r0,rb359040,t0,tb46080,f1792,w2304,o0,bl0,d0) sack cubic
wscale:7,8 rto:301 backoff:5 rtt:100.127/37.576 
mss:1460 rcvmss:536 advmss:1460 cwnd:10 bytes_acked:24 segs_out:12
segs_in:3 data_segs_out:12 send 1.2Mbps lastsnd:1370 l
astrcv:13348 lastack:13248 pacing_rate 2.3Mbps delivery_rate 116.7Kbps
app_limited busy:11346ms rcv_space:29200 notsent:1
44 minrtt:100.043

This is the typical RTO timer, not zero window probe.

Re: [PATCH] tcp: TCP_USER_TIMEOUT can not work in tcp_probe_timer()

[liujian]

在 2017/9/12 23:38, Eric Dumazet 写道:
> On Tue, 2017-09-12 at 08:05 -0700, Eric Dumazet wrote:
>> On Tue, 2017-09-12 at 14:08 +0800, liujian wrote:
>>> Hi,
>>>
>>> In the scenario, tcp server side IP changed, and at that memont,
>>> userspace application still send data continuously;
>>> tcp_send_head(sk)'s timestamp always be refreshed.
>>>
>>> Here is the packetdrill script:
>>>
>>>    0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
>>>    +0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
>>>    +0 bind(3, ..., ...) = 0
>>>    +0 listen(3, 1) = 0
>>>
>>>    +0 < S 0:0(0) win 0 <mss 1460,sackOK,nop,nop,nop,wscale 7>
>>>    +0 > S. 0:0(0) ack 1 <mss 1460,nop,nop,sackOK,nop,wscale 7>
>>>
>>>   +.1 < . 1:1(0) ack 1 win 65530
>>>    +0 accept(3, ..., ...) = 4
>>>
>>>    +0 setsockopt(4, SOL_TCP, TCP_USER_TIMEOUT, [3000], 4) = 0
>>>    +0 write(4, ..., 24) = 24
>>>    +0 > P. 1:25(24) ack 1 win 229
>>>    +.1 < . 1:1(0) ack 25 win 65530
>>>
>>> //change the ipaddress
>>>    +1 `ifconfig tun0 192.168.0.10/16`
>>>
>>>    +1 write(4, ..., 24) = 24
>>>    +1 write(4, ..., 24) = 24
>>>    +1 write(4, ..., 24) = 24
>>>    +1 write(4, ..., 24) = 24
>>>    +3 write(4, ..., 24) = 24
>>>    +3 write(4, ..., 24) = 24
>>>    +3 write(4, ..., 24) = 24
>>>    +3 write(4, ..., 24) = 24
>>>    +3 write(4, ..., 24) = 24
>>>    +3 write(4, ..., 24) = 24
>>>    +3 write(4, ..., 24) = 24
>>>    +3 write(4, ..., 24) = 24
>>>    +3 write(4, ..., 24) = 24
>>>    +3 write(4, ..., 24) = 24
>>>    +3 write(4, ..., 24) = 24
>>>    +3 write(4, ..., 24) = 24
>>>    +3 write(4, ..., 24) = 24
>>>    +3 write(4, ..., 24) = 24
>>>    +3 write(4, ..., 24) = 24
>>>    +3 write(4, ..., 24) = 24
>>>    +3 write(4, ..., 24) = 24
>>>    +3 write(4, ..., 24) = 24
>>>    +3 write(4, ..., 24) = 24
>>>    +3 write(4, ..., 24) = 24
>>>    +3 write(4, ..., 24) = 24
>>>    +3 write(4, ..., 24) = 24
>>>
>>>    +0 `ifconfig tun0 192.168.0.1/16`
>>>    +0 < . 1:1(0) ack 1 win 1000
>>>    +0 write(4, ..., 24) = -1
>>>
>>>
>>
>> This has nothing to do with the code patch you have changed.
>>
>> How have you tested your patch exactly ?
>>
I tested the patch, it can work.

[root@localhost ~]# time ./gtests/net/packetdrill/packetdrill test.pkt
test.pkt:24: runtime error in write call: Expected result 24 but got -1 with errno 110 (Connection timed out)

real	0m5.356s
user	0m0.026s
sys	0m0.104s

[root@localhost ~]# ss -toenmi src :8080
State       Recv-Q Send-Q               Local Address:Port                              Peer Address:Port
ESTAB       0      48                     192.168.0.1:8080                                 192.0.2.1:39559               timer:(persist,186ms,2) ino:37178 sk:6 <->
	 skmem:(r0,rb369280,t0,tb87040,f1792,w2304,o0,bl0) sack cubic wscale:7,7 rto:301 backoff:2 rtt:100.253/37.643 mss:1460 cwnd:10 bytes_acked:24 segs_out:5 segs_in:3 send 1.2Mbps lastsnd:976 lastrcv:4082 lastack:3982 pacing_rate 2.3Mbps rcv_space:29200


if change the TCP_USER_TIMEOUT to 30s, test result as below:

[root@localhost ~]#   time ./gtests/net/packetdrill/packetdrill test.pkt
test.pkt:37: runtime error in write call: Expected result 24 but got -1 with errno 110 (Connection timed out)

real	0m44.362s
user	0m0.018s
sys	0m0.110s

ESTAB       0      360                         192.168.0.1:8080                                      192.0.2.1:47577     timer:(persist,516ms,6) ino:18806 sk:5 <->
	 skmem:(r0,rb369280,t0,tb87040,f1792,w2304,o0,bl0) sack cubic wscale:7,7 rto:301 backoff:6 rtt:100.228/37.623 mss:10 bytes_acked:24 segs_out:22 segs_in:3 send 1.2Mbps lastsnd:2343 lastrcv:40450 lastack:40350 pacing_rate 2.3Mbps rcv_sp

> 
> lpaa23:~# ss -toenmi src :8080
> State      Recv-Q Send-Q Local Address:Port               Peer
> Address:Port              
> ESTAB      0      144    192.168.134.161:8080
> 192.0.2.1:51165               timer:(persist,8.262ms,5) ino:1
> 82083 sk:3 <->
> 	 skmem:(r0,rb359040,t0,tb46080,f1792,w2304,o0,bl0,d0) sack cubic
> wscale:7,8 rto:301 backoff:5 rtt:100.127/37.576 
> mss:1460 rcvmss:536 advmss:1460 cwnd:10 bytes_acked:24 segs_out:12
> segs_in:3 data_segs_out:12 send 1.2Mbps lastsnd:1370 l
> astrcv:13348 lastack:13248 pacing_rate 2.3Mbps delivery_rate 116.7Kbps
> app_limited busy:11346ms rcv_space:29200 notsent:1
> 44 minrtt:100.043userspace application still send data continuously
> 
> This is the typical RTO timer, not zero window probe.
> 
with the script, it is not zero window; but the code enter tcp_probe_timer.
->tcp_sendmsg
-->tcp_push
---->__tcp_push_pending_frames
------>tcp_write_xmit
-------->tcp_transmit_skb (at this function, tcp_send_head(sk)'s timestamp was be refreshed)
---------->ip_queue_xmit (can not find route, return -EHOSTUNREACH)
-------->tcp_transmit_skb (send fail, do not advance send head)
------->tcp_check_probe_timer (reset ICSK_TIME_PROBE0 timer)

Here userspace application still send data continuously,
it triggered ICSK_TIME_PROBE0 timer and tcp_send_head(sk)'s timestamp refresh.

Re: [PATCH] tcp: TCP_USER_TIMEOUT can not work in tcp_probe_timer()

[liujian]

在 2017/9/13 14:56, liujian 写道:
> 
> 
> 在 2017/9/12 23:38, Eric Dumazet 写道:
>> On Tue, 2017-09-12 at 08:05 -0700, Eric Dumazet wrote:
>>> On Tue, 2017-09-12 at 14:08 +0800, liujian wrote:
>>>> Hi,
>>>>
>>>> In the scenario, tcp server side IP changed, and at that memont,
>>>> userspace application still send data continuously;
>>>> tcp_send_head(sk)'s timestamp always be refreshed.
>>>>
>>>> Here is the packetdrill script:
>>>>
>>>>    0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
>>>>    +0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
>>>>    +0 bind(3, ..., ...) = 0
>>>>    +0 listen(3, 1) = 0
>>>>
>>>>    +0 < S 0:0(0) win 0 <mss 1460,sackOK,nop,nop,nop,wscale 7>
>>>>    +0 > S. 0:0(0) ack 1 <mss 1460,nop,nop,sackOK,nop,wscale 7>
>>>>
>>>>   +.1 < . 1:1(0) ack 1 win 65530
>>>>    +0 accept(3, ..., ...) = 4
>>>>
>>>>    +0 setsockopt(4, SOL_TCP, TCP_USER_TIMEOUT, [3000], 4) = 0
>>>>    +0 write(4, ..., 24) = 24
>>>>    +0 > P. 1:25(24) ack 1 win 229
>>>>    +.1 < . 1:1(0) ack 25 win 65530
>>>>
>>>> //change the ipaddress
>>>>    +1 `ifconfig tun0 192.168.0.10/16`
>>>>
>>>>    +1 write(4, ..., 24) = 24
>>>>    +1 write(4, ..., 24) = 24
>>>>    +1 write(4, ..., 24) = 24
>>>>    +1 write(4, ..., 24) = 24
>>>>    +3 write(4, ..., 24) = 24
>>>>    +3 write(4, ..., 24) = 24
>>>>    +3 write(4, ..., 24) = 24
>>>>    +3 write(4, ..., 24) = 24
>>>>    +3 write(4, ..., 24) = 24
>>>>    +3 write(4, ..., 24) = 24
>>>>    +3 write(4, ..., 24) = 24
>>>>    +3 write(4, ..., 24) = 24
>>>>    +3 write(4, ..., 24) = 24
>>>>    +3 write(4, ..., 24) = 24
>>>>    +3 write(4, ..., 24) = 24
>>>>    +3 write(4, ..., 24) = 24
>>>>    +3 write(4, ..., 24) = 24
>>>>    +3 write(4, ..., 24) = 24
>>>>    +3 write(4, ..., 24) = 24
>>>>    +3 write(4, ..., 24) = 24
>>>>    +3 write(4, ..., 24) = 24
>>>>    +3 write(4, ..., 24) = 24
>>>>    +3 write(4, ..., 24) = 24
>>>>    +3 write(4, ..., 24) = 24
>>>>    +3 write(4, ..., 24) = 24
>>>>    +3 write(4, ..., 24) = 24
>>>>
>>>>    +0 `ifconfig tun0 192.168.0.1/16`
>>>>    +0 < . 1:1(0) ack 1 win 1000
>>>>    +0 write(4, ..., 24) = -1
>>>>
>>>>
>>>
>>> This has nothing to do with the code patch you have changed.
>>>
>>> How have you tested your patch exactly ?
>>>
> I tested the patch, it can work.
> 
> [root@localhost ~]# time ./gtests/net/packetdrill/packetdrill test.pkt
> test.pkt:24: runtime error in write call: Expected result 24 but got -1 with errno 110 (Connection timed out)
> 
> real	0m5.356s
> user	0m0.026s
> sys	0m0.104s
> 
> [root@localhost ~]# ss -toenmi src :8080
> State       Recv-Q Send-Q               Local Address:Port                              Peer Address:Port
> ESTAB       0      48                     192.168.0.1:8080                                 192.0.2.1:39559               timer:(persist,186ms,2) ino:37178 sk:6 <->
> 	 skmem:(r0,rb369280,t0,tb87040,f1792,w2304,o0,bl0) sack cubic wscale:7,7 rto:301 backoff:2 rtt:100.253/37.643 mss:1460 cwnd:10 bytes_acked:24 segs_out:5 segs_in:3 send 1.2Mbps lastsnd:976 lastrcv:4082 lastack:3982 pacing_rate 2.3Mbps rcv_space:29200
> 
> 
> if change the TCP_USER_TIMEOUT to 30s, test result as below:
> 
> [root@localhost ~]#   time ./gtests/net/packetdrill/packetdrill test.pkt
> test.pkt:37: runtime error in write call: Expected result 24 but got -1 with errno 110 (Connection timed out)
> 
> real	0m44.362s
> user	0m0.018s
> sys	0m0.110s
> 
> ESTAB       0      360                         192.168.0.1:8080                                      192.0.2.1:47577     timer:(persist,516ms,6) ino:18806 sk:5 <->
> 	 skmem:(r0,rb369280,t0,tb87040,f1792,w2304,o0,bl0) sack cubic wscale:7,7 rto:301 backoff:6 rtt:100.228/37.623 mss:10 bytes_acked:24 segs_out:22 segs_in:3 send 1.2Mbps lastsnd:2343 lastrcv:40450 lastack:40350 pacing_rate 2.3Mbps rcv_sp
> 
>>
>> lpaa23:~# ss -toenmi src :8080
>> State      Recv-Q Send-Q Local Address:Port               Peer
>> Address:Port              
>> ESTAB      0      144    192.168.134.161:8080
>> 192.0.2.1:51165               timer:(persist,8.262ms,5) ino:1
>> 82083 sk:3 <->
>> 	 skmem:(r0,rb359040,t0,tb46080,f1792,w2304,o0,bl0,d0) sack cubic
>> wscale:7,8 rto:301 backoff:5 rtt:100.127/37.576 
>> mss:1460 rcvmss:536 advmss:1460 cwnd:10 bytes_acked:24 segs_out:12
>> segs_in:3 data_segs_out:12 send 1.2Mbps lastsnd:1370 l
>> astrcv:13348 lastack:13248 pacing_rate 2.3Mbps delivery_rate 116.7Kbps
>> app_limited busy:11346ms rcv_space:29200 notsent:1
>> 44 minrtt:100.043userspace application still send data continuously
>>
>> This is the typical RTO timer, not zero window probe.
>>
> with the script, it is not zero window; but the code enter tcp_probe_timer.
> ->tcp_sendmsg
> -->tcp_push
> ---->__tcp_push_pending_frames
> ------>tcp_write_xmit
> -------->tcp_transmit_skb (at this function, tcp_send_head(sk)'s timestamp was be refreshed)
> ---------->ip_queue_xmit (can not find route, return -EHOSTUNREACH)
> -------->tcp_transmit_skb (send fail, do not advance send head)
> ------->tcp_check_probe_timer (reset ICSK_TIME_PROBE0 timer)
sorry, change to
->tcp_sendmsg
-->tcp_push
---->__tcp_push_pending_frames
------>tcp_write_xmit
-------->tcp_transmit_skb (at this function, tcp_send_head(sk)'s timestamp was be refreshed)
---------->ip_queue_xmit (can not find route, return -EHOSTUNREACH)
------>tcp_write_xmit (send fail, do not advance send head)
------>tcp_check_probe_timer (reset ICSK_TIME_PROBE0 timer)

> Here userspace application still send data continuously,
> it triggered ICSK_TIME_PROBE0 timer and tcp_send_head(sk)'s timestamp refresh.
> 
> 

Re: [PATCH] tcp: TCP_USER_TIMEOUT can not work in tcp_probe_timer()

[Eric Dumazet]

On Wed, 2017-09-13 at 15:15 +0800, liujian wrote:
> 
> 在 2017/9/13 14:56, liujian 写道:
> > 
> > 
> > 在 2017/9/12 23:38, Eric Dumazet 写道:
> >> On Tue, 2017-09-12 at 08:05 -0700, Eric Dumazet wrote:
> >>> On Tue, 2017-09-12 at 14:08 +0800, liujian wrote:
> >>>> Hi,
> >>>>
> >>>> In the scenario, tcp server side IP changed, and at that memont,
> >>>> userspace application still send data continuously;
> >>>> tcp_send_head(sk)'s timestamp always be refreshed.
> >>>>
> >>>> Here is the packetdrill script:
> >>>>
> >>>>    0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
> >>>>    +0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
> >>>>    +0 bind(3, ..., ...) = 0
> >>>>    +0 listen(3, 1) = 0
> >>>>
> >>>>    +0 < S 0:0(0) win 0 <mss 1460,sackOK,nop,nop,nop,wscale 7>
> >>>>    +0 > S. 0:0(0) ack 1 <mss 1460,nop,nop,sackOK,nop,wscale 7>
> >>>>
> >>>>   +.1 < . 1:1(0) ack 1 win 65530
> >>>>    +0 accept(3, ..., ...) = 4
> >>>>
> >>>>    +0 setsockopt(4, SOL_TCP, TCP_USER_TIMEOUT, [3000], 4) = 0
> >>>>    +0 write(4, ..., 24) = 24
> >>>>    +0 > P. 1:25(24) ack 1 win 229
> >>>>    +.1 < . 1:1(0) ack 25 win 65530
> >>>>
> >>>> //change the ipaddress
> >>>>    +1 `ifconfig tun0 192.168.0.10/16`
> >>>>
> >>>>    +1 write(4, ..., 24) = 24
> >>>>    +1 write(4, ..., 24) = 24
> >>>>    +1 write(4, ..., 24) = 24
> >>>>    +1 write(4, ..., 24) = 24
> >>>>    +3 write(4, ..., 24) = 24
> >>>>    +3 write(4, ..., 24) = 24
> >>>>    +3 write(4, ..., 24) = 24
> >>>>    +3 write(4, ..., 24) = 24
> >>>>    +3 write(4, ..., 24) = 24
> >>>>    +3 write(4, ..., 24) = 24
> >>>>    +3 write(4, ..., 24) = 24
> >>>>    +3 write(4, ..., 24) = 24
> >>>>    +3 write(4, ..., 24) = 24
> >>>>    +3 write(4, ..., 24) = 24
> >>>>    +3 write(4, ..., 24) = 24
> >>>>    +3 write(4, ..., 24) = 24
> >>>>    +3 write(4, ..., 24) = 24
> >>>>    +3 write(4, ..., 24) = 24
> >>>>    +3 write(4, ..., 24) = 24
> >>>>    +3 write(4, ..., 24) = 24
> >>>>    +3 write(4, ..., 24) = 24
> >>>>    +3 write(4, ..., 24) = 24
> >>>>    +3 write(4, ..., 24) = 24
> >>>>    +3 write(4, ..., 24) = 24
> >>>>    +3 write(4, ..., 24) = 24
> >>>>    +3 write(4, ..., 24) = 24
> >>>>
> >>>>    +0 `ifconfig tun0 192.168.0.1/16`
> >>>>    +0 < . 1:1(0) ack 1 win 1000
> >>>>    +0 write(4, ..., 24) = -1
> >>>>
> >>>>
> >>>
> >>> This has nothing to do with the code patch you have changed.
> >>>
> >>> How have you tested your patch exactly ?
> >>>
> > I tested the patch, it can work.
> > 
> > [root@localhost ~]# time ./gtests/net/packetdrill/packetdrill test.pkt
> > test.pkt:24: runtime error in write call: Expected result 24 but got -1 with errno 110 (Connection timed out)
> > 
> > real	0m5.356s
> > user	0m0.026s
> > sys	0m0.104s
> > 
> > [root@localhost ~]# ss -toenmi src :8080
> > State       Recv-Q Send-Q               Local Address:Port                              Peer Address:Port
> > ESTAB       0      48                     192.168.0.1:8080                                 192.0.2.1:39559               timer:(persist,186ms,2) ino:37178 sk:6 <->
> > 	 skmem:(r0,rb369280,t0,tb87040,f1792,w2304,o0,bl0) sack cubic wscale:7,7 rto:301 backoff:2 rtt:100.253/37.643 mss:1460 cwnd:10 bytes_acked:24 segs_out:5 segs_in:3 send 1.2Mbps lastsnd:976 lastrcv:4082 lastack:3982 pacing_rate 2.3Mbps rcv_space:29200
> > 
> > 
> > if change the TCP_USER_TIMEOUT to 30s, test result as below:
> > 
> > [root@localhost ~]#   time ./gtests/net/packetdrill/packetdrill test.pkt
> > test.pkt:37: runtime error in write call: Expected result 24 but got -1 with errno 110 (Connection timed out)
> > 
> > real	0m44.362s
> > user	0m0.018s
> > sys	0m0.110s
> > 
> > ESTAB       0      360                         192.168.0.1:8080                                      192.0.2.1:47577     timer:(persist,516ms,6) ino:18806 sk:5 <->
> > 	 skmem:(r0,rb369280,t0,tb87040,f1792,w2304,o0,bl0) sack cubic wscale:7,7 rto:301 backoff:6 rtt:100.228/37.623 mss:10 bytes_acked:24 segs_out:22 segs_in:3 send 1.2Mbps lastsnd:2343 lastrcv:40450 lastack:40350 pacing_rate 2.3Mbps rcv_sp
> > 
> >>
> >> lpaa23:~# ss -toenmi src :8080
> >> State      Recv-Q Send-Q Local Address:Port               Peer
> >> Address:Port              
> >> ESTAB      0      144    192.168.134.161:8080
> >> 192.0.2.1:51165               timer:(persist,8.262ms,5) ino:1
> >> 82083 sk:3 <->
> >> 	 skmem:(r0,rb359040,t0,tb46080,f1792,w2304,o0,bl0,d0) sack cubic
> >> wscale:7,8 rto:301 backoff:5 rtt:100.127/37.576 
> >> mss:1460 rcvmss:536 advmss:1460 cwnd:10 bytes_acked:24 segs_out:12
> >> segs_in:3 data_segs_out:12 send 1.2Mbps lastsnd:1370 l
> >> astrcv:13348 lastack:13248 pacing_rate 2.3Mbps delivery_rate 116.7Kbps
> >> app_limited busy:11346ms rcv_space:29200 notsent:1
> >> 44 minrtt:100.043userspace application still send data continuously
> >>
> >> This is the typical RTO timer, not zero window probe.
> >>
> > with the script, it is not zero window; but the code enter tcp_probe_timer.
> > ->tcp_sendmsg
> > -->tcp_push
> > ---->__tcp_push_pending_frames
> > ------>tcp_write_xmit
> > -------->tcp_transmit_skb (at this function, tcp_send_head(sk)'s timestamp was be refreshed)
> > ---------->ip_queue_xmit (can not find route, return -EHOSTUNREACH)
> > -------->tcp_transmit_skb (send fail, do not advance send head)
> > ------->tcp_check_probe_timer (reset ICSK_TIME_PROBE0 timer)
> sorry, change to
> ->tcp_sendmsg
> -->tcp_push
> ---->__tcp_push_pending_frames
> ------>tcp_write_xmit
> -------->tcp_transmit_skb (at this function, tcp_send_head(sk)'s timestamp was be refreshed)
> ---------->ip_queue_xmit (can not find route, return -EHOSTUNREACH)
> ------>tcp_write_xmit (send fail, do not advance send head)
> ------>tcp_check_probe_timer (reset ICSK_TIME_PROBE0 timer)


OK but :

1) keepalive_time_elapsed() returns jiffies, and you compare it to a ms
value. It will break on kernels with HZ=100


2) keepalive_time_elapsed() is reset every time a packet is received.

With following packetdrill your patch (even if we fix 1)) breaks again


# ./packetdrill --local_ip 192.168.102.64 user_timeout.pkt
# cat user_timeout.pkt 

0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
   +0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
   +0 bind(3, ..., ...) = 0
   +0 listen(3, 1) = 0

   +0 `ifconfig tun0 192.168.102.64/16; ip ro add 192.0.2.1 dev tun0`

   +0 < S 0:0(0) win 0 <mss 1460,sackOK,nop,nop,nop,wscale 7>
   +0 > S. 0:0(0) ack 1 <mss 1460,nop,nop,sackOK,nop,wscale 8>

  +.1 < . 1:1(0) ack 1 win 65530
   +0 accept(3, ..., ...) = 4

   +0 setsockopt(4, SOL_TCP, TCP_USER_TIMEOUT, [3000], 4) = 0
   +0 write(4, ..., 24) = 24
   +0 > P. 1:25(24) ack 1 win 115
   +.1 < . 1:1(0) ack 25 win 65530

//change the ipaddress
   +1 `ifconfig tun0 192.168.0.10/16`

   +1 write(4, ..., 24) = 24
   +1 write(4, ..., 24) = 24
   +1 write(4, ..., 24) = 24
   +1 write(4, ..., 24) = 24

   +0 `ifconfig tun0 192.168.102.64/16`
   +0 < . 1:2(1) ack 25 win 65530
   +0 `ifconfig tun0 192.168.0.10/16`

   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24
   +3 write(4, ..., 24) = 24

   +0 `ifconfig tun0 192.168.0.1/16`
   +0 < . 1:1(0) ack 1 win 1000
   +0 write(4, ..., 24) = -1


I would instead try to keep a meaningful value in
tcp_send_head(sk)->skb_mstamp if the transmit failed.

Or more exactly change skb->skb_mstamp only on (successfully)
transmitted skb.

I will provide a patch ( fixing net/ipv4/tcp_output.c )

[PATCH net] tcp: update skb->skb_mstamp more carefully

[Eric Dumazet]

From: Eric Dumazet <edumazet@googl.com>

liujian reported a problem in TCP_USER_TIMEOUT processing with a patch
in tcp_probe_timer() :
      https://www.spinics.net/lists/netdev/msg454496.html

After investigations, the root cause of the problem is that we update
skb->skb_mstamp of skbs in write queue, even if the attempt to send a
clone or copy of it failed. One reason being a routing problem.

This patch prevents this, solving liujian issue.

It also removes a potential RTT miscalculation, since
__tcp_retransmit_skb() is not OR-ing TCP_SKB_CB(skb)->sacked with
TCPCB_EVER_RETRANS if a failure happens, but skb->skb_mstamp has
been changed.

A future ACK would then lead to a very small RTT sample and min_rtt
would then be lowered to this too small value.

Tested:

# cat user_timeout.pkt
--local_ip=192.168.102.64

    0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
   +0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
   +0 bind(3, ..., ...) = 0
   +0 listen(3, 1) = 0

   +0 `ifconfig tun0 192.168.102.64/16; ip ro add 192.0.2.1 dev tun0`

   +0 < S 0:0(0) win 0 <mss 1460>
   +0 > S. 0:0(0) ack 1 <mss 1460>

  +.1 < . 1:1(0) ack 1 win 65530
   +0 accept(3, ..., ...) = 4

   +0 setsockopt(4, SOL_TCP, TCP_USER_TIMEOUT, [3000], 4) = 0
   +0 write(4, ..., 24) = 24
   +0 > P. 1:25(24) ack 1 win 29200
   +.1 < . 1:1(0) ack 25 win 65530

//change the ipaddress
   +1 `ifconfig tun0 192.168.0.10/16`

   +1 write(4, ..., 24) = 24
   +1 write(4, ..., 24) = 24
   +1 write(4, ..., 24) = 24
   +1 write(4, ..., 24) = 24

   +0 `ifconfig tun0 192.168.102.64/16`
   +0 < . 1:2(1) ack 25 win 65530
   +0 `ifconfig tun0 192.168.0.10/16`

   +3 write(4, ..., 24) = -1

# ./packetdrill user_timeout.pkt

Signed-off-by: Eric Dumazet <edumazet@googl.com>
Reported-by: liujian <liujian56@huawei.com>
---
 net/ipv4/tcp_output.c |   19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 5b6690d05abb98884adfa1693f97d896dd202893..a85a8c2948e54b931f8cd956aa7938f7efd355bd 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -991,6 +991,7 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
 	struct tcp_skb_cb *tcb;
 	struct tcp_out_options opts;
 	unsigned int tcp_options_size, tcp_header_size;
+	struct sk_buff *oskb = NULL;
 	struct tcp_md5sig_key *md5;
 	struct tcphdr *th;
 	int err;
@@ -998,12 +999,12 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
 	BUG_ON(!skb || !tcp_skb_pcount(skb));
 	tp = tcp_sk(sk);
 
-	skb->skb_mstamp = tp->tcp_mstamp;
 	if (clone_it) {
 		TCP_SKB_CB(skb)->tx.in_flight = TCP_SKB_CB(skb)->end_seq
 			- tp->snd_una;
 		tcp_rate_skb_sent(sk, skb);
 
+		oskb = skb;
 		if (unlikely(skb_cloned(skb)))
 			skb = pskb_copy(skb, gfp_mask);
 		else
@@ -1011,6 +1012,7 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
 		if (unlikely(!skb))
 			return -ENOBUFS;
 	}
+	skb->skb_mstamp = tp->tcp_mstamp;
 
 	inet = inet_sk(sk);
 	tcb = TCP_SKB_CB(skb);
@@ -1122,12 +1124,14 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
 
 	err = icsk->icsk_af_ops->queue_xmit(sk, skb, &inet->cork.fl);
 
-	if (likely(err <= 0))
-		return err;
-
-	tcp_enter_cwr(sk);
+	if (unlikely(err > 0)) {
+		tcp_enter_cwr(sk);
+		err = net_xmit_eval(err);
+	}
+	if (!err && oskb)
+		oskb->skb_mstamp = tp->tcp_mstamp;
 
-	return net_xmit_eval(err);
+	return err;
 }
 
 /* This routine just queues the buffer for sending.
@@ -2869,10 +2873,11 @@ int __tcp_retransmit_skb(struct sock *sk, struct sk_buff *skb, int segs)
 		     skb_headroom(skb) >= 0xFFFF)) {
 		struct sk_buff *nskb;
 
-		skb->skb_mstamp = tp->tcp_mstamp;
 		nskb = __pskb_copy(skb, MAX_TCP_HEADER, GFP_ATOMIC);
 		err = nskb ? tcp_transmit_skb(sk, nskb, 0, GFP_ATOMIC) :
 			     -ENOBUFS;
+		if (!err)
+			skb->skb_mstamp = tp->tcp_mstamp;
 	} else {
 		err = tcp_transmit_skb(sk, skb, 1, GFP_ATOMIC);
 	}

Re: [PATCH net] tcp: update skb->skb_mstamp more carefully

[liujian]

On 2017/9/14 11:30, Eric Dumazet worte:
> From: Eric Dumazet <edumazet@googl.com>
> 
> liujian reported a problem in TCP_USER_TIMEOUT processing with a patch
> in tcp_probe_timer() :
>       https://www.spinics.net/lists/netdev/msg454496.html
> 
> After investigations, the root cause of the problem is that we update
> skb->skb_mstamp of skbs in write queue, even if the attempt to send a
> clone or copy of it failed. One reason being a routing problem.
> 
> This patch prevents this, solving liujian issue.
> 
> It also removes a potential RTT miscalculation, since
> __tcp_retransmit_skb() is not OR-ing TCP_SKB_CB(skb)->sacked with
> TCPCB_EVER_RETRANS if a failure happens, but skb->skb_mstamp has
> been changed.
> 
> A future ACK would then lead to a very small RTT sample and min_rtt
> would then be lowered to this too small value.
> 
>

I test  on 4.13, it can work.
thank you!

Re: [PATCH net] tcp: update skb->skb_mstamp more carefully

[Neal Cardwell]

On Wed, Sep 13, 2017 at 11:30 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> From: Eric Dumazet <edumazet@googl.com>
>
> liujian reported a problem in TCP_USER_TIMEOUT processing with a patch
> in tcp_probe_timer() :
>       https://www.spinics.net/lists/netdev/msg454496.html
>
> After investigations, the root cause of the problem is that we update
> skb->skb_mstamp of skbs in write queue, even if the attempt to send a
> clone or copy of it failed. One reason being a routing problem.
>
> This patch prevents this, solving liujian issue.
>
> It also removes a potential RTT miscalculation, since
> __tcp_retransmit_skb() is not OR-ing TCP_SKB_CB(skb)->sacked with
> TCPCB_EVER_RETRANS if a failure happens, but skb->skb_mstamp has
> been changed.
>
> A future ACK would then lead to a very small RTT sample and min_rtt
> would then be lowered to this too small value.
...
>
> Signed-off-by: Eric Dumazet <edumazet@googl.com>
> Reported-by: liujian <liujian56@huawei.com>
> ---
>  net/ipv4/tcp_output.c |   19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)

Acked-by: Neal Cardwell <ncardwell@google.com>

Thanks, Eric!

neal

Re: [PATCH net] tcp: update skb->skb_mstamp more carefully

[Yuchung Cheng]

On Thu, Sep 14, 2017 at 6:57 AM, Neal Cardwell <ncardwell@google.com> wrote:
> On Wed, Sep 13, 2017 at 11:30 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>
>> From: Eric Dumazet <edumazet@googl.com>
>>
>> liujian reported a problem in TCP_USER_TIMEOUT processing with a patch
>> in tcp_probe_timer() :
>>       https://www.spinics.net/lists/netdev/msg454496.html
>>
>> After investigations, the root cause of the problem is that we update
>> skb->skb_mstamp of skbs in write queue, even if the attempt to send a
>> clone or copy of it failed. One reason being a routing problem.
>>
>> This patch prevents this, solving liujian issue.
>>
>> It also removes a potential RTT miscalculation, since
>> __tcp_retransmit_skb() is not OR-ing TCP_SKB_CB(skb)->sacked with
>> TCPCB_EVER_RETRANS if a failure happens, but skb->skb_mstamp has
>> been changed.
>>
>> A future ACK would then lead to a very small RTT sample and min_rtt
>> would then be lowered to this too small value.
> ...
>>
>> Signed-off-by: Eric Dumazet <edumazet@googl.com>
>> Reported-by: liujian <liujian56@huawei.com>
>> ---
>>  net/ipv4/tcp_output.c |   19 ++++++++++++-------
>>  1 file changed, 12 insertions(+), 7 deletions(-)
>
> Acked-by: Neal Cardwell <ncardwell@google.com>
Acked-by: Yuchung Cheng <ycheng@google.com>

nicely done!

>
> Thanks, Eric!
>
> neal

Re: [PATCH net] tcp: update skb->skb_mstamp more carefully

[Soheil Hassas Yeganeh]

On Thu, Sep 14, 2017 at 12:32 PM, Yuchung Cheng <ycheng@google.com> wrote:
> On Thu, Sep 14, 2017 at 6:57 AM, Neal Cardwell <ncardwell@google.com> wrote:
>> On Wed, Sep 13, 2017 at 11:30 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>>
>>> From: Eric Dumazet <edumazet@googl.com>
>>>
>>> liujian reported a problem in TCP_USER_TIMEOUT processing with a patch
>>> in tcp_probe_timer() :
>>>       https://www.spinics.net/lists/netdev/msg454496.html
>>>
>>> After investigations, the root cause of the problem is that we update
>>> skb->skb_mstamp of skbs in write queue, even if the attempt to send a
>>> clone or copy of it failed. One reason being a routing problem.
>>>
>>> This patch prevents this, solving liujian issue.
>>>
>>> It also removes a potential RTT miscalculation, since
>>> __tcp_retransmit_skb() is not OR-ing TCP_SKB_CB(skb)->sacked with
>>> TCPCB_EVER_RETRANS if a failure happens, but skb->skb_mstamp has
>>> been changed.
>>>
>>> A future ACK would then lead to a very small RTT sample and min_rtt
>>> would then be lowered to this too small value.
>> ...
>>>
>>> Signed-off-by: Eric Dumazet <edumazet@googl.com>
>>> Reported-by: liujian <liujian56@huawei.com>
>>> ---
>>>  net/ipv4/tcp_output.c |   19 ++++++++++++-------
>>>  1 file changed, 12 insertions(+), 7 deletions(-)
>>
>> Acked-by: Neal Cardwell <ncardwell@google.com>
> Acked-by: Yuchung Cheng <ycheng@google.com>

Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
Very nice! Thank you, Eric!

Re: [PATCH net] tcp: update skb->skb_mstamp more carefully

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 13 Sep 2017 20:30:39 -0700

> From: Eric Dumazet <edumazet@googl.com>
> 
> liujian reported a problem in TCP_USER_TIMEOUT processing with a patch
> in tcp_probe_timer() :
>       https://www.spinics.net/lists/netdev/msg454496.html
> 
> After investigations, the root cause of the problem is that we update
> skb->skb_mstamp of skbs in write queue, even if the attempt to send a
> clone or copy of it failed. One reason being a routing problem.
> 
> This patch prevents this, solving liujian issue.
> 
> It also removes a potential RTT miscalculation, since
> __tcp_retransmit_skb() is not OR-ing TCP_SKB_CB(skb)->sacked with
> TCPCB_EVER_RETRANS if a failure happens, but skb->skb_mstamp has
> been changed.
> 
> A future ACK would then lead to a very small RTT sample and min_rtt
> would then be lowered to this too small value.
> 
> Tested:
...
> Signed-off-by: Eric Dumazet <edumazet@googl.com>
> Reported-by: liujian <liujian56@huawei.com>

Applied and queued up for -stable, thanks.

Re: [PATCH net] tcp: update skb->skb_mstamp more carefully

[Eric Dumazet]

On Fri, 2017-09-15 at 14:37 -0700, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Wed, 13 Sep 2017 20:30:39 -0700
> 
> > From: Eric Dumazet <edumazet@googl.com>
> > 
> > liujian reported a problem in TCP_USER_TIMEOUT processing with a patch
> > in tcp_probe_timer() :
> >       https://www.spinics.net/lists/netdev/msg454496.html
> > 
> > After investigations, the root cause of the problem is that we update
> > skb->skb_mstamp of skbs in write queue, even if the attempt to send a
> > clone or copy of it failed. One reason being a routing problem.
> > 
> > This patch prevents this, solving liujian issue.
> > 
> > It also removes a potential RTT miscalculation, since
> > __tcp_retransmit_skb() is not OR-ing TCP_SKB_CB(skb)->sacked with
> > TCPCB_EVER_RETRANS if a failure happens, but skb->skb_mstamp has
> > been changed.
> > 
> > A future ACK would then lead to a very small RTT sample and min_rtt
> > would then be lowered to this too small value.
> > 
> > Tested:
> ...
> > Signed-off-by: Eric Dumazet <edumazet@googl.com>
> > Reported-by: liujian <liujian56@huawei.com>
> 
> Applied and queued up for -stable, thanks.

Thanks David.

It seems I forgot to move the call to tcp_rate_skb_sent(sk, skb),
as this function depended on skb->skb_mstamp being up to date.

I will send a fix, sorry for not catching this earlier.

[PATCH net] tcp: fix data delivery rate

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

Now skb->mstamp_skb is updated later, we also need to call
tcp_rate_skb_sent() after the update is done.

Fixes: 8c72c65b426b ("tcp: update skb->skb_mstamp more carefully")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/tcp_output.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index a85a8c2948e54b931f8cd956aa7938f7efd355bd..1c839c99114cd22bbfbd181cf702acccb3aeb61b 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1002,8 +1002,6 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
 	if (clone_it) {
 		TCP_SKB_CB(skb)->tx.in_flight = TCP_SKB_CB(skb)->end_seq
 			- tp->snd_una;
-		tcp_rate_skb_sent(sk, skb);
-
 		oskb = skb;
 		if (unlikely(skb_cloned(skb)))
 			skb = pskb_copy(skb, gfp_mask);
@@ -1128,9 +1126,10 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
 		tcp_enter_cwr(sk);
 		err = net_xmit_eval(err);
 	}
-	if (!err && oskb)
+	if (!err && oskb) {
 		oskb->skb_mstamp = tp->tcp_mstamp;
-
+		tcp_rate_skb_sent(sk, oskb);
+	}
 	return err;
 }
 

Re: [PATCH net] tcp: fix data delivery rate

[Soheil Hassas Yeganeh]

On Fri, Sep 15, 2017 at 7:47 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> Now skb->mstamp_skb is updated later, we also need to call
> tcp_rate_skb_sent() after the update is done.
>
> Fixes: 8c72c65b426b ("tcp: update skb->skb_mstamp more carefully")
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Acked-by: Soheil Hassas Yeganeh <soheil@google.com>

Nice catch!

Re: [PATCH net] tcp: fix data delivery rate

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 15 Sep 2017 16:47:42 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> Now skb->mstamp_skb is updated later, we also need to call
> tcp_rate_skb_sent() after the update is done.
> 
> Fixes: 8c72c65b426b ("tcp: update skb->skb_mstamp more carefully")
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied, thanks Eric.