linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 net-next] net/tcp: trace all TCP/IP state transition with tcp_set_state tracepoint
@ 2017-12-02  9:36 Yafang Shao
  2017-12-04 17:49 ` Song Liu
  2017-12-04 19:28 ` Marcelo Ricardo Leitner
  0 siblings, 2 replies; 7+ messages in thread
From: Yafang Shao @ 2017-12-02  9:36 UTC (permalink / raw)
  To: davem
  Cc: kuznet, yoshfuji, rostedt, songliubraving, bgregg, netdev,
	linux-kernel, Yafang Shao

The TCP/IP transition from TCP_LISTEN to TCP_SYN_RECV and some other
transitions are not traced with tcp_set_state tracepoint.

In order to trace the whole tcp lifespans, two helpers are introduced,
void __tcp_set_state(struct sock *sk, int state)
void __sk_state_store(struct sock *sk, int newstate)

When do TCP/IP state transition, we should use these two helpers or use
tcp_set_state() other than assigning a value to sk_state directly.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>

---
v2: test
---
 include/net/tcp.h               |  2 ++
 net/ipv4/inet_connection_sock.c |  6 +++---
 net/ipv4/inet_hashtables.c      |  2 +-
 net/ipv4/tcp.c                  | 12 ++++++++++++
 4 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 85ea578..4f2d015 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1247,6 +1247,8 @@ static inline bool tcp_checksum_complete(struct sk_buff *skb)
 	"Close Wait","Last ACK","Listen","Closing"
 };
 #endif
+void __sk_state_store(struct sock *sk, int newstate);
+void __tcp_set_state(struct sock *sk, int state);
 void tcp_set_state(struct sock *sk, int state);
 
 void tcp_done(struct sock *sk);
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index 4ca46dc..f3967f1 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -783,7 +783,7 @@ struct sock *inet_csk_clone_lock(const struct sock *sk,
 	if (newsk) {
 		struct inet_connection_sock *newicsk = inet_csk(newsk);
 
-		newsk->sk_state = TCP_SYN_RECV;
+		__tcp_set_state(newsk, TCP_SYN_RECV);
 		newicsk->icsk_bind_hash = NULL;
 
 		inet_sk(newsk)->inet_dport = inet_rsk(req)->ir_rmt_port;
@@ -877,7 +877,7 @@ int inet_csk_listen_start(struct sock *sk, int backlog)
 	 * It is OK, because this socket enters to hash table only
 	 * after validation is complete.
 	 */
-	sk_state_store(sk, TCP_LISTEN);
+	__sk_state_store(sk, TCP_LISTEN);
 	if (!sk->sk_prot->get_port(sk, inet->inet_num)) {
 		inet->inet_sport = htons(inet->inet_num);
 
@@ -888,7 +888,7 @@ int inet_csk_listen_start(struct sock *sk, int backlog)
 			return 0;
 	}
 
-	sk->sk_state = TCP_CLOSE;
+	__tcp_set_state(sk, TCP_CLOSE);
 	return err;
 }
 EXPORT_SYMBOL_GPL(inet_csk_listen_start);
diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index e7d15fb..72c15b6 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -430,7 +430,7 @@ bool inet_ehash_nolisten(struct sock *sk, struct sock *osk)
 		sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1);
 	} else {
 		percpu_counter_inc(sk->sk_prot->orphan_count);
-		sk->sk_state = TCP_CLOSE;
+		__tcp_set_state(sk, TCP_CLOSE);
 		sock_set_flag(sk, SOCK_DEAD);
 		inet_csk_destroy_sock(sk);
 	}
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index bf97317..2bc7e04 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2036,6 +2036,18 @@ int tcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int nonblock,
 }
 EXPORT_SYMBOL(tcp_recvmsg);
 
+void __sk_state_store(struct sock *sk, int newstate)
+{
+	trace_tcp_set_state(sk, sk->sk_state, newstate);
+	sk_state_store(sk, newstate);
+}
+
+void __tcp_set_state(struct sock *sk, int state)
+{
+	trace_tcp_set_state(sk, sk->sk_state, state);
+	sk->sk_state = state;
+}
+
 void tcp_set_state(struct sock *sk, int state)
 {
 	int oldstate = sk->sk_state;
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 net-next] net/tcp: trace all TCP/IP state transition with tcp_set_state tracepoint
  2017-12-02  9:36 [PATCH v2 net-next] net/tcp: trace all TCP/IP state transition with tcp_set_state tracepoint Yafang Shao
