Re: [PATCH v2 net-next 2/4] net: tracepoint: replace tcp_set_state tracepoint with sock_set_state tracepoint

From: Song Liu
Date: Fri Dec 15 2017 - 17:47:23 EST

Next message: Rob Herring: "Re: [PATCH v11 1/2] dt: bindings: lm3692x: Add bindings for lm3692x LED driver"
Previous message: Matthew Wilcox: "[PATCH v5 42/78] shmem: Convert replace to XArray"
In reply to: Yafang Shao: "[PATCH v2 net-next 2/4] net: tracepoint: replace tcp_set_state tracepoint with sock_set_state tracepoint"
Next in thread: Yafang Shao: "[PATCH v2 net-next 3/4] net: tracepoint: using sock_set_state tracepoint to trace DCCP state transition"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> On Dec 15, 2017, at 9:56 AM, Yafang Shao <laoar.shao@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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
>

Next message: Rob Herring: "Re: [PATCH v11 1/2] dt: bindings: lm3692x: Add bindings for lm3692x LED driver"
Previous message: Matthew Wilcox: "[PATCH v5 42/78] shmem: Convert replace to XArray"
In reply to: Yafang Shao: "[PATCH v2 net-next 2/4] net: tracepoint: replace tcp_set_state tracepoint with sock_set_state tracepoint"
Next in thread: Yafang Shao: "[PATCH v2 net-next 3/4] net: tracepoint: using sock_set_state tracepoint to trace DCCP state transition"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]