TCP keepalive question

TCP keepalive question

[Flavio Leitner]

[-- Attachment #1: Type: text/plain, Size: 700 bytes --]


Hi,


I'm reading the RFC 1122 and it says the following:
...
           Keep-alive packets MUST only be sent when no data or
           acknowledgement packets have been received for the
           connection within an interval.  
...

The receiving acknowledgement part seems to be ok and handled 
by tcp_keepalive_timer() when it does 
elapsed = tcp_time_stamp - tp->rcv_tstamp;

However, if one side just receive data and reply with ACK, the
keepalive probes is sent anyway - 2.6.32.9-70.fc12.i686.PAE.

Any reason to not reset keepalive timer when data is received?

Socket options used:
SO_KEEPALIVE, TCP_KEEPIDLE=40, TCP_KEEPCNT=6, TCP_KEEPINTVL=5

Traffic dump attached.

thanks!
-- 
Flavio

[-- Attachment #2: keepalive.pcap.gz --]
[-- Type: application/octet-stream, Size: 552 bytes --]

[PATCH] TCP: avoid to send keepalive probes if it is receiving data

[Flavio Leitner]

RFC 1122 says the following:
...
  Keep-alive packets MUST only be sent when no data or
  acknowledgement packets have been received for the
  connection within an interval.
...

Fix this by storing the timestamp of last received data
packet and checking for it when the keepalive timer expires.

Signed-off-by: Flavio Leitner <fleitner@redhat.com>
---
 include/linux/tcp.h  |    1 +
 net/ipv4/tcp_input.c |    3 +++
 net/ipv4/tcp_timer.c |    8 ++++++++
 3 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index a778ee0..405678f 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -314,6 +314,7 @@ struct tcp_sock {
  	u32	snd_sml;	/* Last byte of the most recently transmitted small packet */
 	u32	rcv_tstamp;	/* timestamp of last received ACK (for keepalives) */
 	u32	lsndtime;	/* timestamp of last sent data packet (for restart window) */
+	u32	lrcvtime;	/* timestamp of last received data packet (for keepalives) */
 
 	/* Data for direct copy to user */
 	struct {
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index f240f57..60d2980 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -5391,6 +5391,8 @@ no_ack:
 				__kfree_skb(skb);
 			else
 				sk->sk_data_ready(sk, 0);
+
+			tp->lrcvtime = tcp_time_stamp;
 			return 0;
 		}
 	}
@@ -5421,6 +5423,7 @@ step5:
 
 	tcp_data_snd_check(sk);
 	tcp_ack_snd_check(sk);
+	tp->lrcvtime = tcp_time_stamp;
 	return 0;
 
 csum_error:
diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index 8a0ab29..74dd804 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -554,6 +554,14 @@ static void tcp_keepalive_timer (unsigned long data)
 	if (tp->packets_out || tcp_send_head(sk))
 		goto resched;
 
+	elapsed = tcp_time_stamp - tp->lrcvtime;
+	
+	/* receiving data means alive */
+	if (elapsed < keepalive_time_when(tp)) {
+		elapsed = keepalive_time_when(tp) - elapsed;
+		goto resched;
+	}
+
 	elapsed = tcp_time_stamp - tp->rcv_tstamp;
 
 	if (elapsed >= keepalive_time_when(tp)) {
-- 
1.6.6.1

Re: [PATCH] TCP: avoid to send keepalive probes if it is receiving data

[Eric Dumazet]

Le samedi 17 avril 2010 à 14:28 -0300, Flavio Leitner a écrit :
> RFC 1122 says the following:
> ...
>   Keep-alive packets MUST only be sent when no data or
>   acknowledgement packets have been received for the
>   connection within an interval.
> ...
> 
> Fix this by storing the timestamp of last received data
> packet and checking for it when the keepalive timer expires.
> 
> Signed-off-by: Flavio Leitner <fleitner@redhat.com>

Thanks Flavio !

Shouldnt you also change do_tcp_setsockopt() TCP_KEEPIDLE for
consistency ?

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 0f8caf6..a4048d7 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2298,7 +2298,10 @@ static int do_tcp_setsockopt(struct sock *sk, int level,
 			if (sock_flag(sk, SOCK_KEEPOPEN) &&
 			    !((1 << sk->sk_state) &
 			      (TCPF_CLOSE | TCPF_LISTEN))) {
-				__u32 elapsed = tcp_time_stamp - tp->rcv_tstamp;
+				u32 elapsed = min_t(u32,
+						      tcp_time_stamp - tp->rcv_tstamp,
+						      tcp_time_stamp - tp->lrcvtime);
+
 				if (tp->keepalive_time > elapsed)
 					elapsed = tp->keepalive_time - elapsed;
 				else






> ---
>  include/linux/tcp.h  |    1 +
>  net/ipv4/tcp_input.c |    3 +++
>  net/ipv4/tcp_timer.c |    8 ++++++++
>  3 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/tcp.h b/include/linux/tcp.h
> index a778ee0..405678f 100644
> --- a/include/linux/tcp.h
> +++ b/include/linux/tcp.h
> @@ -314,6 +314,7 @@ struct tcp_sock {
>   	u32	snd_sml;	/* Last byte of the most recently transmitted small packet */
>  	u32	rcv_tstamp;	/* timestamp of last received ACK (for keepalives) */
>  	u32	lsndtime;	/* timestamp of last sent data packet (for restart window) */
> +	u32	lrcvtime;	/* timestamp of last received data packet (for keepalives) */
>  
>  	/* Data for direct copy to user */
>  	struct {
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index f240f57..60d2980 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -5391,6 +5391,8 @@ no_ack:
>  				__kfree_skb(skb);
>  			else
>  				sk->sk_data_ready(sk, 0);
> +
> +			tp->lrcvtime = tcp_time_stamp;
>  			return 0;
>  		}
>  	}
> @@ -5421,6 +5423,7 @@ step5:
>  
>  	tcp_data_snd_check(sk);
>  	tcp_ack_snd_check(sk);
> +	tp->lrcvtime = tcp_time_stamp;
>  	return 0;
>  
>  csum_error:
> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
> index 8a0ab29..74dd804 100644
> --- a/net/ipv4/tcp_timer.c
> +++ b/net/ipv4/tcp_timer.c
> @@ -554,6 +554,14 @@ static void tcp_keepalive_timer (unsigned long data)
>  	if (tp->packets_out || tcp_send_head(sk))
>  		goto resched;
>  
> +	elapsed = tcp_time_stamp - tp->lrcvtime;
> +	
> +	/* receiving data means alive */
> +	if (elapsed < keepalive_time_when(tp)) {
> +		elapsed = keepalive_time_when(tp) - elapsed;
> +		goto resched;
> +	}
> +
>  	elapsed = tcp_time_stamp - tp->rcv_tstamp;
>  
>  	if (elapsed >= keepalive_time_when(tp)) {

[PATCH v2] TCP: avoid to send keepalive probes if it is receiving data

[Flavio Leitner]

RFC 1122 says the following:
...
  Keep-alive packets MUST only be sent when no data or
  acknowledgement packets have been received for the
  connection within an interval.
...

Fix this by storing the timestamp of last received data
packet and checking for it when the keepalive timer expires.

-v2 fix do_tcp_setsockopt() as pointed by Eric Dumazet <eric.dumazet@gmail.com>

Signed-off-by: Flavio Leitner <fleitner@redhat.com>
---
 include/linux/tcp.h  |    1 +
 net/ipv4/tcp.c       |    5 ++++-
 net/ipv4/tcp_input.c |    3 +++
 net/ipv4/tcp_timer.c |    8 ++++++++
 4 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index a778ee0..405678f 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -314,6 +314,7 @@ struct tcp_sock {
  	u32	snd_sml;	/* Last byte of the most recently transmitted small packet */
 	u32	rcv_tstamp;	/* timestamp of last received ACK (for keepalives) */
 	u32	lsndtime;	/* timestamp of last sent data packet (for restart window) */
+	u32	lrcvtime;	/* timestamp of last received data packet (for keepalives) */
 
 	/* Data for direct copy to user */
 	struct {
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 0f8caf6..a4048d7 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2298,7 +2298,10 @@ static int do_tcp_setsockopt(struct sock *sk, int level,
 			if (sock_flag(sk, SOCK_KEEPOPEN) &&
 			    !((1 << sk->sk_state) &
 			      (TCPF_CLOSE | TCPF_LISTEN))) {
-				__u32 elapsed = tcp_time_stamp - tp->rcv_tstamp;
+				u32 elapsed = min_t(u32,
+						      tcp_time_stamp - tp->rcv_tstamp,
+						      tcp_time_stamp - tp->lrcvtime);
+
 				if (tp->keepalive_time > elapsed)
 					elapsed = tp->keepalive_time - elapsed;
 				else
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index f240f57..60d2980 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -5391,6 +5391,8 @@ no_ack:
 				__kfree_skb(skb);
 			else
 				sk->sk_data_ready(sk, 0);
+
+			tp->lrcvtime = tcp_time_stamp;
 			return 0;
 		}
 	}
@@ -5421,6 +5423,7 @@ step5:
 
 	tcp_data_snd_check(sk);
 	tcp_ack_snd_check(sk);
+	tp->lrcvtime = tcp_time_stamp;
 	return 0;
 
 csum_error:
diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index 8a0ab29..74dd804 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -554,6 +554,14 @@ static void tcp_keepalive_timer (unsigned long data)
 	if (tp->packets_out || tcp_send_head(sk))
 		goto resched;
 
+	elapsed = tcp_time_stamp - tp->lrcvtime;
+	
+	/* receiving data means alive */
+	if (elapsed < keepalive_time_when(tp)) {
+		elapsed = keepalive_time_when(tp) - elapsed;
+		goto resched;
+	}
+
 	elapsed = tcp_time_stamp - tp->rcv_tstamp;
 
 	if (elapsed >= keepalive_time_when(tp)) {
-- 
1.5.5.6

Re: [PATCH v2] TCP: avoid to send keepalive probes if it is receiving data

[Eric Dumazet]

Le dimanche 18 avril 2010 à 11:55 -0300, Flavio Leitner a écrit :
> RFC 1122 says the following:
> ...
>   Keep-alive packets MUST only be sent when no data or
>   acknowledgement packets have been received for the
>   connection within an interval.
> ...
> 
> Fix this by storing the timestamp of last received data
> packet and checking for it when the keepalive timer expires.
> 
> -v2 fix do_tcp_setsockopt() as pointed by Eric Dumazet <eric.dumazet@gmail.com>
> 
> Signed-off-by: Flavio Leitner <fleitner@redhat.com>


I find this patch very welcome, and we could easily use this new
lrcvtime information available in diagnostic tools (ss command)

But are you sure you update it for all valid packets ?

If we receive a pure ACK, it seems you do not ...



> ---
>  include/linux/tcp.h  |    1 +
>  net/ipv4/tcp.c       |    5 ++++-
>  net/ipv4/tcp_input.c |    3 +++
>  net/ipv4/tcp_timer.c |    8 ++++++++
>  4 files changed, 16 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/tcp.h b/include/linux/tcp.h
> index a778ee0..405678f 100644
> --- a/include/linux/tcp.h
> +++ b/include/linux/tcp.h
> @@ -314,6 +314,7 @@ struct tcp_sock {
>   	u32	snd_sml;	/* Last byte of the most recently transmitted small packet */
>  	u32	rcv_tstamp;	/* timestamp of last received ACK (for keepalives) */
>  	u32	lsndtime;	/* timestamp of last sent data packet (for restart window) */
> +	u32	lrcvtime;	/* timestamp of last received data packet (for keepalives) */
>  
>  	/* Data for direct copy to user */
>  	struct {
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 0f8caf6..a4048d7 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -2298,7 +2298,10 @@ static int do_tcp_setsockopt(struct sock *sk, int level,
>  			if (sock_flag(sk, SOCK_KEEPOPEN) &&
>  			    !((1 << sk->sk_state) &
>  			      (TCPF_CLOSE | TCPF_LISTEN))) {
> -				__u32 elapsed = tcp_time_stamp - tp->rcv_tstamp;
> +				u32 elapsed = min_t(u32,
> +						      tcp_time_stamp - tp->rcv_tstamp,
> +						      tcp_time_stamp - tp->lrcvtime);
> +
>  				if (tp->keepalive_time > elapsed)
>  					elapsed = tp->keepalive_time - elapsed;
>  				else
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index f240f57..60d2980 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -5391,6 +5391,8 @@ no_ack:
>  				__kfree_skb(skb);
>  			else
>  				sk->sk_data_ready(sk, 0);
> +
> +			tp->lrcvtime = tcp_time_stamp;
>  			return 0;
>  		}
>  	}
> @@ -5421,6 +5423,7 @@ step5:
>  
>  	tcp_data_snd_check(sk);
>  	tcp_ack_snd_check(sk);
> +	tp->lrcvtime = tcp_time_stamp;
>  	return 0;
>  
>  csum_error:
> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
> index 8a0ab29..74dd804 100644
> --- a/net/ipv4/tcp_timer.c
> +++ b/net/ipv4/tcp_timer.c
> @@ -554,6 +554,14 @@ static void tcp_keepalive_timer (unsigned long data)
>  	if (tp->packets_out || tcp_send_head(sk))
>  		goto resched;
>  
> +	elapsed = tcp_time_stamp - tp->lrcvtime;
> +	
> +	/* receiving data means alive */
> +	if (elapsed < keepalive_time_when(tp)) {
> +		elapsed = keepalive_time_when(tp) - elapsed;
> +		goto resched;
> +	}
> +
>  	elapsed = tcp_time_stamp - tp->rcv_tstamp;
>  
>  	if (elapsed >= keepalive_time_when(tp)) {

Re: [PATCH v2] TCP: avoid to send keepalive probes if it is receiving data

[Ilpo Järvinen]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3631 bytes --]

On Sun, 18 Apr 2010, Eric Dumazet wrote:

> Le dimanche 18 avril 2010 à 11:55 -0300, Flavio Leitner a écrit :
> > RFC 1122 says the following:
> > ...
> >   Keep-alive packets MUST only be sent when no data or
> >   acknowledgement packets have been received for the
> >   connection within an interval.
> > ...
> > 
> > Fix this by storing the timestamp of last received data
> > packet and checking for it when the keepalive timer expires.
> > 
> > -v2 fix do_tcp_setsockopt() as pointed by Eric Dumazet <eric.dumazet@gmail.com>
> > 
> > Signed-off-by: Flavio Leitner <fleitner@redhat.com>
> 
> 
> I find this patch very welcome, and we could easily use this new
> lrcvtime information available in diagnostic tools (ss command)
> 
> But are you sure you update it for all valid packets ?
> 
> If we receive a pure ACK, it seems you do not ...

I fail to see why the addition of this new variable is necessary at all, 
could either of you enlight me why exactly it's necessary and rcv_tstamp 
will not suffice?

> > ---
> >  include/linux/tcp.h  |    1 +
> >  net/ipv4/tcp.c       |    5 ++++-
> >  net/ipv4/tcp_input.c |    3 +++
> >  net/ipv4/tcp_timer.c |    8 ++++++++
> >  4 files changed, 16 insertions(+), 1 deletions(-)
> > 
> > diff --git a/include/linux/tcp.h b/include/linux/tcp.h
> > index a778ee0..405678f 100644
> > --- a/include/linux/tcp.h
> > +++ b/include/linux/tcp.h
> > @@ -314,6 +314,7 @@ struct tcp_sock {
> >   	u32	snd_sml;	/* Last byte of the most recently transmitted small packet */
> >  	u32	rcv_tstamp;	/* timestamp of last received ACK (for keepalives) */
> >  	u32	lsndtime;	/* timestamp of last sent data packet (for restart window) */
> > +	u32	lrcvtime;	/* timestamp of last received data packet (for keepalives) */
> >  
> >  	/* Data for direct copy to user */
> >  	struct {
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > index 0f8caf6..a4048d7 100644
> > --- a/net/ipv4/tcp.c
> > +++ b/net/ipv4/tcp.c
> > @@ -2298,7 +2298,10 @@ static int do_tcp_setsockopt(struct sock *sk, int level,
> >  			if (sock_flag(sk, SOCK_KEEPOPEN) &&
> >  			    !((1 << sk->sk_state) &
> >  			      (TCPF_CLOSE | TCPF_LISTEN))) {
> > -				__u32 elapsed = tcp_time_stamp - tp->rcv_tstamp;
> > +				u32 elapsed = min_t(u32,
> > +						      tcp_time_stamp - tp->rcv_tstamp,
> > +						      tcp_time_stamp - tp->lrcvtime);
> > +
> >  				if (tp->keepalive_time > elapsed)
> >  					elapsed = tp->keepalive_time - elapsed;
> >  				else
> > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > index f240f57..60d2980 100644
> > --- a/net/ipv4/tcp_input.c
> > +++ b/net/ipv4/tcp_input.c
> > @@ -5391,6 +5391,8 @@ no_ack:
> >  				__kfree_skb(skb);
> >  			else
> >  				sk->sk_data_ready(sk, 0);
> > +
> > +			tp->lrcvtime = tcp_time_stamp;
> >  			return 0;
> >  		}
> >  	}
> > @@ -5421,6 +5423,7 @@ step5:
> >  
> >  	tcp_data_snd_check(sk);
> >  	tcp_ack_snd_check(sk);
> > +	tp->lrcvtime = tcp_time_stamp;
> >  	return 0;
> >  
> >  csum_error:
> > diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
> > index 8a0ab29..74dd804 100644
> > --- a/net/ipv4/tcp_timer.c
> > +++ b/net/ipv4/tcp_timer.c
> > @@ -554,6 +554,14 @@ static void tcp_keepalive_timer (unsigned long data)
> >  	if (tp->packets_out || tcp_send_head(sk))
> >  		goto resched;
> >  
> > +	elapsed = tcp_time_stamp - tp->lrcvtime;
> > +	
> > +	/* receiving data means alive */
> > +	if (elapsed < keepalive_time_when(tp)) {
> > +		elapsed = keepalive_time_when(tp) - elapsed;
> > +		goto resched;
> > +	}
> > +
> >  	elapsed = tcp_time_stamp - tp->rcv_tstamp;
> >  
> >  	if (elapsed >= keepalive_time_when(tp)) {


-- 
 i.

Re: [PATCH v2] TCP: avoid to send keepalive probes if it is receiving data

[David Miller]

From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Sun, 18 Apr 2010 23:34:15 +0300 (EEST)

> I fail to see why the addition of this new variable is necessary at all, 
> could either of you enlight me why exactly it's necessary and rcv_tstamp 
> will not suffice?

I agree, the existing rcv_tstamp should serve this purpose just
fine.

Re: [PATCH v2] TCP: avoid to send keepalive probes if it is receiving data

[Flavio Leitner]

On Wed, Apr 21, 2010 at 10:42:05PM -0700, David Miller wrote:
> From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
> Date: Sun, 18 Apr 2010 23:34:15 +0300 (EEST)
> 
> > I fail to see why the addition of this new variable is necessary at all, 
> > could either of you enlight me why exactly it's necessary and rcv_tstamp 
> > will not suffice?
> 
> I agree, the existing rcv_tstamp should serve this purpose just
> fine.

I thought it would break TCP_INFO/tcp_get_info(). Actually, reviewing
the code again I found another variable which does exactly what is needed.

I'll post a new patch for review.

thanks,
-- 
Flavio

[PATCH v3] TCP: avoid to send keepalive probes if receiving data

[Flavio Leitner]

RFC 1122 says the following:
...
  Keep-alive packets MUST only be sent when no data or
  acknowledgement packets have been received for the
  connection within an interval.
...

The acknowledgement packet is reseting the keepalive
timer but the data packet isn't. This patch fixes it by
checking the timestamp of the last received data packet
too when the keepalive timer expires.

Signed-off-by: Flavio Leitner <fleitner@redhat.com>
---
 net/ipv4/tcp.c       |    5 ++++-
 net/ipv4/tcp_timer.c |    8 ++++++++
 2 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 0f8caf6..94f605c 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2298,7 +2298,10 @@ static int do_tcp_setsockopt(struct sock *sk, int level,
 			if (sock_flag(sk, SOCK_KEEPOPEN) &&
 			    !((1 << sk->sk_state) &
 			      (TCPF_CLOSE | TCPF_LISTEN))) {
-				__u32 elapsed = tcp_time_stamp - tp->rcv_tstamp;
+				u32 elapsed = min_t(u32,
+				      tcp_time_stamp - icsk->icsk_ack.lrcvtime,
+				      tcp_time_stamp - tp->rcv_tstamp);
+
 				if (tp->keepalive_time > elapsed)
 					elapsed = tp->keepalive_time - elapsed;
 				else
diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index 8a0ab29..9d1bb6f 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -554,6 +554,14 @@ static void tcp_keepalive_timer (unsigned long data)
 	if (tp->packets_out || tcp_send_head(sk))
 		goto resched;
 
+	elapsed = tcp_time_stamp - icsk->icsk_ack.lrcvtime;
+
+	/* receiving data means alive */
+	if (elapsed < keepalive_time_when(tp)) {
+		elapsed = keepalive_time_when(tp) - elapsed;
+		goto resched;
+	}
+
 	elapsed = tcp_time_stamp - tp->rcv_tstamp;
 
 	if (elapsed >= keepalive_time_when(tp)) {
-- 
1.5.5.6

Re: [PATCH v3] TCP: avoid to send keepalive probes if receiving data

[Ilpo Järvinen]

On Sun, 25 Apr 2010, Flavio Leitner wrote:

> RFC 1122 says the following:
> ...
>   Keep-alive packets MUST only be sent when no data or
>   acknowledgement packets have been received for the
>   connection within an interval.
> ...
> 
> The acknowledgement packet is reseting the keepalive
> timer but the data packet isn't. This patch fixes it by
> checking the timestamp of the last received data packet
> too when the keepalive timer expires.
> 
> Signed-off-by: Flavio Leitner <fleitner@redhat.com>
> ---
>  net/ipv4/tcp.c       |    5 ++++-
>  net/ipv4/tcp_timer.c |    8 ++++++++
>  2 files changed, 12 insertions(+), 1 deletions(-)
> 
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 0f8caf6..94f605c 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -2298,7 +2298,10 @@ static int do_tcp_setsockopt(struct sock *sk, int level,
>  			if (sock_flag(sk, SOCK_KEEPOPEN) &&
>  			    !((1 << sk->sk_state) &
>  			      (TCPF_CLOSE | TCPF_LISTEN))) {
> -				__u32 elapsed = tcp_time_stamp - tp->rcv_tstamp;
> +				u32 elapsed = min_t(u32,
> +				      tcp_time_stamp - icsk->icsk_ack.lrcvtime,
> +				      tcp_time_stamp - tp->rcv_tstamp);
> +

I'd put this into a helper in include/net/tcp.h

>  				if (tp->keepalive_time > elapsed)
>  					elapsed = tp->keepalive_time - elapsed;
>  				else
> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
> index 8a0ab29..9d1bb6f 100644
> --- a/net/ipv4/tcp_timer.c
> +++ b/net/ipv4/tcp_timer.c
> @@ -554,6 +554,14 @@ static void tcp_keepalive_timer (unsigned long data)
>  	if (tp->packets_out || tcp_send_head(sk))
>  		goto resched;
>  
> +	elapsed = tcp_time_stamp - icsk->icsk_ack.lrcvtime;
> +
> +	/* receiving data means alive */
> +	if (elapsed < keepalive_time_when(tp)) {
> +		elapsed = keepalive_time_when(tp) - elapsed;
> +		goto resched;
> +	}
> +
>  	elapsed = tcp_time_stamp - tp->rcv_tstamp;

...And then use it here too for setting the elapsed and doing the test 
and what follows only once?

>  	if (elapsed >= keepalive_time_when(tp)) {
> 

-- 
 i.

Re: [PATCH v3] TCP: avoid to send keepalive probes if receiving data

[David Miller]

From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Mon, 26 Apr 2010 12:47:13 +0300 (EEST)

>>  			    !((1 << sk->sk_state) &
>>  			      (TCPF_CLOSE | TCPF_LISTEN))) {
>> -				__u32 elapsed = tcp_time_stamp - tp->rcv_tstamp;
>> +				u32 elapsed = min_t(u32,
>> +				      tcp_time_stamp - icsk->icsk_ack.lrcvtime,
>> +				      tcp_time_stamp - tp->rcv_tstamp);
>> +
> 
> I'd put this into a helper in include/net/tcp.h
> 
 ...
>> +	if (elapsed < keepalive_time_when(tp)) {
>> +		elapsed = keepalive_time_when(tp) - elapsed;
>> +		goto resched;
>> +	}
>> +
>>  	elapsed = tcp_time_stamp - tp->rcv_tstamp;
> 
> ...And then use it here too for setting the elapsed and doing the test 
> and what follows only once?
> 

Agreed.

[PATCH v4] TCP: avoid to send keepalive probes if receiving data

[Flavio Leitner]

RFC 1122 says the following:
...
  Keep-alive packets MUST only be sent when no data or
  acknowledgement packets have been received for the
  connection within an interval.
...

The acknowledgement packet is reseting the keepalive
timer but the data packet isn't. This patch fixes it by
checking the timestamp of the last received data packet
too when the keepalive timer expires.

Signed-off-by: Flavio Leitner <fleitner@redhat.com>
---
 include/net/tcp.h    |    8 ++++++++
 net/ipv4/tcp.c       |    2 +-
 net/ipv4/tcp_timer.c |    4 ++--
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 75be5a2..305810e 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1032,6 +1032,14 @@ static inline int keepalive_probes(const struct tcp_sock *tp)
 	return tp->keepalive_probes ? : sysctl_tcp_keepalive_probes;
 }
 
+static inline u32 keepalive_time_elapsed(const struct tcp_sock *tp)
+{
+	const struct inet_connection_sock *icsk = &tp->inet_conn;
+
+	return min_t(u32, tcp_time_stamp - icsk->icsk_ack.lrcvtime,
+			  tcp_time_stamp - tp->rcv_tstamp);
+}
+
 static inline int tcp_fin_time(const struct sock *sk)
 {
 	int fin_timeout = tcp_sk(sk)->linger2 ? : sysctl_tcp_fin_timeout;
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 0f8caf6..48a6f33 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2298,7 +2298,7 @@ static int do_tcp_setsockopt(struct sock *sk, int level,
 			if (sock_flag(sk, SOCK_KEEPOPEN) &&
 			    !((1 << sk->sk_state) &
 			      (TCPF_CLOSE | TCPF_LISTEN))) {
-				__u32 elapsed = tcp_time_stamp - tp->rcv_tstamp;
+				u32 elapsed = keepalive_time_elapsed(tp);
 				if (tp->keepalive_time > elapsed)
 					elapsed = tp->keepalive_time - elapsed;
 				else
diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index 8a0ab29..a9155f9 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -517,7 +517,7 @@ static void tcp_keepalive_timer (unsigned long data)
 	struct sock *sk = (struct sock *) data;
 	struct inet_connection_sock *icsk = inet_csk(sk);
 	struct tcp_sock *tp = tcp_sk(sk);
-	__u32 elapsed;
+	u32 elapsed;
 
 	/* Only process if socket is not in use. */
 	bh_lock_sock(sk);
@@ -554,7 +554,7 @@ static void tcp_keepalive_timer (unsigned long data)
 	if (tp->packets_out || tcp_send_head(sk))
 		goto resched;
 
-	elapsed = tcp_time_stamp - tp->rcv_tstamp;
+	elapsed = keepalive_time_elapsed(tp);
 
 	if (elapsed >= keepalive_time_when(tp)) {
 		if (icsk->icsk_probes_out >= keepalive_probes(tp)) {
-- 
1.5.5.6

Re: [PATCH v4] TCP: avoid to send keepalive probes if receiving data

[Eric Dumazet]

Le mardi 27 avril 2010 à 01:33 -0300, Flavio Leitner a écrit :
> RFC 1122 says the following:
> ...
>   Keep-alive packets MUST only be sent when no data or
>   acknowledgement packets have been received for the
>   connection within an interval.
> ...
> 
> The acknowledgement packet is reseting the keepalive
> timer but the data packet isn't. This patch fixes it by
> checking the timestamp of the last received data packet
> too when the keepalive timer expires.
> 
> Signed-off-by: Flavio Leitner <fleitner@redhat.com>
> ---

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

>  include/net/tcp.h    |    8 ++++++++
>  net/ipv4/tcp.c       |    2 +-
>  net/ipv4/tcp_timer.c |    4 ++--
>  3 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 75be5a2..305810e 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1032,6 +1032,14 @@ static inline int keepalive_probes(const struct tcp_sock *tp)
>  	return tp->keepalive_probes ? : sysctl_tcp_keepalive_probes;
>  }
>  
> +static inline u32 keepalive_time_elapsed(const struct tcp_sock *tp)
> +{
> +	const struct inet_connection_sock *icsk = &tp->inet_conn;
> +
> +	return min_t(u32, tcp_time_stamp - icsk->icsk_ack.lrcvtime,
> +			  tcp_time_stamp - tp->rcv_tstamp);
> +}
> +
>  static inline int tcp_fin_time(const struct sock *sk)
>  {
>  	int fin_timeout = tcp_sk(sk)->linger2 ? : sysctl_tcp_fin_timeout;
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 0f8caf6..48a6f33 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -2298,7 +2298,7 @@ static int do_tcp_setsockopt(struct sock *sk, int level,
>  			if (sock_flag(sk, SOCK_KEEPOPEN) &&
>  			    !((1 << sk->sk_state) &
>  			      (TCPF_CLOSE | TCPF_LISTEN))) {
> -				__u32 elapsed = tcp_time_stamp - tp->rcv_tstamp;
> +				u32 elapsed = keepalive_time_elapsed(tp);
>  				if (tp->keepalive_time > elapsed)
>  					elapsed = tp->keepalive_time - elapsed;
>  				else
> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
> index 8a0ab29..a9155f9 100644
> --- a/net/ipv4/tcp_timer.c
> +++ b/net/ipv4/tcp_timer.c
> @@ -517,7 +517,7 @@ static void tcp_keepalive_timer (unsigned long data)
>  	struct sock *sk = (struct sock *) data;
>  	struct inet_connection_sock *icsk = inet_csk(sk);
>  	struct tcp_sock *tp = tcp_sk(sk);
> -	__u32 elapsed;
> +	u32 elapsed;
>  
>  	/* Only process if socket is not in use. */
>  	bh_lock_sock(sk);
> @@ -554,7 +554,7 @@ static void tcp_keepalive_timer (unsigned long data)
>  	if (tp->packets_out || tcp_send_head(sk))
>  		goto resched;
>  
> -	elapsed = tcp_time_stamp - tp->rcv_tstamp;
> +	elapsed = keepalive_time_elapsed(tp);
>  
>  	if (elapsed >= keepalive_time_when(tp)) {
>  		if (icsk->icsk_probes_out >= keepalive_probes(tp)) {

Re: [PATCH v4] TCP: avoid to send keepalive probes if receiving data

[Ilpo Järvinen]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2797 bytes --]

On Tue, 27 Apr 2010, Flavio Leitner wrote:

> RFC 1122 says the following:
> ...
>   Keep-alive packets MUST only be sent when no data or
>   acknowledgement packets have been received for the
>   connection within an interval.
> ...
> 
> The acknowledgement packet is reseting the keepalive
> timer but the data packet isn't. This patch fixes it by
> checking the timestamp of the last received data packet
> too when the keepalive timer expires.
> 
> Signed-off-by: Flavio Leitner <fleitner@redhat.com>
> ---
>  include/net/tcp.h    |    8 ++++++++
>  net/ipv4/tcp.c       |    2 +-
>  net/ipv4/tcp_timer.c |    4 ++--
>  3 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 75be5a2..305810e 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1032,6 +1032,14 @@ static inline int keepalive_probes(const struct tcp_sock *tp)
>  	return tp->keepalive_probes ? : sysctl_tcp_keepalive_probes;
>  }
>  
> +static inline u32 keepalive_time_elapsed(const struct tcp_sock *tp)
> +{
> +	const struct inet_connection_sock *icsk = &tp->inet_conn;
> +
> +	return min_t(u32, tcp_time_stamp - icsk->icsk_ack.lrcvtime,
> +			  tcp_time_stamp - tp->rcv_tstamp);
> +}
> +
>  static inline int tcp_fin_time(const struct sock *sk)
>  {
>  	int fin_timeout = tcp_sk(sk)->linger2 ? : sysctl_tcp_fin_timeout;
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 0f8caf6..48a6f33 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -2298,7 +2298,7 @@ static int do_tcp_setsockopt(struct sock *sk, int level,
>  			if (sock_flag(sk, SOCK_KEEPOPEN) &&
>  			    !((1 << sk->sk_state) &
>  			      (TCPF_CLOSE | TCPF_LISTEN))) {
> -				__u32 elapsed = tcp_time_stamp - tp->rcv_tstamp;
> +				u32 elapsed = keepalive_time_elapsed(tp);
>  				if (tp->keepalive_time > elapsed)
>  					elapsed = tp->keepalive_time - elapsed;
>  				else
> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
> index 8a0ab29..a9155f9 100644
> --- a/net/ipv4/tcp_timer.c
> +++ b/net/ipv4/tcp_timer.c
> @@ -517,7 +517,7 @@ static void tcp_keepalive_timer (unsigned long data)
>  	struct sock *sk = (struct sock *) data;
>  	struct inet_connection_sock *icsk = inet_csk(sk);
>  	struct tcp_sock *tp = tcp_sk(sk);
> -	__u32 elapsed;
> +	u32 elapsed;
>  
>  	/* Only process if socket is not in use. */
>  	bh_lock_sock(sk);
> @@ -554,7 +554,7 @@ static void tcp_keepalive_timer (unsigned long data)
>  	if (tp->packets_out || tcp_send_head(sk))
>  		goto resched;
>  
> -	elapsed = tcp_time_stamp - tp->rcv_tstamp;
> +	elapsed = keepalive_time_elapsed(tp);
>  
>  	if (elapsed >= keepalive_time_when(tp)) {
>  		if (icsk->icsk_probes_out >= keepalive_probes(tp)) {

Thanks. Looks very nice now.

Acked-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>

-- 
 i.

Re: [PATCH v4] TCP: avoid to send keepalive probes if receiving data

[David Miller]

From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Tue, 27 Apr 2010 09:08:08 +0300 (EEST)

> On Tue, 27 Apr 2010, Flavio Leitner wrote:
> 
>> RFC 1122 says the following:
>> ...
>>   Keep-alive packets MUST only be sent when no data or
>>   acknowledgement packets have been received for the
>>   connection within an interval.
>> ...
>> 
>> The acknowledgement packet is reseting the keepalive
>> timer but the data packet isn't. This patch fixes it by
>> checking the timestamp of the last received data packet
>> too when the keepalive timer expires.
>> 
>> Signed-off-by: Flavio Leitner <fleitner@redhat.com>
 ...
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
 ...
> Acked-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>

Applied, thanks everyone.

Re: [PATCH v2] TCP: avoid to send keepalive probes if it is receiving data

[Flavio Leitner]

On Sun, Apr 18, 2010 at 07:15:19PM +0200, Eric Dumazet wrote:
> Le dimanche 18 avril 2010 à 11:55 -0300, Flavio Leitner a écrit :
> > RFC 1122 says the following:
> > ...
> >   Keep-alive packets MUST only be sent when no data or
> >   acknowledgement packets have been received for the
> >   connection within an interval.
> > ...
> > 
> > Fix this by storing the timestamp of last received data
> > packet and checking for it when the keepalive timer expires.
> > 
> > -v2 fix do_tcp_setsockopt() as pointed by Eric Dumazet <eric.dumazet@gmail.com>
> > 
> > Signed-off-by: Flavio Leitner <fleitner@redhat.com>
> 
> 
> I find this patch very welcome, and we could easily use this new
> lrcvtime information available in diagnostic tools (ss command)
> 
> But are you sure you update it for all valid packets ?
> 
> If we receive a pure ACK, it seems you do not ...

Pure ack is handled by rcv_tstamp in the struct which is
considered in tcp_keepalive_time() too.

The idea of exporting those variables is nice, I'll see
how 'ss' works.

thanks for reviewing the patch!
 

 
> > ---
> >  include/linux/tcp.h  |    1 +
> >  net/ipv4/tcp.c       |    5 ++++-
> >  net/ipv4/tcp_input.c |    3 +++
> >  net/ipv4/tcp_timer.c |    8 ++++++++
> >  4 files changed, 16 insertions(+), 1 deletions(-)
> > 
> > diff --git a/include/linux/tcp.h b/include/linux/tcp.h
> > index a778ee0..405678f 100644
> > --- a/include/linux/tcp.h
> > +++ b/include/linux/tcp.h
> > @@ -314,6 +314,7 @@ struct tcp_sock {
> >   	u32	snd_sml;	/* Last byte of the most recently transmitted small packet */
> >  	u32	rcv_tstamp;	/* timestamp of last received ACK (for keepalives) */
> >  	u32	lsndtime;	/* timestamp of last sent data packet (for restart window) */
> > +	u32	lrcvtime;	/* timestamp of last received data packet (for keepalives) */
> >  
> >  	/* Data for direct copy to user */
> >  	struct {
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > index 0f8caf6..a4048d7 100644
> > --- a/net/ipv4/tcp.c
> > +++ b/net/ipv4/tcp.c
> > @@ -2298,7 +2298,10 @@ static int do_tcp_setsockopt(struct sock *sk, int level,
> >  			if (sock_flag(sk, SOCK_KEEPOPEN) &&
> >  			    !((1 << sk->sk_state) &
> >  			      (TCPF_CLOSE | TCPF_LISTEN))) {
> > -				__u32 elapsed = tcp_time_stamp - tp->rcv_tstamp;
> > +				u32 elapsed = min_t(u32,
> > +						      tcp_time_stamp - tp->rcv_tstamp,
> > +						      tcp_time_stamp - tp->lrcvtime);
> > +
> >  				if (tp->keepalive_time > elapsed)
> >  					elapsed = tp->keepalive_time - elapsed;
> >  				else
> > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > index f240f57..60d2980 100644
> > --- a/net/ipv4/tcp_input.c
> > +++ b/net/ipv4/tcp_input.c
> > @@ -5391,6 +5391,8 @@ no_ack:
> >  				__kfree_skb(skb);
> >  			else
> >  				sk->sk_data_ready(sk, 0);
> > +
> > +			tp->lrcvtime = tcp_time_stamp;
> >  			return 0;
> >  		}
> >  	}
> > @@ -5421,6 +5423,7 @@ step5:
> >  
> >  	tcp_data_snd_check(sk);
> >  	tcp_ack_snd_check(sk);
> > +	tp->lrcvtime = tcp_time_stamp;
> >  	return 0;
> >  
> >  csum_error:
> > diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
> > index 8a0ab29..74dd804 100644
> > --- a/net/ipv4/tcp_timer.c
> > +++ b/net/ipv4/tcp_timer.c
> > @@ -554,6 +554,14 @@ static void tcp_keepalive_timer (unsigned long data)
> >  	if (tp->packets_out || tcp_send_head(sk))
> >  		goto resched;
> >  
> > +	elapsed = tcp_time_stamp - tp->lrcvtime;
> > +	
> > +	/* receiving data means alive */
> > +	if (elapsed < keepalive_time_when(tp)) {
> > +		elapsed = keepalive_time_when(tp) - elapsed;
> > +		goto resched;
> > +	}
> > +
> >  	elapsed = tcp_time_stamp - tp->rcv_tstamp;
> >  
> >  	if (elapsed >= keepalive_time_when(tp)) {
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Flavio