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 <[email protected]>
Acked-by: Song Liu <[email protected]>
---
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
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 <[email protected]>
> Acked-by: Song Liu <[email protected]>
Reviewed-by: Marcelo Ricardo Leitner <[email protected]>
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
>
From: Yafang Shao <[email protected]>
Date: Tue, 5 Dec 2017 14:12:42 +0000
> }
>
> +/* 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 *);
...
> --- 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);
> +}
> +
Please do not define a sock generic function in the TCP protocol code.
If it belongs in TCP and is only used by TCP, give is a tcp specific
name prefix rather than a generic sk_ one.
This is even more ugly because I see that inet_connection_sock.c and
inet_hashtables.c uses this thing too. Which means that DCCP sockets
will trigger this tracepoint.
inet_connection_sock.c and inet_hashtables.c are not guaranteed to
operate on only TCP sockets, and you must make amends to handle that
properly.
Thanks.