@ 2017-12-04 17:49 ` Song Liu
  2017-12-04 19:28 ` Marcelo Ricardo Leitner
  1 sibling, 0 replies; 7+ messages in thread
From: Song Liu @ 2017-12-04 17:49 UTC (permalink / raw)
  To: Yafang Shao
  Cc: David Miller, kuznet, yoshfuji, Steven Rostedt, Brendan Gregg,
	netdev, linux-kernel


> On Dec 2, 2017, at 1:36 AM, Yafang Shao <laoar.shao@gmail.com> wrote:
> 
> The TCP/IP transition from TCP_LISTEN to TCP_SYN_RECV and some other
> transitions are not traced with tcp_set_state tracepoint.
> 
> In order to trace the whole tcp lifespans, two helpers are introduced,
> void __tcp_set_state(struct sock *sk, int state)
> void __sk_state_store(struct sock *sk, int newstate)
> 
> When do TCP/IP state transition, we should use these two helpers or use
> tcp_set_state() other than assigning a value to sk_state directly.
> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> 
> ---
> v2: test
> ---
> include/net/tcp.h               |  2 ++
> net/ipv4/inet_connection_sock.c |  6 +++---
> net/ipv4/inet_hashtables.c      |  2 +-
> net/ipv4/tcp.c                  | 12 ++++++++++++
> 4 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 85ea578..4f2d015 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1247,6 +1247,8 @@ static inline bool tcp_checksum_complete(struct sk_buff *skb)
> 	"Close Wait","Last ACK","Listen","Closing"
> };
> #endif
> +void __sk_state_store(struct sock *sk, int newstate);
> +void __tcp_set_state(struct sock *sk, int state);
> void tcp_set_state(struct sock *sk, int state);
> 
> void tcp_done(struct sock *sk);
> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index 4ca46dc..f3967f1 100644
> --- a/net/ipv4/inet_connection_sock.c
> +++ b/net/ipv4/inet_connection_sock.c
> @@ -783,7 +783,7 @@ struct sock *inet_csk_clone_lock(const struct sock *sk,
> 	if (newsk) {
> 		struct inet_connection_sock *newicsk = inet_csk(newsk);
> 
> -		newsk->sk_state = TCP_SYN_RECV;
> +		__tcp_set_state(newsk, TCP_SYN_RECV);
> 		newicsk->icsk_bind_hash = NULL;
> 
> 		inet_sk(newsk)->inet_dport = inet_rsk(req)->ir_rmt_port;
> @@ -877,7 +877,7 @@ int inet_csk_listen_start(struct sock *sk, int backlog)
> 	 * It is OK, because this socket enters to hash table only
> 	 * after validation is complete.
> 	 */
> -	sk_state_store(sk, TCP_LISTEN);
> +	__sk_state_store(sk, TCP_LISTEN);
> 	if (!sk->sk_prot->get_port(sk, inet->inet_num)) {
> 		inet->inet_sport = htons(inet->inet_num);
> 
> @@ -888,7 +888,7 @@ int inet_csk_listen_start(struct sock *sk, int backlog)
> 			return 0;
> 	}
> 
> -	sk->sk_state = TCP_CLOSE;
> +	__tcp_set_state(sk, TCP_CLOSE);
> 	return err;
> }
> EXPORT_SYMBOL_GPL(inet_csk_listen_start);
> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
> index e7d15fb..72c15b6 100644
> --- a/net/ipv4/inet_hashtables.c
> +++ b/net/ipv4/inet_hashtables.c
> @@ -430,7 +430,7 @@ bool inet_ehash_nolisten(struct sock *sk, struct sock *osk)
> 		sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1);
> 	} else {
> 		percpu_counter_inc(sk->sk_prot->orphan_count);
> -		sk->sk_state = TCP_CLOSE;
> +		__tcp_set_state(sk, TCP_CLOSE);
> 		sock_set_flag(sk, SOCK_DEAD);
> 		inet_csk_destroy_sock(sk);
> 	}
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index bf97317..2bc7e04 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -2036,6 +2036,18 @@ int tcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int nonblock,
> }
> EXPORT_SYMBOL(tcp_recvmsg);
> 
> +void __sk_state_store(struct sock *sk, int newstate)
> +{
> +	trace_tcp_set_state(sk, sk->sk_state, newstate);
> +	sk_state_store(sk, newstate);
> +}
> +
> +void __tcp_set_state(struct sock *sk, int state)
> +{
> +	trace_tcp_set_state(sk, sk->sk_state, state);
> +	sk->sk_state = state;
> +}
> +
> void tcp_set_state(struct sock *sk, int state)
> {
> 	int oldstate = sk->sk_state;
> -- 
> 1.8.3.1
> 

Asked-by: Song Liu <songliubraving@fb.com>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 net-next] net/tcp: trace all TCP/IP state transition with tcp_set_state tracepoint
  2017-12-02  9:36 [PATCH v2 net-next] net/tcp: trace all TCP/IP state transition with tcp_set_state tracepoint Yafang Shao
  2017-12-04 17:49 ` Song Liu
@ 2017-12-04 19:28 ` Marcelo Ricardo Leitner
  2017-12-05  7:17   ` Yafang Shao
  1 sibling, 1 reply; 7+ messages in thread
From: Marcelo Ricardo Leitner @ 2017-12-04 19:28 UTC (permalink / raw)
  To: Yafang Shao
  Cc: davem, kuznet, yoshfuji, rostedt, songliubraving, bgregg, netdev,
	linux-kernel

On Sat, Dec 02, 2017 at 09:36:41AM +0000, Yafang Shao wrote:
> The TCP/IP transition from TCP_LISTEN to TCP_SYN_RECV and some other
> transitions are not traced with tcp_set_state tracepoint.
> 
> In order to trace the whole tcp lifespans, two helpers are introduced,
> void __tcp_set_state(struct sock *sk, int state)
> void __sk_state_store(struct sock *sk, int newstate)
> 
> When do TCP/IP state transition, we should use these two helpers or use
> tcp_set_state() other than assigning a value to sk_state directly.
> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> 
> ---
> v2: test
> ---
>  include/net/tcp.h               |  2 ++
>  net/ipv4/inet_connection_sock.c |  6 +++---
>  net/ipv4/inet_hashtables.c      |  2 +-
>  net/ipv4/tcp.c                  | 12 ++++++++++++
>  4 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 85ea578..4f2d015 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1247,6 +1247,8 @@ static inline bool tcp_checksum_complete(struct sk_buff *skb)
>  	"Close Wait","Last ACK","Listen","Closing"
>  };
>  #endif
> +void __sk_state_store(struct sock *sk, int newstate);
> +void __tcp_set_state(struct sock *sk, int state);
>  void tcp_set_state(struct sock *sk, int state);
>  
>  void tcp_done(struct sock *sk);
> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index 4ca46dc..f3967f1 100644
> --- a/net/ipv4/inet_connection_sock.c
> +++ b/net/ipv4/inet_connection_sock.c
> @@ -783,7 +783,7 @@ struct sock *inet_csk_clone_lock(const struct sock *sk,
>  	if (newsk) {
>  		struct inet_connection_sock *newicsk = inet_csk(newsk);
>  
> -		newsk->sk_state = TCP_SYN_RECV;
> +		__tcp_set_state(newsk, TCP_SYN_RECV);
>  		newicsk->icsk_bind_hash = NULL;
>  
>  		inet_sk(newsk)->inet_dport = inet_rsk(req)->ir_rmt_port;
> @@ -877,7 +877,7 @@ int inet_csk_listen_start(struct sock *sk, int backlog)
>  	 * It is OK, because this socket enters to hash table only
>  	 * after validation is complete.
>  	 */
> -	sk_state_store(sk, TCP_LISTEN);
> +	__sk_state_store(sk, TCP_LISTEN);
>  	if (!sk->sk_prot->get_port(sk, inet->inet_num)) {
>  		inet->inet_sport = htons(inet->inet_num);
>  
> @@ -888,7 +888,7 @@ int inet_csk_listen_start(struct sock *sk, int backlog)
>  			return 0;
>  	}
>  
> -	sk->sk_state = TCP_CLOSE;
> +	__tcp_set_state(sk, TCP_CLOSE);
>  	return err;
>  }
>  EXPORT_SYMBOL_GPL(inet_csk_listen_start);
> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
> index e7d15fb..72c15b6 100644
> --- a/net/ipv4/inet_hashtables.c
> +++ b/net/ipv4/inet_hashtables.c
> @@ -430,7 +430,7 @@ bool inet_ehash_nolisten(struct sock *sk, struct sock *osk)
>  		sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1);
>  	} else {
>  		percpu_counter_inc(sk->sk_prot->orphan_count);
> -		sk->sk_state = TCP_CLOSE;
> +		__tcp_set_state(sk, TCP_CLOSE);
>  		sock_set_flag(sk, SOCK_DEAD);
>  		inet_csk_destroy_sock(sk);
>  	}
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index bf97317..2bc7e04 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -2036,6 +2036,18 @@ int tcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int nonblock,
>  }
>  EXPORT_SYMBOL(tcp_recvmsg);
>  
> +void __sk_state_store(struct sock *sk, int newstate)
> +{
> +	trace_tcp_set_state(sk, sk->sk_state, newstate);
> +	sk_state_store(sk, newstate);

This sounds counter-intuitive, to have a __func() to call func(). It's
usually the other way around.
There is only 1 call to sk_state_store in the stack, what about
inverting these __ ?

I guess you applied the same standard as to the one below, but it's a
different case.

> +}
> +
> +void __tcp_set_state(struct sock *sk, int state)
> +{
> +	trace_tcp_set_state(sk, sk->sk_state, state);
> +	sk->sk_state = state;
> +}
> +
>  void tcp_set_state(struct sock *sk, int state)
>  {
>  	int oldstate = sk->sk_state;
> -- 
> 1.8.3.1
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 net-next] net/tcp: trace all TCP/IP state transition with tcp_set_state tracepoint
  2017-12-04 19:28 ` Marcelo Ricardo Leitner
