Re: [PATCH v5 net-next] net/tcp: trace all TCP/IP state transition with tcp_set_state tracepoint

[Yafang Shao <laoar.shao@gmail.com>]

2017-12-08 9:41 GMT+08:00 Yafang Shao <laoar.shao@gmail.com>:
> 2017-12-08 4:02 GMT+08:00 Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>:
>> On Thu, Dec 07, 2017 at 02:10:42PM +0000, Yafang Shao wrote:
>>> The TCP/IP transition from TCP_LISTEN to TCP_SYN_RECV and some other
>>> transitions are not traced with tcp_set_state tracepoint.
>>>
>>> In order to trace the whole tcp lifespans, two helpers are introduced,
>>> void sk_set_state(struct sock *sk, int state);
>>> void sk_state_store(struct sock *sk, int newstate);
>>>
>>> When do TCP/IP state transition, we should use these two helpers or use
>>> tcp_set_state() other than assigning a value to sk_state directly.
>>>
>>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>>> Acked-by: Song Liu <songliubraving@fb.com>
>>> Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
>>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>>> ---
>>> v4->v5: Trace only TCP sockets, whatever it is stream socket or raw socket.
>>> v3->v4: Do not trace DCCP socket
>>> v2->v3: Per suggestion from Marcelo Ricardo Leitner, inverting  __
>>>                to sk_state_store.
>>> ---
>>>  include/net/sock.h              |  8 ++++++--
>>>  net/core/sock.c                 | 15 +++++++++++++++
>>>  net/ipv4/inet_connection_sock.c |  5 +++--
>>>  net/ipv4/inet_hashtables.c      |  2 +-
>>>  net/ipv4/tcp.c                  |  2 +-
>>>  5 files changed, 26 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/include/net/sock.h b/include/net/sock.h
>>> index 79e1a2c..1cf7685 100644
>>> --- a/include/net/sock.h
>>> +++ b/include/net/sock.h
>>> @@ -2349,18 +2349,22 @@ static inline int sk_state_load(const struct sock *sk)
>>>  }
>>>
>>>  /**
>>> - * sk_state_store - update 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)
>>> +static inline void __sk_state_store(struct sock *sk, int newstate)
>>>  {
>>>       smp_store_release(&sk->sk_state, newstate);
>>>  }
>>>
>>> +/* For tcp_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 *);
>>>  int sock_get_timestampns(struct sock *, struct timespec __user *);
>>> diff --git a/net/core/sock.c b/net/core/sock.c
>>> index c0b5b2f..61841a2 100644
>>> --- a/net/core/sock.c
>>> +++ b/net/core/sock.c
>>> @@ -138,6 +138,7 @@
>>>  #include <net/sock_reuseport.h>
>>>
>>>  #include <trace/events/sock.h>
>>> +#include <trace/events/tcp.h>
>>>
>>>  #include <net/tcp.h>
>>>  #include <net/busy_poll.h>
>>> @@ -2859,6 +2860,20 @@ int sock_get_timestampns(struct sock *sk, struct timespec __user *userstamp)
>>>  }
>>>  EXPORT_SYMBOL(sock_get_timestampns);
>>>
>>> +void sk_state_store(struct sock *sk, int newstate)
>>> +{
>>> +     if (sk->sk_protocol == IPPROTO_TCP)
>>> +             trace_tcp_set_state(sk, sk->sk_state, newstate);
>>
>> I think this is going in the wrong way. When Dave said to not define a
>> sock generic function in tcp code on v3, you moved it all from tcp.h
>> to sock.h. But now sock.h gets to deal with more tcp code, which also
>> isn't nice.
>>
>> Instead, if you move it back to tcp.h but rename the function to
>> tcp_state_store (instead of the original sk_state_store), it won't be
>> a generic sock code and will fit nicely into tcp.h.
>>
>> You may then even keep sk_state_store() as it is now, and just define
>> tcp_state_store():
>>
>> tcp_state_store()
>> {
>>         trace_tcp_...();
>>         sk_state_store();
>> }
>>
>> Making it very clear that this code is only to be used by tcp stack.
>>
>
> Then we have to do bellow 'if' test in inet_connection_sock.c and
> /inet_hashtables.
>
> if (sk->sk_protocol == IPPROTO_TCP)
>     tcp_state_store(sk, TCP_CLOSE)
> else
>     sk->sk_state = TCP_CLOSE;
>
> And same code about other changes.
>
> Is that proper ?
>
>

It will looks like these,

    if (sk->sk_protocol == IPPROTO_TCP)
        __tcp_set_state(newsk, TCP_SYN_RECV);
    else
        newsk->sk_state = TCP_SYN_RECV;


    if (sk->sk_protocol == IPPROTO_TCP)
          __tcp_set_state(sk, TCP_CLOSE);
    else
          sk->sk_state = TCP_CLOSE;

    if (sk->sk_protocol == IPPROTO_TCP)
          tcp_state_store(sk,  state);
    else
          sk_state_store(sk, state);


Some redundant code.

IMO, put these similar code into a wrapper is more nice.

Thanks
Yafang