linux-kernel - Re: [PATCH] tracepoint: add new `tcp:tcp_ca_event

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 7 Aug 2023 16:00:18 -0400
From:   Neal Cardwell <ncardwell@...gle.com>
To:     Manjusaka <me@...jusaka.me>
Cc:     edumazet@...gle.com, mhiramat@...nel.org, rostedt@...dmis.org,
        davem@...emloft.net, dsahern@...nel.org, kuba@...nel.org,
        pabeni@...hat.com, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
        bpf@...r.kernel.org
Subject: Re: [PATCH] tracepoint: add new `tcp:tcp_ca_event_set` trace event

On Mon, Aug 7, 2023 at 2:33 PM Manjusaka <me@...jusaka.me> 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.
>
> So I propose to add a `tcp:tcp_ca_event_set` trace event
> like `tcp:tcp_cong_state_set` to help the people to
> trace the TCP connection status
>
> Signed-off-by: Manjusaka <me@...jusaka.me>
> ---
>  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..38415c5f1d52 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_set,

Regarding the proposed name, "tcp_ca_event_set"... including "set" in
the name is confusing, since the tcp_ca_event() function is not really
setting anything. :-)

The trace_tcp_cong_state_set() call you are using as a model has "set"
in the name because the function it is tracing, tcp_set_ca_state(),
has "set" in the name. :-)

Would it work to use something like:
  TRACE_EVENT(tcp_ca_event,

thanks,
neal