From: Song Liu <songliubraving@fb.com>
To: Yafang Shao <laoar.shao@gmail.com>
CC: David Miller <davem@davemloft.net>,
        "marcelo.leitner@gmail.com" <marcelo.leitner@gmail.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        "Brendan Gregg" <bgregg@netflix.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 net-next 2/4] net: tracepoint: replace tcp_set_state
 tracepoint with sock_set_state tracepoint
Thread-Topic: [PATCH v2 net-next 2/4] net: tracepoint: replace tcp_set_state
 tracepoint with sock_set_state tracepoint
Thread-Index: AQHTdc43bVNw9ah3B0yBoBbefNH56aNFAZ6A
Date: Fri, 15 Dec 2017 22:47:01 +0000
Message-ID: <4AB9713E-7E2D-4232-868B-ACE8364E33A0@fb.com>
References: <1513360611-11392-1-git-send-email-laoar.shao@gmail.com>
 <1513360611-11392-3-git-send-email-laoar.shao@gmail.com>
In-Reply-To: <1513360611-11392-3-git-send-email-laoar.shao@gmail.com>
Accept-Language: en-US
Content-Language: en-US
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
Content-ID: <736382728EC5254E81008F2522621ECC@namprd15.prod.outlook.com>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 15d26478-42a8-44c3-78a7-08d5440dc170
X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Dec 2017 22:47:01.0548
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR15MB1511
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-12-15_13:,,
 signatures=0
X-Proofpoint-Spam-Reason: safe
X-FB-Internal: Safe
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from quoted-printable to 8bit by nfs id vBFMlUeL003361
Content-Length: 11240
Lines: 382


> On Dec 15, 2017, at 9:56 AM, Yafang Shao <laoar.shao@gmail.com> wrote:
> 
> As sk_state is a common field for struct sock, so the state
> transition should not be a TCP specific feature.
> So I rename tcp_set_state tracepoint to sock_set_state tracepoint with
> some minor changes and move it into file trace/events/sock.h.
> 
> Two helpers are introduced to trace sk_state transition
>    - void sk_state_store(struct sock *sk, int state);
>    - void sk_set_state(struct sock *sk, int state);
> As trace header should not be included in other header files,
> so they are defined in sock.c.
> 
> The protocol such as SCTP maybe compiled as a ko, hence export
> sk_set_state().
> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
> include/net/sock.h              |  15 +-----
> include/trace/events/sock.h     | 106 ++++++++++++++++++++++++++++++++++++++++
> include/trace/events/tcp.h      |  91 ----------------------------------
> net/core/sock.c                 |  13 +++++
> net/ipv4/inet_connection_sock.c |   4 +-
> net/ipv4/inet_hashtables.c      |   2 +-
> net/ipv4/tcp.c                  |   4 --
> 7 files changed, 124 insertions(+), 111 deletions(-)
> 
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 9a90472..988ce82 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -2344,19 +2344,8 @@ static inline int sk_state_load(const struct sock *sk)
> 	return smp_load_acquire(&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)
> -{
> -	smp_store_release(&sk->sk_state, newstate);
> -}
> -
> +void sk_state_store(struct sock *sk, int newstate);
> +void sk_set_state(struct sock *sk, int state);
> 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/trace/events/sock.h b/include/trace/events/sock.h
> index ec4dade..61977e5 100644
> --- a/include/trace/events/sock.h
> +++ b/include/trace/events/sock.h
> @@ -6,7 +6,49 @@
> #define _TRACE_SOCK_H
> 
> #include <net/sock.h>
> +#include <net/ipv6.h>
> #include <linux/tracepoint.h>
> +#include <linux/ipv6.h>
> +#include <linux/tcp.h>
> +
> +#define inet_protocol_names		\
> +		EM(IPPROTO_TCP)			\
> +		EM(IPPROTO_DCCP)		\
> +		EMe(IPPROTO_SCTP)
> +
> +#define tcp_state_names			\
> +		EM(TCP_ESTABLISHED)		\
> +		EM(TCP_SYN_SENT)		\
> +		EM(TCP_SYN_RECV)		\
> +		EM(TCP_FIN_WAIT1)	   \
> +		EM(TCP_FIN_WAIT2)	   \
> +		EM(TCP_TIME_WAIT)	   \
> +		EM(TCP_CLOSE)		   \
> +		EM(TCP_CLOSE_WAIT)	  \
> +		EM(TCP_LAST_ACK)		\
> +		EM(TCP_LISTEN)		  \
> +		EM(TCP_CLOSING)		 \
> +		EMe(TCP_NEW_SYN_RECV)

Please keep these backslashes aligned. 

