| Message ID | 1512483162-5557-1-git-send-email-laoar.shao@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v3,net-next] net/tcp: trace all TCP/IP state transition with tcp_set_state tracepoint | expand |
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 >
From: Yafang Shao <laoar.shao@gmail.com> 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.
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]);