Re: [PATCH] net/tcp: introduce TRACE_EVENT for TCP/IPv4 state transition

[Yafang Shao]

2017-11-09 14:43 GMT+08:00 Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx>:
> On Thu, Nov 09, 2017 at 02:01:38PM +0800, Yafang Shao wrote:
>> With this newly introduced TRACE_EVENT, it will be very easy to minotor
>> TCP/IPv4 state transition.
>>
>> A new TRACE_SYSTEM named tcp is added, in which we can trace other TCP
>> event as well.
>>
>> Two helpers are added,
>> static inline void __tcp_set_state(struct sock *sk, int state)
>> static inline void __sk_state_store(struct sock *sk, int newstate)
>>
>> When do TCP/IPv4 state transition, we should use these two helpers or
>> use tcp_set_state() instead of assign a value to sk_state directly.
>>
>> Signed-off-by: Yafang Shao <laoar.shao@xxxxxxxxx>
>
> when you submit a patch pls make it clear which tree this patch is targeting.
> In this case it should have been net-next,
> but the patch clearly conflicts with it.
> Make sure to rebase.

I do it based on this tree
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
OK I will check the net-next then.

>
>> +/*
>> + * To trace TCP state transition.
>> + */
>> +static inline void __tcp_set_state(struct sock *sk, int state)
>> +{
>> +     trace_tcp_set_state(sk, sk->sk_state, state);
>> +     sk->sk_state = state;
>> +}
>> +
>> +static inline void __sk_state_store(struct sock *sk, int newstate)
>> +{
>> +     trace_tcp_set_state(sk, sk->sk_state, newstate);
>> +     sk_state_store(sk, newstate);
>> +}
>> +
>>  void tcp_done(struct sock *sk);
>>
>>  int tcp_abort(struct sock *sk, int err);
>> diff --git a/include/trace/events/tcp.h b/include/trace/events/tcp.h
>> new file mode 100644
>> index 0000000..abf65af
>> --- /dev/null
>> +++ b/include/trace/events/tcp.h
>> @@ -0,0 +1,58 @@
>> +#undef TRACE_SYSTEM
>> +#define TRACE_SYSTEM tcp
>> +
>> +#if !defined(_TRACE_TCP_H) || defined(TRACE_HEADER_MULTI_READ)
>> +#define _TRACE_TCP_H
>> +
>> +#include <linux/tracepoint.h>
>> +#include <net/sock.h>
>> +#include <net/inet_timewait_sock.h>
>> +#include <net/request_sock.h>
>> +#include <net/inet_sock.h>
>> +#include <net/tcp_states.h>
>> +
>> +TRACE_EVENT(tcp_set_state,
>> +     TP_PROTO(struct sock *sk, int oldstate, int newstate),
>> +     TP_ARGS(sk, oldstate, newstate),
>> +
>> +     TP_STRUCT__entry(
>> +             __field(__be32, dst)
>> +             __field(__be32, src)
>> +             __field(__u16, dport)
>> +             __field(__u16, sport)
>> +             __field(int, oldstate)
>> +             __field(int, newstate)
>> +     ),
>> +
>> +     TP_fast_assign(
>> +             if (oldstate == TCP_TIME_WAIT) {
>> +                     __entry->dst = inet_twsk(sk)->tw_daddr;
>> +                     __entry->src = inet_twsk(sk)->tw_rcv_saddr;
>> +                     __entry->dport = ntohs(inet_twsk(sk)->tw_dport);
>> +                     __entry->sport = ntohs(inet_twsk(sk)->tw_sport);
>> +             } else if (oldstate == TCP_NEW_SYN_RECV) {
>> +                     __entry->dst = inet_rsk(inet_reqsk(sk))->ir_rmt_addr;
>> +                     __entry->src = inet_rsk(inet_reqsk(sk))->ir_loc_addr;
>> +                     __entry->dport =
>> +                             ntohs(inet_rsk(inet_reqsk(sk))->ir_rmt_port);
>> +                     __entry->sport = inet_rsk(inet_reqsk(sk))->ir_num;
>> +             } else {
>> +                     __entry->dst = inet_sk(sk)->inet_daddr;
>> +                     __entry->src = inet_sk(sk)->inet_rcv_saddr;
>> +                     __entry->dport = ntohs(inet_sk(sk)->inet_dport);
>> +                     __entry->sport = ntohs(inet_sk(sk)->inet_sport);
>> +             }
>> +
>> +             __entry->oldstate = oldstate;
>> +             __entry->newstate = newstate;
>> +     ),
>> +
>> +     TP_printk("%08X:%04X %08X:%04X, %02x %02x",
>> +             __entry->src, __entry->sport, __entry->dst, __entry->dport,
>> +             __entry->oldstate, __entry->newstate)
>
> direct %x of state is not allowed.
> This has to use show_tcp_state_name() like it's done in trace_tcp_set_state
>
> Also I'm missing the reason to introduce another tracepoint
> that looks just like trace_tcp_set_state.
>

I will check net-next

Thanks
Yafang