On Tue, Aug 8, 2023 at 7:59 AM Manjusaka <[email protected]> wrote:
>
> In normal use case, the tcp_ca_event would be changed in high frequency.
>
> It's a good indicator to represent the network quanlity.
quality ?
Honestly, it is more about TCP stack tracing than 'network quality'
>
> So I propose to add a `tcp:tcp_ca_event` trace event
> like `tcp:tcp_cong_state_set` to help the people to
> trace the TCP connection status
>
> Signed-off-by: Manjusaka <[email protected]>
> ---
> include/net/tcp.h | 9 ++------
> include/trace/events/tcp.h | 45 ++++++++++++++++++++++++++++++++++++++
> net/ipv4/tcp_cong.c | 10 +++++++++
> 3 files changed, 57 insertions(+), 7 deletions(-)
>
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 0ca972ebd3dd..a68c5b61889c 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1154,13 +1154,8 @@ static inline bool tcp_ca_needs_ecn(const struct sock *sk)
> return icsk->icsk_ca_ops->flags & TCP_CONG_NEEDS_ECN;
> }
>
> -static inline void tcp_ca_event(struct sock *sk, const enum tcp_ca_event event)
> -{
> - const struct inet_connection_sock *icsk = inet_csk(sk);
> -
> - if (icsk->icsk_ca_ops->cwnd_event)
> - icsk->icsk_ca_ops->cwnd_event(sk, event);
> -}
> +/* from tcp_cong.c */
> +void tcp_ca_event(struct sock *sk, const enum tcp_ca_event event);
>
> /* From tcp_cong.c */
> void tcp_set_ca_state(struct sock *sk, const u8 ca_state);
> diff --git a/include/trace/events/tcp.h b/include/trace/events/tcp.h
> index bf06db8d2046..b374eb636af9 100644
> --- a/include/trace/events/tcp.h
> +++ b/include/trace/events/tcp.h
> @@ -416,6 +416,51 @@ TRACE_EVENT(tcp_cong_state_set,
> __entry->cong_state)
> );
>
> +TRACE_EVENT(tcp_ca_event,
> +
> + TP_PROTO(struct sock *sk, const u8 ca_event),
> +
> + TP_ARGS(sk, ca_event),
> +
> + TP_STRUCT__entry(
> + __field(const void *, skaddr)
> + __field(__u16, sport)
> + __field(__u16, dport)
> + __array(__u8, saddr, 4)
> + __array(__u8, daddr, 4)
> + __array(__u8, saddr_v6, 16)
> + __array(__u8, daddr_v6, 16)
> + __field(__u8, ca_event)
> + ),
> +
Please add the family (look at commit 3dd344ea84e1 ("net: tracepoint:
exposing sk_family in all tcp:tracepoints"))
> + TP_fast_assign(
> + struct inet_sock *inet = inet_sk(sk);
> + __be32 *p32;
> +
> + __entry->skaddr = sk;
> +
> + __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;
We keep copying IPv4 addresses that might contain garbage for IPv6 sockets :/
> +
> + TP_STORE_ADDRS(__entry, inet->inet_saddr, inet->inet_daddr,
> + sk->sk_v6_rcv_saddr, sk->sk_v6_daddr);
I will send a cleanup, because IP_STORE_ADDRS() should really take
care of all details.
> +
> + __entry->ca_event = ca_event;
> + ),
> +
> + TP_printk("sport=%hu dport=%hu saddr=%pI4 daddr=%pI4 saddrv6=%pI6c daddrv6=%pI6c ca_event=%u",
> + __entry->sport, __entry->dport,
> + __entry->saddr, __entry->daddr,
> + __entry->saddr_v6, __entry->daddr_v6,
> + __entry->ca_event)
Please print the symbol instead of numeric ca_event.
Look at show_tcp_state_name() for instance.
On 2023/8/8 16:26, Eric Dumazet wrote:
> On Tue, Aug 8, 2023 at 7:59 AM Manjusaka <[email protected]> wrote:
>>
>> In normal use case, the tcp_ca_event would be changed in high frequency.
>>
>> It's a good indicator to represent the network quanlity.
>
> quality ?
>
> Honestly, it is more about TCP stack tracing than 'network quality'
>
>>
>> So I propose to add a `tcp:tcp_ca_event` trace event
>> like `tcp:tcp_cong_state_set` to help the people to
>> trace the TCP connection status
>>
>> Signed-off-by: Manjusaka <[email protected]>
>> ---
>> include/net/tcp.h | 9 ++------
>> include/trace/events/tcp.h | 45 ++++++++++++++++++++++++++++++++++++++
>> net/ipv4/tcp_cong.c | 10 +++++++++
>> 3 files changed, 57 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/net/tcp.h b/include/net/tcp.h
>> index 0ca972ebd3dd..a68c5b61889c 100644
>> --- a/include/net/tcp.h
>> +++ b/include/net/tcp.h
>> @@ -1154,13 +1154,8 @@ static inline bool tcp_ca_needs_ecn(const struct sock *sk)
>> return icsk->icsk_ca_ops->flags & TCP_CONG_NEEDS_ECN;
>> }
>>
>> -static inline void tcp_ca_event(struct sock *sk, const enum tcp_ca_event event)
>> -{
>> - const struct inet_connection_sock *icsk = inet_csk(sk);
>> -
>> - if (icsk->icsk_ca_ops->cwnd_event)
>> - icsk->icsk_ca_ops->cwnd_event(sk, event);
>> -}
>> +/* from tcp_cong.c */
>> +void tcp_ca_event(struct sock *sk, const enum tcp_ca_event event);
>>
>> /* From tcp_cong.c */
>> void tcp_set_ca_state(struct sock *sk, const u8 ca_state);
>> diff --git a/include/trace/events/tcp.h b/include/trace/events/tcp.h
>> index bf06db8d2046..b374eb636af9 100644
>> --- a/include/trace/events/tcp.h
>> +++ b/include/trace/events/tcp.h
>> @@ -416,6 +416,51 @@ TRACE_EVENT(tcp_cong_state_set,
>> __entry->cong_state)
>> );
>>
>> +TRACE_EVENT(tcp_ca_event,
>> +
>> + TP_PROTO(struct sock *sk, const u8 ca_event),
>> +
>> + TP_ARGS(sk, ca_event),
>> +
>> + TP_STRUCT__entry(
>> + __field(const void *, skaddr)
>> + __field(__u16, sport)
>> + __field(__u16, dport)
>> + __array(__u8, saddr, 4)
>> + __array(__u8, daddr, 4)
>> + __array(__u8, saddr_v6, 16)
>> + __array(__u8, daddr_v6, 16)
>> + __field(__u8, ca_event)
>> + ),
>> +
>
> Please add the family (look at commit 3dd344ea84e1 ("net: tracepoint:
> exposing sk_family in all tcp:tracepoints"))
>
>
>
>> + TP_fast_assign(
>> + struct inet_sock *inet = inet_sk(sk);
>> + __be32 *p32;
>> +
>> + __entry->skaddr = sk;
>> +
>> + __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;
>
> We keep copying IPv4 addresses that might contain garbage for IPv6 sockets :/
>
>> +
>> + TP_STORE_ADDRS(__entry, inet->inet_saddr, inet->inet_daddr,
>> + sk->sk_v6_rcv_saddr, sk->sk_v6_daddr);
>
> I will send a cleanup, because IP_STORE_ADDRS() should really take
> care of all details.
>
>
>> +
>> + __entry->ca_event = ca_event;
>> + ),
>> +
>> + TP_printk("sport=%hu dport=%hu saddr=%pI4 daddr=%pI4 saddrv6=%pI6c daddrv6=%pI6c ca_event=%u",
>> + __entry->sport, __entry->dport,
>> + __entry->saddr, __entry->daddr,
>> + __entry->saddr_v6, __entry->daddr_v6,
>> + __entry->ca_event)
>
> Please print the symbol instead of numeric ca_event.
>
> Look at show_tcp_state_name() for instance.
Thanks for the kindness code review, I still get some issue here(Sorry for the first time to contribute):
1. > We keep copying IPv4 addresses that might contain garbage for IPv6 sockets :/
I'm not getting your means, would you mean that we should only save the IPv4 Address here?
2. > I will send a cleanup, because IP_STORE_ADDRS() should really take care of all details.
I think you will make the address assignment code in TP_fast_assign as a new function.
Should I submit the new change until you send the cleanup patch or I can make this in my patch(cleanup the address assignment)