| Message ID | 20230806075216.13378-1-me@manjusaka.me (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [RFC] tcp event: add new tcp:tcp_cwnd_restart event | expand |
On Sun, Aug 6, 2023 at 9:52 AM Manjusaka <me@manjusaka.me> wrote: > > The tcp_cwnd_restart function would be called if the user has enabled the > tcp_slow_start_after_idle configuration and would be triggered when the > connection is idle (like LONG RTO etc.). I think it would be great to > add a new trace event named 'tcp:tcp_cwnd_reset'; it would help people > to monitor the TCP state in a complicated network environment(like > overlay/underlay SDN in Kubernetes, etc) > > Signed-off-by: Manjusaka <me@manjusaka.me> > --- > include/trace/events/tcp.h | 7 +++++++ > net/ipv4/tcp_output.c | 1 + > 2 files changed, 8 insertions(+) > > diff --git a/include/trace/events/tcp.h b/include/trace/events/tcp.h > index bf06db8d2046..fa44191cc609 100644 > --- a/include/trace/events/tcp.h > +++ b/include/trace/events/tcp.h > @@ -187,6 +187,13 @@ DEFINE_EVENT(tcp_event_sk, tcp_rcv_space_adjust, > TP_ARGS(sk) > ); > > +DEFINE_EVENT(tcp_event_sk, tcp_cwnd_restart, > + > + TP_PROTO(struct sock *sk), > + > + TP_ARGS(sk) > +); > + > TRACE_EVENT(tcp_retransmit_synack, > > TP_PROTO(const struct sock *sk, const struct request_sock *req), > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c > index 51d8638d4b4c..e902fa74303d 100644 > --- a/net/ipv4/tcp_output.c > +++ b/net/ipv4/tcp_output.c > @@ -141,6 +141,7 @@ static __u16 tcp_advertise_mss(struct sock *sk) > */ > void tcp_cwnd_restart(struct sock *sk, s32 delta) > { > + trace_tcp_cwnd_restart(sk); Do not include code before variable declarations. > struct tcp_sock *tp = tcp_sk(sk); > u32 restart_cwnd = tcp_init_cwnd(tp, __sk_dst_get(sk)); > u32 cwnd = tcp_snd_cwnd(tp); > -- > 2.34.1 > I would rather add a trace in tcp_ca_event(), this would be more generic ?
> Do not include code before variable declarations. Sorry about that. I will update the code later. > I would rather add a trace in tcp_ca_event(), this would be more generic ? https://elixir.bootlin.com/linux/latest/source/net/ipv4/tcp_cong.c#L41 I think maybe we already have the tcp_ca_event but named tcp_cong_state_set? On Mon, Aug 7, 2023, at 8:34 PM, Eric Dumazet wrote: > On Sun, Aug 6, 2023 at 9:52 AM Manjusaka <me@manjusaka.me> wrote: > > > > The tcp_cwnd_restart function would be called if the user has enabled the > > tcp_slow_start_after_idle configuration and would be triggered when the > > connection is idle (like LONG RTO etc.). I think it would be great to > > add a new trace event named 'tcp:tcp_cwnd_reset'; it would help people > > to monitor the TCP state in a complicated network environment(like > > overlay/underlay SDN in Kubernetes, etc) > > > > Signed-off-by: Manjusaka <me@manjusaka.me> > > --- > > include/trace/events/tcp.h | 7 +++++++ > > net/ipv4/tcp_output.c | 1 + > > 2 files changed, 8 insertions(+) > > > > diff --git a/include/trace/events/tcp.h b/include/trace/events/tcp.h > > index bf06db8d2046..fa44191cc609 100644 > > --- a/include/trace/events/tcp.h > > +++ b/include/trace/events/tcp.h > > @@ -187,6 +187,13 @@ DEFINE_EVENT(tcp_event_sk, tcp_rcv_space_adjust, > > TP_ARGS(sk) > > ); > > > > +DEFINE_EVENT(tcp_event_sk, tcp_cwnd_restart, > > + > > + TP_PROTO(struct sock *sk), > > + > > + TP_ARGS(sk) > > +); > > + > > TRACE_EVENT(tcp_retransmit_synack, > > > > TP_PROTO(const struct sock *sk, const struct request_sock *req), > > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c > > index 51d8638d4b4c..e902fa74303d 100644 > > --- a/net/ipv4/tcp_output.c > > +++ b/net/ipv4/tcp_output.c > > @@ -141,6 +141,7 @@ static __u16 tcp_advertise_mss(struct sock *sk) > > */ > > void tcp_cwnd_restart(struct sock *sk, s32 delta) > > { > > + trace_tcp_cwnd_restart(sk); > > Do not include code before variable declarations. > > > struct tcp_sock *tp = tcp_sk(sk); > > u32 restart_cwnd = tcp_init_cwnd(tp, __sk_dst_get(sk)); > > u32 cwnd = tcp_snd_cwnd(tp); > > -- > > 2.34.1 > > > > I would rather add a trace in tcp_ca_event(), this would be more generic ? >
On Mon, Aug 7, 2023 at 2:49 PM Manjusaka <me@manjusaka.me> wrote: > > > Do not include code before variable declarations. > Sorry about that. I will update the code later. > > > I would rather add a trace in tcp_ca_event(), this would be more generic ? > > https://elixir.bootlin.com/linux/latest/source/net/ipv4/tcp_cong.c#L41 > > I think maybe we already have the tcp_ca_event but named tcp_cong_state_set? I am speaking of tcp_ca_event()... For instance, tcp_cwnd_restart() calls tcp_ca_event(sk, CA_EVENT_CWND_RESTART); tcp_set_ca_state() can only set icsk_ca_state to one value from enum tcp_ca_state: TCP_CA_Open, TCP_CA_Disorder, TCP_CA_CWR, TCP_CA_Recovery, TCP_CA_Loss enum tcp_ca_event has instead: CA_EVENT_TX_START, CA_EVENT_CWND_RESTART, CA_EVENT_COMPLETE_CWR, CA_EVENT_LOSS, CA_EVENT_ECN_NO_CE, CA_EVENT_ECN_IS_CE
Got you means! LGTM I will make a patch later and make a try On Mon, Aug 7, 2023, at 9:25 PM, Eric Dumazet wrote: > On Mon, Aug 7, 2023 at 2:49 PM Manjusaka <me@manjusaka.me> wrote: > > > > > Do not include code before variable declarations. > > Sorry about that. I will update the code later. > > > > > I would rather add a trace in tcp_ca_event(), this would be more generic ? > > > > https://elixir.bootlin.com/linux/latest/source/net/ipv4/tcp_cong.c#L41 > > > > I think maybe we already have the tcp_ca_event but named tcp_cong_state_set? > > I am speaking of tcp_ca_event()... > > For instance, tcp_cwnd_restart() calls tcp_ca_event(sk, CA_EVENT_CWND_RESTART); > > tcp_set_ca_state() can only set icsk_ca_state to one value from enum > tcp_ca_state: > TCP_CA_Open, TCP_CA_Disorder, TCP_CA_CWR, TCP_CA_Recovery, TCP_CA_Loss > > enum tcp_ca_event has instead: > CA_EVENT_TX_START, CA_EVENT_CWND_RESTART, CA_EVENT_COMPLETE_CWR, > CA_EVENT_LOSS, CA_EVENT_ECN_NO_CE, CA_EVENT_ECN_IS_CE >
diff --git a/include/trace/events/tcp.h b/include/trace/events/tcp.h index bf06db8d2046..fa44191cc609 100644 --- a/include/trace/events/tcp.h +++ b/include/trace/events/tcp.h @@ -187,6 +187,13 @@ DEFINE_EVENT(tcp_event_sk, tcp_rcv_space_adjust, TP_ARGS(sk) ); +DEFINE_EVENT(tcp_event_sk, tcp_cwnd_restart, + + TP_PROTO(struct sock *sk), + + TP_ARGS(sk) +); + TRACE_EVENT(tcp_retransmit_synack, TP_PROTO(const struct sock *sk, const struct request_sock *req), diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c index 51d8638d4b4c..e902fa74303d 100644 --- a/net/ipv4/tcp_output.c +++ b/net/ipv4/tcp_output.c @@ -141,6 +141,7 @@ static __u16 tcp_advertise_mss(struct sock *sk) */ void tcp_cwnd_restart(struct sock *sk, s32 delta) { + trace_tcp_cwnd_restart(sk); struct tcp_sock *tp = tcp_sk(sk); u32 restart_cwnd = tcp_init_cwnd(tp, __sk_dst_get(sk)); u32 cwnd = tcp_snd_cwnd(tp);
The tcp_cwnd_restart function would be called if the user has enabled the tcp_slow_start_after_idle configuration and would be triggered when the connection is idle (like LONG RTO etc.). I think it would be great to add a new trace event named 'tcp:tcp_cwnd_reset'; it would help people to monitor the TCP state in a complicated network environment(like overlay/underlay SDN in Kubernetes, etc) Signed-off-by: Manjusaka <me@manjusaka.me> --- include/trace/events/tcp.h | 7 +++++++ net/ipv4/tcp_output.c | 1 + 2 files changed, 8 insertions(+)