Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751658AbdLPGen (ORCPT ); Sat, 16 Dec 2017 01:34:43 -0500 Received: from mail-io0-f193.google.com ([209.85.223.193]:40470 "EHLO mail-io0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750730AbdLPGel (ORCPT ); Sat, 16 Dec 2017 01:34:41 -0500 X-Google-Smtp-Source: ACJfBounYUe0Z+uUzJtzWCYEoe7nCtz+Ov7KctI1/IfduZ3t3+hxVYixfSZBSsEgtAS0aQMVqkbxrUFHwyCkrNFzLxA= MIME-Version: 1.0 In-Reply-To: <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> <4AB9713E-7E2D-4232-868B-ACE8364E33A0@fb.com> From: Yafang Shao Date: Sat, 16 Dec 2017 14:34:00 +0800 Message-ID: Subject: Re: [PATCH v2 net-next 2/4] net: tracepoint: replace tcp_set_state tracepoint with sock_set_state tracepoint To: Song Liu Cc: David Miller , "marcelo.leitner@gmail.com" , Steven Rostedt , Brendan Gregg , "netdev@vger.kernel.org" , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6929 Lines: 197 2017-12-16 6:47 GMT+08:00 Song Liu : > >> On Dec 15, 2017, at 9:56 AM, Yafang Shao 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 >> --- >> 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 >> +#include >> #include >> +#include >> +#include >> + >> +#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. > OK This is because I made it aligned with TAB in my original code. >> +/* 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. > This is the question I had been think of. Do it make sense to trace as much protocol as possible ? Maybe not. Take IPPROTO_UDP for example, it only has two states, TCP_CLOSE and TCP_ESTABLISHED. Maybe it is useless to trace UDP sk_state transition. So In this patch I only trace TCP/DCCP/SCTP state transition, which have multi states and the states transition could help us analyze problems. All these three protocol are AF_INET/AF_INET6. IMO, maybe it doesn't need to trace protocols which are not AF_INET/AF_INET6. So we don't need to make the code complicate and output some usless infomation. Thanks Yafang