Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752565AbdLMJvD (ORCPT ); Wed, 13 Dec 2017 04:51:03 -0500 Received: from mail-io0-f193.google.com ([209.85.223.193]:42545 "EHLO mail-io0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752377AbdLMJu5 (ORCPT ); Wed, 13 Dec 2017 04:50:57 -0500 X-Google-Smtp-Source: ACJfBov7BUPn/7X1rCBeMsfwoGYU6EwOxI3YhDDbsxkONLVYYv0RzNGk9ZZGNTbioF4SU9b7nQSfWzELkTjx14woK/U= MIME-Version: 1.0 In-Reply-To: <662FAEA6-5A62-4950-8D6D-66776C35E8FE@fb.com> References: <1512919904-14166-1-git-send-email-laoar.shao@gmail.com> <1512919904-14166-2-git-send-email-laoar.shao@gmail.com> <662FAEA6-5A62-4950-8D6D-66776C35E8FE@fb.com> From: Yafang Shao Date: Wed, 13 Dec 2017 17:50:15 +0800 Message-ID: Subject: Re: [PATCH net-next 1/3] net:tracepoint: replace tcp_set_state tracepoint with sock_set_state tracepoint To: Song Liu Cc: David Miller , "marcelo.leitner@gmail.com" , "edumazet@google.com" , Cong Wang , "mingo@redhat.com" , "kuznet@ms2.inr.ac.ru" , "yoshfuji@linux-ipv6.org" , 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: 13572 Lines: 374 2017-12-13 9:19 GMT+08:00 Song Liu : > >> On Dec 10, 2017, at 7:31 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. >> >> The minor changes against on the original tcp_set_state tracepoint: >> - Protocol name is printed to distinguish which protocol it is belonging >> to >> - The macros defined in the file are undefed at the end of this file as >> they are only used in this file >> >> 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 | 95 +++++++++++++++++++++++++++++++++++++++++ >> include/trace/events/tcp.h | 76 --------------------------------- >> 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, 114 insertions(+), 95 deletions(-) >> >> diff --git a/include/net/sock.h b/include/net/sock.h >> index 79e1a2c..b307b60 100644 >> --- a/include/net/sock.h >> +++ b/include/net/sock.h >> @@ -2348,18 +2348,9 @@ 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); >> -} >> +/* For sock_set_state tracepoint */ >> +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 *); >> diff --git a/include/trace/events/sock.h b/include/trace/events/sock.h >> index ec4dade..2728892 100644 >> --- a/include/trace/events/sock.h >> +++ b/include/trace/events/sock.h >> @@ -6,7 +6,33 @@ >> #define _TRACE_SOCK_H >> >> #include >> +#include >> #include >> +#include >> +#include >> + >> +#define inet_protocol_name(protocol) {IPPROTO_##protocol, #protocol} >> +#define show_inet_protocol_name(val) \ >> + __print_symbolic(val, \ >> + inet_protocol_name(TCP), \ >> + inet_protocol_name(DCCP), \ >> + inet_protocol_name(SCTP)) >> + >> +#define tcp_state_name(state) { state, #state } >> +#define show_tcp_state_name(val) \ >> + __print_symbolic(val, \ >> + tcp_state_name(TCP_ESTABLISHED), \ >> + tcp_state_name(TCP_SYN_SENT), \ >> + tcp_state_name(TCP_SYN_RECV), \ >> + tcp_state_name(TCP_FIN_WAIT1), \ >> + tcp_state_name(TCP_FIN_WAIT2), \ >> + tcp_state_name(TCP_TIME_WAIT), \ >> + tcp_state_name(TCP_CLOSE), \ >> + tcp_state_name(TCP_CLOSE_WAIT), \ >> + tcp_state_name(TCP_LAST_ACK), \ >> + tcp_state_name(TCP_LISTEN), \ >> + tcp_state_name(TCP_CLOSING), \ >> + tcp_state_name(TCP_NEW_SYN_RECV)) > > Please include Steven's fix to export names to user space: > > https://patchwork.kernel.org/patch/10052201/ > OK. > >> TRACE_EVENT(sock_rcvqueue_full, >> >> @@ -63,6 +89,75 @@ >> __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); >> + } >> + ), >> + >> + 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)) >> +); >> + >> +#undef show_tcp_state_name >> +#undef tcp_state_name >> +#undef show_inet_protocol_name >> +#undef inet_protocol_name >> + >> #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 07cccca..7399399 100644 >> --- a/include/trace/events/tcp.h >> +++ b/include/trace/events/tcp.h >> @@ -9,22 +9,6 @@ >> #include >> #include >> >> -#define tcp_state_name(state) { state, #state } >> -#define show_tcp_state_name(val) \ >> - __print_symbolic(val, \ >> - tcp_state_name(TCP_ESTABLISHED), \ >> - tcp_state_name(TCP_SYN_SENT), \ >> - tcp_state_name(TCP_SYN_RECV), \ >> - tcp_state_name(TCP_FIN_WAIT1), \ >> - tcp_state_name(TCP_FIN_WAIT2), \ >> - tcp_state_name(TCP_TIME_WAIT), \ >> - tcp_state_name(TCP_CLOSE), \ >> - tcp_state_name(TCP_CLOSE_WAIT), \ >> - tcp_state_name(TCP_LAST_ACK), \ >> - tcp_state_name(TCP_LISTEN), \ >> - tcp_state_name(TCP_CLOSING), \ >> - tcp_state_name(TCP_NEW_SYN_RECV)) >> - >> /* >> * tcp event with arguments sk and skb >> * >> @@ -177,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..ee0c1bc 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); > > These two functions are short. Can we keep this in sock.h? sock.h is included in trace/events/sock.h, so it will cause compile error if hese two functions are defined in sock.h. > > Thanks, > Song > >> + >> 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 1803116..000504f 100644 >> --- a/net/ipv4/tcp.c >> +++ b/net/ipv4/tcp.c >> @@ -283,8 +283,6 @@ >> #include >> #include >> >> -#include >> - >> 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 >> >