@ 2017-12-05  7:17   ` Yafang Shao
  0 siblings, 0 replies; 7+ messages in thread
From: Yafang Shao @ 2017-12-05  7:17 UTC (permalink / raw)
  To: Marcelo Ricardo Leitner
  Cc: David Miller, Alexey Kuznetsov, yoshfuji, Steven Rostedt,
	Song Liu, Brendan Gregg, netdev, LKML

2017-12-05 3:28 GMT+08:00 Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>:
> On Sat, Dec 02, 2017 at 09:36:41AM +0000, Yafang Shao wrote:
>> The TCP/IP transition from TCP_LISTEN to TCP_SYN_RECV and some other
>> transitions are not traced with tcp_set_state tracepoint.
>>
>> In order to trace the whole tcp lifespans, two helpers are introduced,
>> void __tcp_set_state(struct sock *sk, int state)
>> void __sk_state_store(struct sock *sk, int newstate)
>>
>> When do TCP/IP state transition, we should use these two helpers or use
>> tcp_set_state() other than assigning a value to sk_state directly.
>>
>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>>
>> ---
>> v2: test
>> ---
>>  include/net/tcp.h               |  2 ++
>>  net/ipv4/inet_connection_sock.c |  6 +++---
>>  net/ipv4/inet_hashtables.c      |  2 +-
>>  net/ipv4/tcp.c                  | 12 ++++++++++++
>>  4 files changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/net/tcp.h b/include/net/tcp.h
>> index 85ea578..4f2d015 100644
>> --- a/include/net/tcp.h
>> +++ b/include/net/tcp.h
>> @@ -1247,6 +1247,8 @@ static inline bool tcp_checksum_complete(struct sk_buff *skb)
>>       "Close Wait","Last ACK","Listen","Closing"
>>  };
>>  #endif
>> +void __sk_state_store(struct sock *sk, int newstate);
>> +void __tcp_set_state(struct sock *sk, int state);
>>  void tcp_set_state(struct sock *sk, int state);
>>
>>  void tcp_done(struct sock *sk);
>> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
>> index 4ca46dc..f3967f1 100644
>> --- a/net/ipv4/inet_connection_sock.c
>> +++ b/net/ipv4/inet_connection_sock.c
>> @@ -783,7 +783,7 @@ struct sock *inet_csk_clone_lock(const struct sock *sk,
>>       if (newsk) {
>>               struct inet_connection_sock *newicsk = inet_csk(newsk);
>>
>> -             newsk->sk_state = TCP_SYN_RECV;
>> +             __tcp_set_state(newsk, TCP_SYN_RECV);
>>               newicsk->icsk_bind_hash = NULL;
>>
>>               inet_sk(newsk)->inet_dport = inet_rsk(req)->ir_rmt_port;
>> @@ -877,7 +877,7 @@ int inet_csk_listen_start(struct sock *sk, int backlog)
>>        * It is OK, because this socket enters to hash table only
>>        * after validation is complete.
>>        */
>> -     sk_state_store(sk, TCP_LISTEN);
>> +     __sk_state_store(sk, TCP_LISTEN);
>>       if (!sk->sk_prot->get_port(sk, inet->inet_num)) {
>>               inet->inet_sport = htons(inet->inet_num);
>>
>> @@ -888,7 +888,7 @@ int inet_csk_listen_start(struct sock *sk, int backlog)
>>                       return 0;
>>       }
>>
>> -     sk->sk_state = TCP_CLOSE;
>> +     __tcp_set_state(sk, TCP_CLOSE);
>>       return err;
>>  }
>>  EXPORT_SYMBOL_GPL(inet_csk_listen_start);
>> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
>> index e7d15fb..72c15b6 100644
>> --- a/net/ipv4/inet_hashtables.c
>> +++ b/net/ipv4/inet_hashtables.c
>> @@ -430,7 +430,7 @@ bool inet_ehash_nolisten(struct sock *sk, struct sock *osk)
>>               sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1);
>>       } else {
>>               percpu_counter_inc(sk->sk_prot->orphan_count);
>> -             sk->sk_state = TCP_CLOSE;
>> +             __tcp_set_state(sk, TCP_CLOSE);
>>               sock_set_flag(sk, SOCK_DEAD);
>>               inet_csk_destroy_sock(sk);
>>       }
>> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
>> index bf97317..2bc7e04 100644
>> --- a/net/ipv4/tcp.c
>> +++ b/net/ipv4/tcp.c
>> @@ -2036,6 +2036,18 @@ int tcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int nonblock,
>>  }
>>  EXPORT_SYMBOL(tcp_recvmsg);
>>
>> +void __sk_state_store(struct sock *sk, int newstate)
>> +{
>> +     trace_tcp_set_state(sk, sk->sk_state, newstate);
>> +     sk_state_store(sk, newstate);
>
> This sounds counter-intuitive, to have a __func() to call func(). It's
> usually the other way around.

Agree to that.

> There is only 1 call to sk_state_store in the stack, what about
> inverting these __ ?
>

Sounds like a good idea.

> I guess you applied the same standard as to the one below, but it's a
> different case.
>
>> +}
>> +
>> +void __tcp_set_state(struct sock *sk, int state)
>> +{
>> +     trace_tcp_set_state(sk, sk->sk_state, state);
>> +     sk->sk_state = state;
>> +}
>> +
>>  void tcp_set_state(struct sock *sk, int state)
>>  {
>>       int oldstate = sk->sk_state;
>> --
>> 1.8.3.1
>>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 net-next] net/tcp: trace all TCP/IP state transition with tcp_set_state tracepoint
  2017-11-18 15:32 Yafang Shao
  2017-11-18 18:49 ` Song Liu