> +/* enums need to be exported to user space */
> +#undef EM
> +#undef EMe
> +#define EM(a)       TRACE_DEFINE_ENUM(a);
> +#define EMe(a)      TRACE_DEFINE_ENUM(a);
> +
> +inet_protocol_names
> +tcp_state_names
> +
> +#undef EM
> +#undef EMe
> +#define EM(a)       { a, #a },
> +#define EMe(a)      { a, #a }
> +
> +#define show_inet_protocol_name(val)	\
> +	__print_symbolic(val, inet_protocol_names)
> +
> +#define show_tcp_state_name(val)		\
> +	__print_symbolic(val, tcp_state_names)
> 
> TRACE_EVENT(sock_rcvqueue_full,
> 
> @@ -63,6 +105,70 @@
> 		__entry->rmem_alloc)
> );
> 
> +TRACE_EVENT(sock_set_state,
> +
> +	TP_PROTO(const struct sock *sk, const int oldstate, const int newstate),
> +
> +	TP_ARGS(sk, oldstate, newstate),
> +
> +	TP_STRUCT__entry(
> +		__field(const void *, skaddr)
> +		__field(int, oldstate)
> +		__field(int, newstate)
> +		__field(__u16, sport)
> +		__field(__u16, dport)
> +		__field(__u8, protocol)
> +		__array(__u8, saddr, 4)
> +		__array(__u8, daddr, 4)
> +		__array(__u8, saddr_v6, 16)
> +		__array(__u8, daddr_v6, 16)
> +	),
> +
> +	TP_fast_assign(
> +		struct inet_sock *inet = inet_sk(sk);
> +		struct in6_addr *pin6;
> +		__be32 *p32;
> +
> +		__entry->skaddr = sk;
> +		__entry->oldstate = oldstate;
> +		__entry->newstate = newstate;
> +
> +		__entry->protocol = sk->sk_protocol;
> +		__entry->sport = ntohs(inet->inet_sport);
> +		__entry->dport = ntohs(inet->inet_dport);
> +
> +		p32 = (__be32 *) __entry->saddr;
> +		*p32 = inet->inet_saddr;
> +
> +		p32 = (__be32 *) __entry->daddr;
> +		*p32 =  inet->inet_daddr;
> +
> +#if IS_ENABLED(CONFIG_IPV6)
> +		if (sk->sk_family == AF_INET6) {
> +			pin6 = (struct in6_addr *)__entry->saddr_v6;
> +			*pin6 = sk->sk_v6_rcv_saddr;
> +			pin6 = (struct in6_addr *)__entry->daddr_v6;
> +			*pin6 = sk->sk_v6_daddr;
> +		} else
> +#endif
> +		{
> +			pin6 = (struct in6_addr *)__entry->saddr_v6;
> +			ipv6_addr_set_v4mapped(inet->inet_saddr, pin6);
> +			pin6 = (struct in6_addr *)__entry->daddr_v6;
> +			ipv6_addr_set_v4mapped(inet->inet_daddr, pin6);
> +		}
> +	),

What if sk_family is not AF_INET or AF_INET6? We are probably OK not 
checking it for tcp, but we should definitely consider this for all 
sockets in general. 

Thanks,
Song

> +	TP_printk("protocol=%s sport=%hu dport=%hu saddr=%pI4 daddr=%pI4"
> +			"saddrv6=%pI6c daddrv6=%pI6c oldstate=%s newstate=%s",
> +			show_inet_protocol_name(__entry->protocol),
> +			__entry->sport, __entry->dport,
> +			__entry->saddr, __entry->daddr,
> +			__entry->saddr_v6, __entry->daddr_v6,
> +			show_tcp_state_name(__entry->oldstate),
> +			show_tcp_state_name(__entry->newstate))
> +);
> +
> #endif /* _TRACE_SOCK_H */
> 
> /* This part must be outside protection */
> diff --git a/include/trace/events/tcp.h b/include/trace/events/tcp.h
> index 40240ac..7399399 100644
> --- a/include/trace/events/tcp.h
> +++ b/include/trace/events/tcp.h
> @@ -9,37 +9,6 @@
> #include <linux/tracepoint.h>
> #include <net/ipv6.h>
> 
> -#define tcp_state_names			\
> -		EM(TCP_ESTABLISHED)		\
> -		EM(TCP_SYN_SENT)		\
> -		EM(TCP_SYN_RECV)		\
> -		EM(TCP_FIN_WAIT1)		\
> -		EM(TCP_FIN_WAIT2)		\
> -		EM(TCP_TIME_WAIT)		\
> -		EM(TCP_CLOSE)			\
> -		EM(TCP_CLOSE_WAIT)		\
> -		EM(TCP_LAST_ACK)		\
> -		EM(TCP_LISTEN)			\
> -		EM(TCP_CLOSING)			\
> -		EMe(TCP_NEW_SYN_RECV)	\
> -
> -/* enums need to be exported to user space */
> -#undef EM
> -#undef EMe
> -#define EM(a)         TRACE_DEFINE_ENUM(a);
> -#define EMe(a)        TRACE_DEFINE_ENUM(a);
> -
> -tcp_state_names
> -
> -#undef EM
> -#undef EMe
> -#define EM(a)         tcp_state_name(a),
> -#define EMe(a)        tcp_state_name(a)
> -
> -#define tcp_state_name(state)	{ state, #state }
> -#define show_tcp_state_name(val)			\
> -	__print_symbolic(val, tcp_state_names)
> -
> /*
>  * tcp event with arguments sk and skb
>  *
> @@ -192,66 +161,6 @@
> 	TP_ARGS(sk)
> );
> 
> -TRACE_EVENT(tcp_set_state,
> -
> -	TP_PROTO(const struct sock *sk, const int oldstate, const int newstate),
> -
> -	TP_ARGS(sk, oldstate, newstate),
> -
> -	TP_STRUCT__entry(
> -		__field(const void *, skaddr)
> -		__field(int, oldstate)
> -		__field(int, newstate)
> -		__field(__u16, sport)
> -		__field(__u16, dport)
> -		__array(__u8, saddr, 4)
> -		__array(__u8, daddr, 4)
> -		__array(__u8, saddr_v6, 16)
> -		__array(__u8, daddr_v6, 16)
> -	),
> -
> -	TP_fast_assign(
> -		struct inet_sock *inet = inet_sk(sk);
> -		struct in6_addr *pin6;
> -		__be32 *p32;
> -
> -		__entry->skaddr = sk;
> -		__entry->oldstate = oldstate;
> -		__entry->newstate = newstate;
> -
> -		__entry->sport = ntohs(inet->inet_sport);
> -		__entry->dport = ntohs(inet->inet_dport);
> -
> -		p32 = (__be32 *) __entry->saddr;
> -		*p32 = inet->inet_saddr;
> -
> -		p32 = (__be32 *) __entry->daddr;
> -		*p32 =  inet->inet_daddr;
> -
> -#if IS_ENABLED(CONFIG_IPV6)
> -		if (sk->sk_family == AF_INET6) {
> -			pin6 = (struct in6_addr *)__entry->saddr_v6;
> -			*pin6 = sk->sk_v6_rcv_saddr;
> -			pin6 = (struct in6_addr *)__entry->daddr_v6;
> -			*pin6 = sk->sk_v6_daddr;
> -		} else
> -#endif
> -		{
> -			pin6 = (struct in6_addr *)__entry->saddr_v6;
> -			ipv6_addr_set_v4mapped(inet->inet_saddr, pin6);
> -			pin6 = (struct in6_addr *)__entry->daddr_v6;
> -			ipv6_addr_set_v4mapped(inet->inet_daddr, pin6);
> -		}
> -	),
> -
> -	TP_printk("sport=%hu dport=%hu saddr=%pI4 daddr=%pI4 saddrv6=%pI6c daddrv6=%pI6c oldstate=%s newstate=%s",
> -		  __entry->sport, __entry->dport,
> -		  __entry->saddr, __entry->daddr,
> -		  __entry->saddr_v6, __entry->daddr_v6,
> -		  show_tcp_state_name(__entry->oldstate),
> -		  show_tcp_state_name(__entry->newstate))
> -);
> -
> TRACE_EVENT(tcp_retransmit_synack,
> 
> 	TP_PROTO(const struct sock *sk, const struct request_sock *req),
> diff --git a/net/core/sock.c b/net/core/sock.c
> index c0b5b2f..717f7f6 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -2859,6 +2859,19 @@ int sock_get_timestampns(struct sock *sk, struct timespec __user *userstamp)
> }
> EXPORT_SYMBOL(sock_get_timestampns);
> 
> +void sk_state_store(struct sock *sk, int state)
> +{
> +	trace_sock_set_state(sk, sk->sk_state, state);
> +	smp_store_release(&sk->sk_state, state);
> +}
> +
> +void sk_set_state(struct sock *sk, int state)
> +{
> +	trace_sock_set_state(sk, sk->sk_state, state);
> +	 sk->sk_state = state;
> +}
> +EXPORT_SYMBOL(sk_set_state);
> +
> void sock_enable_timestamp(struct sock *sk, int flag)
> {
> 	if (!sock_flag(sk, flag)) {
> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index 4ca46dc..001f7b0 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;
> +		sk_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;
> +	sk_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..5973693 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;
> +		sk_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 c470fec..df6da92 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -283,8 +283,6 @@
> #include <asm/ioctls.h>
> #include <net/busy_poll.h>
> 
> -#include <trace/events/tcp.h>
> -
> struct percpu_counter tcp_orphan_count;
> EXPORT_SYMBOL_GPL(tcp_orphan_count);
> 
> @@ -2040,8 +2038,6 @@ void tcp_set_state(struct sock *sk, int state)
> {
> 	int oldstate = sk->sk_state;
> 
> -	trace_tcp_set_state(sk, oldstate, state);
> -
> 	switch (state) {
> 	case TCP_ESTABLISHED:
> 		if (oldstate != TCP_ESTABLISHED)
> --
> 1.8.3.1
>