2017-12-08 9:41 GMT+08:00 Yafang Shao <laoar.s...@gmail.com>:
> 2017-12-08 4:02 GMT+08:00 Marcelo Ricardo Leitner <marcelo.leit...@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.s...@gmail.com>
>>> Acked-by: Song Liu <songliubrav...@fb.com>
>>> Reviewed-by: Marcelo Ricardo Leitner <marcelo.leit...@gmail.com>
>>> Signed-off-by: Yafang Shao <laoar.s...@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