@ 2017-11-19  3:40 ` David Miller
  1 sibling, 0 replies; 7+ messages in thread
From: David Miller @ 2017-11-19  3:40 UTC (permalink / raw)
  To: laoar.shao
  Cc: kuznet, yoshfuji, rostedt, songliubraving, netdev, linux-kernel

From: Yafang Shao <laoar.shao@gmail.com>
Date: Sat, 18 Nov 2017 15:32:36 +0000

> The TCP/IP transition from TCP_LISTEN to TCP_SYN_RECV and some other
> transitions are not traced with tcp_set_state tracepoint.
> 
> In order to trace the whole tcp lifespans, two helpers are introduced,
> void __tcp_set_state(struct sock *sk, int state)
> void __sk_state_store(struct sock *sk, int newstate)
> 
> When do TCP/IP state transition, we should use these two helpers or use
> tcp_set_state() other than assigning a value to sk_state directly.
> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>

Please resubmit this when the net-next tree opens back up as it is
closed right now.

Thank you.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 net-next] net/tcp: trace all TCP/IP state transition with tcp_set_state tracepoint
  2017-11-18 15:32 Yafang Shao
@ 2017-11-18 18:49 ` Song Liu
  2017-11-19  3:40 ` David Miller
  1 sibling, 0 replies; 7+ messages in thread
From: Song Liu @ 2017-11-18 18:49 UTC (permalink / raw)
  To: Yafang Shao, Brendan Gregg
  Cc: davem, kuznet, yoshfuji, rostedt, netdev, linux-kernel


> On Nov 18, 2017, at 7:32 AM, Yafang Shao <laoar.shao@gmail.com> wrote:
> 
> The TCP/IP transition from TCP_LISTEN to TCP_SYN_RECV and some other
> transitions are not traced with tcp_set_state tracepoint.
> 
> In order to trace the whole tcp lifespans, two helpers are introduced,
> void __tcp_set_state(struct sock *sk, int state)
> void __sk_state_store(struct sock *sk, int newstate)
> 
> When do TCP/IP state transition, we should use these two helpers or use
> tcp_set_state() other than assigning a value to sk_state directly.
> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
> include/net/tcp.h               |  2 ++
> net/ipv4/inet_connection_sock.c |  6 +++---
> net/ipv4/inet_hashtables.c      |  2 +-
> net/ipv4/tcp.c                  | 12 ++++++++++++
> 4 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 85ea578..4f2d015 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1247,6 +1247,8 @@ static inline bool tcp_checksum_complete(struct sk_buff *skb)
> 	"Close Wait","Last ACK","Listen","Closing"
> };
> #endif
> +void __sk_state_store(struct sock *sk, int newstate);
> +void __tcp_set_state(struct sock *sk, int state);
> void tcp_set_state(struct sock *sk, int state);
> 
> void tcp_done(struct sock *sk);
> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index 4ca46dc..f3967f1 100644
> --- a/net/ipv4/inet_connection_sock.c
> +++ b/net/ipv4/inet_connection_sock.c
> @@ -783,7 +783,7 @@ struct sock *inet_csk_clone_lock(const struct sock *sk,
> 	if (newsk) {
> 		struct inet_connection_sock *newicsk = inet_csk(newsk);
> 
> -		newsk->sk_state = TCP_SYN_RECV;
> +		__tcp_set_state(newsk, TCP_SYN_RECV);
> 		newicsk->icsk_bind_hash = NULL;
> 
> 		inet_sk(newsk)->inet_dport = inet_rsk(req)->ir_rmt_port;
> @@ -877,7 +877,7 @@ int inet_csk_listen_start(struct sock *sk, int backlog)
> 	 * It is OK, because this socket enters to hash table only
> 	 * after validation is complete.
> 	 */
> -	sk_state_store(sk, TCP_LISTEN);
> +	__sk_state_store(sk, TCP_LISTEN);
> 	if (!sk->sk_prot->get_port(sk, inet->inet_num)) {
> 		inet->inet_sport = htons(inet->inet_num);
> 
> @@ -888,7 +888,7 @@ int inet_csk_listen_start(struct sock *sk, int backlog)
> 			return 0;
> 	}
> 
> -	sk->sk_state = TCP_CLOSE;
> +	__tcp_set_state(sk, TCP_CLOSE);
> 	return err;
> }
> EXPORT_SYMBOL_GPL(inet_csk_listen_start);
> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
> index e7d15fb..72c15b6 100644
> --- a/net/ipv4/inet_hashtables.c
> +++ b/net/ipv4/inet_hashtables.c
> @@ -430,7 +430,7 @@ bool inet_ehash_nolisten(struct sock *sk, struct sock *osk)
> 		sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1);
> 	} else {
> 		percpu_counter_inc(sk->sk_prot->orphan_count);
> -		sk->sk_state = TCP_CLOSE;
> +		__tcp_set_state(sk, TCP_CLOSE);
> 		sock_set_flag(sk, SOCK_DEAD);
> 		inet_csk_destroy_sock(sk);
> 	}
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index bf97317..2bc7e04 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -2036,6 +2036,18 @@ int tcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int nonblock,
> }
> EXPORT_SYMBOL(tcp_recvmsg);
> 
> +void __sk_state_store(struct sock *sk, int newstate)
> +{
> +	trace_tcp_set_state(sk, sk->sk_state, newstate);
> +	sk_state_store(sk, newstate);
> +}
> +
> +void __tcp_set_state(struct sock *sk, int state)
> +{
> +	trace_tcp_set_state(sk, sk->sk_state, state);
> +	sk->sk_state = state;
> +}
> +
> void tcp_set_state(struct sock *sk, int state)
> {
> 	int oldstate = sk->sk_state;
> -- 
> 1.8.3.1
> 

+ Brendan

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2 net-next] net/tcp: trace all TCP/IP state transition with tcp_set_state tracepoint
@ 2017-11-18 15:32 Yafang Shao
  2017-11-18 18:49 ` Song Liu
  2017-11-19  3:40 ` David Miller
  0 siblings, 2 replies; 7+ messages in thread
From: Yafang Shao @ 2017-11-18 15:32 UTC (permalink / raw)
  To: davem
  Cc: kuznet, yoshfuji, rostedt, songliubraving, netdev, linux-kernel,
	Yafang Shao

The TCP/IP transition from TCP_LISTEN to TCP_SYN_RECV and some other
transitions are not traced with tcp_set_state tracepoint.

In order to trace the whole tcp lifespans, two helpers are introduced,
void __tcp_set_state(struct sock *sk, int state)
void __sk_state_store(struct sock *sk, int newstate)

When do TCP/IP state transition, we should use these two helpers or use
tcp_set_state() other than assigning a value to sk_state directly.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 include/net/tcp.h               |  2 ++
 net/ipv4/inet_connection_sock.c |  6 +++---
 net/ipv4/inet_hashtables.c      |  2 +-
 net/ipv4/tcp.c                  | 12 ++++++++++++
 4 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 85ea578..4f2d015 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1247,6 +1247,8 @@ static inline bool tcp_checksum_complete(struct sk_buff *skb)
 	"Close Wait","Last ACK","Listen","Closing"
 };
 #endif
+void __sk_state_store(struct sock *sk, int newstate);
+void __tcp_set_state(struct sock *sk, int state);
 void tcp_set_state(struct sock *sk, int state);
 
 void tcp_done(struct sock *sk);
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index 4ca46dc..f3967f1 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -783,7 +783,7 @@ struct sock *inet_csk_clone_lock(const struct sock *sk,
 	if (newsk) {
 		struct inet_connection_sock *newicsk = inet_csk(newsk);
 
-		newsk->sk_state = TCP_SYN_RECV;
+		__tcp_set_state(newsk, TCP_SYN_RECV);
 		newicsk->icsk_bind_hash = NULL;
 
 		inet_sk(newsk)->inet_dport = inet_rsk(req)->ir_rmt_port;
@@ -877,7 +877,7 @@ int inet_csk_listen_start(struct sock *sk, int backlog)
 	 * It is OK, because this socket enters to hash table only
 	 * after validation is complete.
 	 */
-	sk_state_store(sk, TCP_LISTEN);
+	__sk_state_store(sk, TCP_LISTEN);
 	if (!sk->sk_prot->get_port(sk, inet->inet_num)) {
 		inet->inet_sport = htons(inet->inet_num);
 
@@ -888,7 +888,7 @@ int inet_csk_listen_start(struct sock *sk, int backlog)
 			return 0;
 	}
 
-	sk->sk_state = TCP_CLOSE;
+	__tcp_set_state(sk, TCP_CLOSE);
 	return err;
 }
 EXPORT_SYMBOL_GPL(inet_csk_listen_start);
diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index e7d15fb..72c15b6 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -430,7 +430,7 @@ bool inet_ehash_nolisten(struct sock *sk, struct sock *osk)
 		sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1);
 	} else {
 		percpu_counter_inc(sk->sk_prot->orphan_count);
-		sk->sk_state = TCP_CLOSE;
+		__tcp_set_state(sk, TCP_CLOSE);
 		sock_set_flag(sk, SOCK_DEAD);
 		inet_csk_destroy_sock(sk);
 	}
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index bf97317..2bc7e04 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2036,6 +2036,18 @@ int tcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int nonblock,
 }
 EXPORT_SYMBOL(tcp_recvmsg);
 
+void __sk_state_store(struct sock *sk, int newstate)
+{
+	trace_tcp_set_state(sk, sk->sk_state, newstate);
+	sk_state_store(sk, newstate);
+}
+
+void __tcp_set_state(struct sock *sk, int state)
+{
+	trace_tcp_set_state(sk, sk->sk_state, state);
+	sk->sk_state = state;
+}
+
 void tcp_set_state(struct sock *sk, int state)
 {
 	int oldstate = sk->sk_state;
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2017-12-05  7:18 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-02  9:36 [PATCH v2 net-next] net/tcp: trace all TCP/IP state transition with tcp_set_state tracepoint Yafang Shao
2017-12-04 17:49 ` Song Liu
2017-12-04 19:28 ` Marcelo Ricardo Leitner
2017-12-05  7:17   ` Yafang Shao
  -- strict thread matches above, loose matches on Subject: below --
2017-11-18 15:32 Yafang Shao
2017-11-18 18:49 ` Song Liu
2017-11-19  3:40 ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).