Re: [PATCH v2 net-next 2/4] net: tracepoint: replace tcp_set_state tracepoint with sock_set_state tracepoint
From: Yafang Shao
Date: Sat Dec 16 2017 - 01:34:59 EST
2017-12-16 6:47 GMT+08:00 Song Liu <songliubraving@xxxxxx>:
>
>> 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.
>
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