| Message ID | 20221208154656.60623-1-leitao@debian.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,net-next] tcp: socket-specific version of WARN_ON_ONCE() | expand |
From: Breno Leitao <leitao@debian.org> Date: Thu, 8 Dec 2022 07:46:56 -0800 > There are cases where we need relevant information about the socket > during a warning, so, it could help us to find bugs that happens and do > not have an easy repro. > > This patch creates a TCP-socket specific version of WARN_ON_ONCE(), which > dumps revelant information about the TCP socket when it hits rare > warnings, which is super useful for debugging purposes. > > Hooking this warning tcp_snd_cwnd_set() for now, but, the intent is to > convert more TCP warnings to this helper later. > > Signed-off-by: Breno Leitao <leitao@debian.org> Acked-by: Kuniyuki Iwashima <kuniyu@amazon.com> Looks good to me, thank you. A minor comment below. > --- > include/net/tcp.h | 3 ++- > include/net/tcp_debug.h | 10 ++++++++++ > net/ipv4/tcp.c | 30 ++++++++++++++++++++++++++++++ > 3 files changed, 42 insertions(+), 1 deletion(-) > create mode 100644 include/net/tcp_debug.h > > diff --git a/include/net/tcp.h b/include/net/tcp.h > index 14d45661a84d..e490af8e6fdc 100644 > --- a/include/net/tcp.h > +++ b/include/net/tcp.h > @@ -40,6 +40,7 @@ > #include <net/inet_ecn.h> > #include <net/dst.h> > #include <net/mptcp.h> > +#include <net/tcp_debug.h> > > #include <linux/seq_file.h> > #include <linux/memcontrol.h> > @@ -1229,7 +1230,7 @@ static inline u32 tcp_snd_cwnd(const struct tcp_sock *tp) > > static inline void tcp_snd_cwnd_set(struct tcp_sock *tp, u32 val) > { > - WARN_ON_ONCE((int)val <= 0); > + TCP_SOCK_WARN_ON_ONCE(tp, (int)val <= 0); > tp->snd_cwnd = val; > } > > diff --git a/include/net/tcp_debug.h b/include/net/tcp_debug.h > new file mode 100644 > index 000000000000..50e96d87d335 > --- /dev/null > +++ b/include/net/tcp_debug.h > @@ -0,0 +1,10 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef _LINUX_TCP_DEBUG_H > +#define _LINUX_TCP_DEBUG_H > + > +void tcp_sock_warn(const struct tcp_sock *tp); > + > +#define TCP_SOCK_WARN_ON_ONCE(tcp_sock, condition) \ > + DO_ONCE_LITE_IF(condition, tcp_sock_warn, tcp_sock) > + > +#endif /* _LINUX_TCP_DEBUG_H */ > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c > index 54836a6b81d6..5985ba9c4231 100644 > --- a/net/ipv4/tcp.c > +++ b/net/ipv4/tcp.c > @@ -4705,6 +4705,36 @@ int tcp_abort(struct sock *sk, int err) > } > EXPORT_SYMBOL_GPL(tcp_abort); > > +void tcp_sock_warn(const struct tcp_sock *tp) > +{ > + const struct sock *sk = (const struct sock *)tp; > + struct inet_sock *inet = inet_sk(sk); > + struct inet_connection_sock *icsk = inet_csk(sk); Let's keep the reverse christmas tree order. $ cat -n Documentation/process/maintainer-netdev.rst | grep xmas -C 30 > + > + WARN_ON(1); > + > + pr_warn("Socket Info: family=%u state=%d ccname=%s cwnd=%u", > + sk->sk_family, sk->sk_state, icsk->icsk_ca_ops->name, > + tcp_snd_cwnd(tp)); > + > + switch (sk->sk_family) { > + case AF_INET: > + pr_warn("saddr=%pI4:%u daddr=%pI4:%u", &inet->inet_saddr, > + ntohs(inet->inet_sport), &inet->inet_daddr, > + ntohs(inet->inet_dport)); > + > + break; > +#if IS_ENABLED(CONFIG_IPV6) > + case AF_INET6: > + pr_warn("saddr=[%pI6]:%u daddr=[%pI6]:%u", &sk->sk_v6_rcv_saddr, > + ntohs(inet->inet_sport), &sk->sk_v6_daddr, > + ntohs(inet->inet_dport)); > + break; > +#endif > + } > +} > +EXPORT_SYMBOL_GPL(tcp_sock_warn); > + > extern struct tcp_congestion_ops tcp_reno; > > static __initdata unsigned long thash_entries; > -- > 2.30.2
Breno Leitao <leitao@debian.org> writes: > There are cases where we need relevant information about the socket > during a warning, so, it could help us to find bugs that happens and do > not have an easy repro. > > This patch creates a TCP-socket specific version of WARN_ON_ONCE(), which > dumps revelant information about the TCP socket when it hits rare > warnings, which is super useful for debugging purposes. > > Hooking this warning tcp_snd_cwnd_set() for now, but, the intent is to > convert more TCP warnings to this helper later. > > Signed-off-by: Breno Leitao <leitao@debian.org> > --- > include/net/tcp.h | 3 ++- > include/net/tcp_debug.h | 10 ++++++++++ > net/ipv4/tcp.c | 30 ++++++++++++++++++++++++++++++ > 3 files changed, 42 insertions(+), 1 deletion(-) > create mode 100644 include/net/tcp_debug.h > > diff --git a/include/net/tcp.h b/include/net/tcp.h > index 14d45661a84d..e490af8e6fdc 100644 > --- a/include/net/tcp.h > +++ b/include/net/tcp.h > @@ -40,6 +40,7 @@ > #include <net/inet_ecn.h> > #include <net/dst.h> > #include <net/mptcp.h> > +#include <net/tcp_debug.h> > > #include <linux/seq_file.h> > #include <linux/memcontrol.h> > @@ -1229,7 +1230,7 @@ static inline u32 tcp_snd_cwnd(const struct tcp_sock *tp) > > static inline void tcp_snd_cwnd_set(struct tcp_sock *tp, u32 val) > { > - WARN_ON_ONCE((int)val <= 0); > + TCP_SOCK_WARN_ON_ONCE(tp, (int)val <= 0); > tp->snd_cwnd = val; > } > > diff --git a/include/net/tcp_debug.h b/include/net/tcp_debug.h > new file mode 100644 > index 000000000000..50e96d87d335 > --- /dev/null > +++ b/include/net/tcp_debug.h > @@ -0,0 +1,10 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef _LINUX_TCP_DEBUG_H > +#define _LINUX_TCP_DEBUG_H > + > +void tcp_sock_warn(const struct tcp_sock *tp); > + > +#define TCP_SOCK_WARN_ON_ONCE(tcp_sock, condition) \ > + DO_ONCE_LITE_IF(condition, tcp_sock_warn, tcp_sock) > + > +#endif /* _LINUX_TCP_DEBUG_H */ > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c > index 54836a6b81d6..5985ba9c4231 100644 > --- a/net/ipv4/tcp.c > +++ b/net/ipv4/tcp.c > @@ -4705,6 +4705,36 @@ int tcp_abort(struct sock *sk, int err) > } > EXPORT_SYMBOL_GPL(tcp_abort); > > +void tcp_sock_warn(const struct tcp_sock *tp) > +{ > + const struct sock *sk = (const struct sock *)tp; > + struct inet_sock *inet = inet_sk(sk); > + struct inet_connection_sock *icsk = inet_csk(sk); > + > + WARN_ON(1); Never looked into the details of WARN_ON, but shouldn't that come at the end of the function? If one has kernel.panic_on_warn=1, the kernel would already panic in WARN_ON, and the lines below wouldn't be printed? > + > + pr_warn("Socket Info: family=%u state=%d ccname=%s cwnd=%u", > + sk->sk_family, sk->sk_state, icsk->icsk_ca_ops->name, > + tcp_snd_cwnd(tp)); > + > + switch (sk->sk_family) { > + case AF_INET: > + pr_warn("saddr=%pI4:%u daddr=%pI4:%u", &inet->inet_saddr, > + ntohs(inet->inet_sport), &inet->inet_daddr, > + ntohs(inet->inet_dport)); > + > + break; > +#if IS_ENABLED(CONFIG_IPV6) > + case AF_INET6: > + pr_warn("saddr=[%pI6]:%u daddr=[%pI6]:%u", &sk->sk_v6_rcv_saddr, > + ntohs(inet->inet_sport), &sk->sk_v6_daddr, > + ntohs(inet->inet_dport)); > + break; > +#endif > + } > +} > +EXPORT_SYMBOL_GPL(tcp_sock_warn); > + > extern struct tcp_congestion_ops tcp_reno; > > static __initdata unsigned long thash_entries;
diff --git a/include/net/tcp.h b/include/net/tcp.h index 14d45661a84d..e490af8e6fdc 100644 --- a/include/net/tcp.h +++ b/include/net/tcp.h @@ -40,6 +40,7 @@ #include <net/inet_ecn.h> #include <net/dst.h> #include <net/mptcp.h> +#include <net/tcp_debug.h> #include <linux/seq_file.h> #include <linux/memcontrol.h> @@ -1229,7 +1230,7 @@ static inline u32 tcp_snd_cwnd(const struct tcp_sock *tp) static inline void tcp_snd_cwnd_set(struct tcp_sock *tp, u32 val) { - WARN_ON_ONCE((int)val <= 0); + TCP_SOCK_WARN_ON_ONCE(tp, (int)val <= 0); tp->snd_cwnd = val; } diff --git a/include/net/tcp_debug.h b/include/net/tcp_debug.h new file mode 100644 index 000000000000..50e96d87d335 --- /dev/null +++ b/include/net/tcp_debug.h @@ -0,0 +1,10 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _LINUX_TCP_DEBUG_H +#define _LINUX_TCP_DEBUG_H + +void tcp_sock_warn(const struct tcp_sock *tp); + +#define TCP_SOCK_WARN_ON_ONCE(tcp_sock, condition) \ + DO_ONCE_LITE_IF(condition, tcp_sock_warn, tcp_sock) + +#endif /* _LINUX_TCP_DEBUG_H */ diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index 54836a6b81d6..5985ba9c4231 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -4705,6 +4705,36 @@ int tcp_abort(struct sock *sk, int err) } EXPORT_SYMBOL_GPL(tcp_abort); +void tcp_sock_warn(const struct tcp_sock *tp) +{ + const struct sock *sk = (const struct sock *)tp; + struct inet_sock *inet = inet_sk(sk); + struct inet_connection_sock *icsk = inet_csk(sk); + + WARN_ON(1); + + pr_warn("Socket Info: family=%u state=%d ccname=%s cwnd=%u", + sk->sk_family, sk->sk_state, icsk->icsk_ca_ops->name, + tcp_snd_cwnd(tp)); + + switch (sk->sk_family) { + case AF_INET: + pr_warn("saddr=%pI4:%u daddr=%pI4:%u", &inet->inet_saddr, + ntohs(inet->inet_sport), &inet->inet_daddr, + ntohs(inet->inet_dport)); + + break; +#if IS_ENABLED(CONFIG_IPV6) + case AF_INET6: + pr_warn("saddr=[%pI6]:%u daddr=[%pI6]:%u", &sk->sk_v6_rcv_saddr, + ntohs(inet->inet_sport), &sk->sk_v6_daddr, + ntohs(inet->inet_dport)); + break; +#endif + } +} +EXPORT_SYMBOL_GPL(tcp_sock_warn); + extern struct tcp_congestion_ops tcp_reno; static __initdata unsigned long thash_entries;
There are cases where we need relevant information about the socket during a warning, so, it could help us to find bugs that happens and do not have an easy repro. This patch creates a TCP-socket specific version of WARN_ON_ONCE(), which dumps revelant information about the TCP socket when it hits rare warnings, which is super useful for debugging purposes. Hooking this warning tcp_snd_cwnd_set() for now, but, the intent is to convert more TCP warnings to this helper later. Signed-off-by: Breno Leitao <leitao@debian.org> --- include/net/tcp.h | 3 ++- include/net/tcp_debug.h | 10 ++++++++++ net/ipv4/tcp.c | 30 ++++++++++++++++++++++++++++++ 3 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 include/net/tcp_debug.h