Re: [PATCH v3 net-next] net/tcp: trace all TCP/IP state transition with tcp_set_state tracepoint

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

On Tue, Dec 05, 2017 at 02:12:42PM +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>
> Acked-by: Song Liu <songliubraving@fb.com>

Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Thanks

> ---
> v2->v3: Per suggestion from Marcelo Ricardo Leitner, inverting  __
> 		to sk_state_store. 
> ---
>  include/net/sock.h              |  7 +++++--
>  include/net/tcp.h               |  1 +
>  net/ipv4/inet_connection_sock.c |  4 ++--
>  net/ipv4/inet_hashtables.c      |  2 +-
>  net/ipv4/tcp.c                  | 14 +++++++++++++-
>  5 files changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 79e1a2c..e89a098 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -2349,18 +2349,21 @@ static inline int sk_state_load(const struct sock *sk)
>  }
>  
>  /**
> - * sk_state_store - update sk->sk_state
> + * __sk_state_store - update sk->sk_state
>   * @sk: socket pointer
>   * @newstate: new state
>   *
>   * Paired with sk_state_load(). Should be used in contexts where
>   * state change might impact lockless readers.
>   */
> -static inline void sk_state_store(struct sock *sk, int newstate)
> +static inline void __sk_state_store(struct sock *sk, int newstate)
>  {
>  	smp_store_release(&sk->sk_state, newstate);
>  }
>  
> +/* For tcp_set_state tracepoint */
> +void sk_state_store(struct sock *sk, int newstate);
> +
>  void sock_enable_timestamp(struct sock *sk, int flag);
>  int sock_get_timestamp(struct sock *, struct timeval __user *);
>  int sock_get_timestampns(struct sock *, struct timespec __user *);
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 4e09398..cc43ce9 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1247,6 +1247,7 @@ static inline bool tcp_checksum_complete(struct sk_buff *skb)
>  	"Close Wait","Last ACK","Listen","Closing"
>  };
>  #endif
> +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..74523c4 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;
> @@ -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 f6f5810..b24cc12 100644
> --- a/net/ipv4/inet_hashtables.c
> +++ b/net/ipv4/inet_hashtables.c
> @@ -544,7 +544,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 1803116..4ade887 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;
> @@ -2065,7 +2077,7 @@ void tcp_set_state(struct sock *sk, int state)
>  	/* Change state AFTER socket is unhashed to avoid closed
>  	 * socket sitting in hash tables.
>  	 */
> -	sk_state_store(sk, state);
> +	__sk_state_store(sk, state);
>  
>  #ifdef STATE_TRACE
>  	SOCK_DEBUG(sk, "TCP sk=%p, State %s -> %s\n", sk, statename[oldstate], statename[state]);
> -- 
> 1.8.3.1
>