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

Subsystems: networking [general], networking [tcp], the rest

STALE3244d

4 messages, 3 authors, 2017-12-05 · open the first message on its own page

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

From: Yafang Shao <hidden>
Date: 2017-12-02 09:37:10

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 <redacted>

---
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

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

From: Song Liu <hidden>
Date: 2017-12-04 17:50:31

quoted hunk
On Dec 2, 2017, at 1:36 AM, Yafang Shao [off-list ref] 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 <redacted>

---
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 [off-list ref]

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

From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: 2017-12-04 19:28:14

On Sat, Dec 02, 2017 at 09:36:41AM +0000, Yafang Shao wrote:
quoted hunk
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 <redacted>

---
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

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

From: Yafang Shao <hidden>
Date: 2017-12-05 07:18:10

2017-12-05 3:28 GMT+08:00 Marcelo Ricardo Leitner [off-list ref]:
On Sat, Dec 02, 2017 at 09:36:41AM +0000, Yafang Shao wrote:
quoted
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 <redacted>

---
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.
quoted
+}
+
+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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help