diff mbox series

[net-next,1/2] tcp: add a common helper to debug the underlying issue

Message ID 20241020145029.27725-2-kerneljasonxing@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series tcp: add tcp_warn_once() common helper | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 12 this patch: 12
netdev/build_tools success Errors and warnings before: 0 (+1) this patch: 0 (+1)
netdev/cc_maintainers success CCed 4 of 4 maintainers
netdev/build_clang success Errors and warnings before: 15 this patch: 15
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 909 this patch: 909
netdev/checkpatch warning WARNING: break quoted strings at a space character WARNING: line length of 83 exceeds 80 columns WARNING: quoted string split across lines
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 2 this patch: 2
netdev/source_inline success Was 0 now: 0

Commit Message

Jason Xing Oct. 20, 2024, 2:50 p.m. UTC 
From: Jason Xing <kernelxing@tencent.com>

Following the commit c8770db2d544 ("tcp: check skb is non-NULL
in tcp_rto_delta_us()"), we decided to add a helper so that it's
easier to get verbose warning on either cases.

Link: https://lore.kernel.org/all/5632e043-bdba-4d75-bc7e-bf58014492fd@redhat.com/
Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Jason Xing <kernelxing@tencent.com>
Cc: Neal Cardwell <ncardwell@google.com>
---
 include/net/tcp.h | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

Comments

David Ahern Oct. 20, 2024, 4:18 p.m. UTC | #1 
On 10/20/24 8:50 AM, Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.com>
> 
> Following the commit c8770db2d544 ("tcp: check skb is non-NULL
> in tcp_rto_delta_us()"), we decided to add a helper so that it's
> easier to get verbose warning on either cases.
> 
> Link: https://lore.kernel.org/all/5632e043-bdba-4d75-bc7e-bf58014492fd@redhat.com/
> Suggested-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> Cc: Neal Cardwell <ncardwell@google.com>
> ---
>  include/net/tcp.h | 28 +++++++++++++++++-----------
>  1 file changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 739a9fb83d0c..cac7bbff61ce 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -2430,6 +2430,22 @@ void tcp_plb_update_state(const struct sock *sk, struct tcp_plb_state *plb,
>  void tcp_plb_check_rehash(struct sock *sk, struct tcp_plb_state *plb);
>  void tcp_plb_update_state_upon_rto(struct sock *sk, struct tcp_plb_state *plb);
>  
> +static inline void tcp_warn_once(const struct sock *sk, bool cond, const char *str)
> +{
> +	WARN_ONCE(cond,
> +		  "%s"
> +		  "out:%u sacked:%u lost:%u retrans:%u "
> +		  "tlp_high_seq:%u sk_state:%u ca_state:%u "
> +		  "advmss:%u mss_cache:%u pmtu:%u\n",

format lines should not be split across lines. Yes, I realize the
existing code is, but since you are moving it and making changes to it
this can be fixed as well.
Jason Xing Oct. 20, 2024, 4:34 p.m. UTC | #2 
Hello David,

On Mon, Oct 21, 2024 at 12:18 AM David Ahern <dsahern@kernel.org> wrote:
>
> On 10/20/24 8:50 AM, Jason Xing wrote:
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > Following the commit c8770db2d544 ("tcp: check skb is non-NULL
> > in tcp_rto_delta_us()"), we decided to add a helper so that it's
> > easier to get verbose warning on either cases.
> >
> > Link: https://lore.kernel.org/all/5632e043-bdba-4d75-bc7e-bf58014492fd@redhat.com/
> > Suggested-by: Paolo Abeni <pabeni@redhat.com>
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > Cc: Neal Cardwell <ncardwell@google.com>
> > ---
> >  include/net/tcp.h | 28 +++++++++++++++++-----------
> >  1 file changed, 17 insertions(+), 11 deletions(-)
> >
> > diff --git a/include/net/tcp.h b/include/net/tcp.h
> > index 739a9fb83d0c..cac7bbff61ce 100644
> > --- a/include/net/tcp.h
> > +++ b/include/net/tcp.h
> > @@ -2430,6 +2430,22 @@ void tcp_plb_update_state(const struct sock *sk, struct tcp_plb_state *plb,
> >  void tcp_plb_check_rehash(struct sock *sk, struct tcp_plb_state *plb);
> >  void tcp_plb_update_state_upon_rto(struct sock *sk, struct tcp_plb_state *plb);
> >
> > +static inline void tcp_warn_once(const struct sock *sk, bool cond, const char *str)
> > +{
> > +     WARN_ONCE(cond,
> > +               "%s"
> > +               "out:%u sacked:%u lost:%u retrans:%u "
> > +               "tlp_high_seq:%u sk_state:%u ca_state:%u "
> > +               "advmss:%u mss_cache:%u pmtu:%u\n",
>
> format lines should not be split across lines. Yes, I realize the
> existing code is, but since you are moving it and making changes to it
> this can be fixed as well.

Thanks for reminding me. Actually before submitting this series, I
noticed this warning. I was thinking it looks a little ugly if we are
going to add more information in this function?

This function could be like this:
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 739a9fb83d0c..8b8d94bb1746 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -2430,6 +2430,19 @@ void tcp_plb_update_state(const struct sock
*sk, struct tcp_plb_state *plb,
 void tcp_plb_check_rehash(struct sock *sk, struct tcp_plb_state *plb);
 void tcp_plb_update_state_upon_rto(struct sock *sk, struct tcp_plb_state *plb);

+static inline void tcp_warn_once(const struct sock *sk, bool cond,
const char *str)
+{
+       WARN_ONCE(cond,
+                 "%sout:%u sacked:%u lost:%u retrans:%u
tlp_high_seq:%u sk_state:%u ca_state:%u advmss:%u mss_cache:%u
pmtu:%u\n",
+                 str,
+                 tcp_sk(sk)->packets_out, tcp_sk(sk)->sacked_out,
+                 tcp_sk(sk)->lost_out, tcp_sk(sk)->retrans_out,
+                 tcp_sk(sk)->tlp_high_seq, sk->sk_state,
+                 inet_csk(sk)->icsk_ca_state,
+                 tcp_sk(sk)->advmss, tcp_sk(sk)->mss_cache,
+                 inet_csk(sk)->icsk_pmtu_cookie);
+}
+
That quoted line seems a little long... Do you think this format is
fine with you? If so, I will adjust it in the next version.

Thanks,
Jason
David Ahern Oct. 20, 2024, 4:40 p.m. UTC | #3 
On 10/20/24 10:34 AM, Jason Xing wrote:
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 739a9fb83d0c..8b8d94bb1746 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -2430,6 +2430,19 @@ void tcp_plb_update_state(const struct sock
> *sk, struct tcp_plb_state *plb,
>  void tcp_plb_check_rehash(struct sock *sk, struct tcp_plb_state *plb);
>  void tcp_plb_update_state_upon_rto(struct sock *sk, struct tcp_plb_state *plb);
> 
> +static inline void tcp_warn_once(const struct sock *sk, bool cond,
> const char *str)
> +{
> +       WARN_ONCE(cond,
> +                 "%sout:%u sacked:%u lost:%u retrans:%u
> tlp_high_seq:%u sk_state:%u ca_state:%u advmss:%u mss_cache:%u
> pmtu:%u\n",
> +                 str,
> +                 tcp_sk(sk)->packets_out, tcp_sk(sk)->sacked_out,
> +                 tcp_sk(sk)->lost_out, tcp_sk(sk)->retrans_out,
> +                 tcp_sk(sk)->tlp_high_seq, sk->sk_state,
> +                 inet_csk(sk)->icsk_ca_state,
> +                 tcp_sk(sk)->advmss, tcp_sk(sk)->mss_cache,
> +                 inet_csk(sk)->icsk_pmtu_cookie);
> +}
> +
> That quoted line seems a little long... Do you think this format is
> fine with you? If so, I will adjust it in the next version.
> 

Format strings are an exception to the 80-column rule. Strings should
not be split to allow for grep to find a match, for example.
Jason Xing Oct. 20, 2024, 4:50 p.m. UTC | #4 
On Mon, Oct 21, 2024 at 12:41 AM David Ahern <dsahern@kernel.org> wrote:
>
> On 10/20/24 10:34 AM, Jason Xing wrote:
> > diff --git a/include/net/tcp.h b/include/net/tcp.h
> > index 739a9fb83d0c..8b8d94bb1746 100644
> > --- a/include/net/tcp.h
> > +++ b/include/net/tcp.h
> > @@ -2430,6 +2430,19 @@ void tcp_plb_update_state(const struct sock
> > *sk, struct tcp_plb_state *plb,
> >  void tcp_plb_check_rehash(struct sock *sk, struct tcp_plb_state *plb);
> >  void tcp_plb_update_state_upon_rto(struct sock *sk, struct tcp_plb_state *plb);
> >
> > +static inline void tcp_warn_once(const struct sock *sk, bool cond,
> > const char *str)
> > +{
> > +       WARN_ONCE(cond,
> > +                 "%sout:%u sacked:%u lost:%u retrans:%u
> > tlp_high_seq:%u sk_state:%u ca_state:%u advmss:%u mss_cache:%u
> > pmtu:%u\n",
> > +                 str,
> > +                 tcp_sk(sk)->packets_out, tcp_sk(sk)->sacked_out,
> > +                 tcp_sk(sk)->lost_out, tcp_sk(sk)->retrans_out,
> > +                 tcp_sk(sk)->tlp_high_seq, sk->sk_state,
> > +                 inet_csk(sk)->icsk_ca_state,
> > +                 tcp_sk(sk)->advmss, tcp_sk(sk)->mss_cache,
> > +                 inet_csk(sk)->icsk_pmtu_cookie);
> > +}
> > +
> > That quoted line seems a little long... Do you think this format is
> > fine with you? If so, I will adjust it in the next version.
> >
>
> Format strings are an exception to the 80-column rule. Strings should
> not be split to allow for grep to find a match, for example.

Thanks. I got it. I will use the above code snippet which can pass the
checkpatch script.
diff mbox series

Patch

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 739a9fb83d0c..cac7bbff61ce 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -2430,6 +2430,22 @@  void tcp_plb_update_state(const struct sock *sk, struct tcp_plb_state *plb,
 void tcp_plb_check_rehash(struct sock *sk, struct tcp_plb_state *plb);
 void tcp_plb_update_state_upon_rto(struct sock *sk, struct tcp_plb_state *plb);
 
+static inline void tcp_warn_once(const struct sock *sk, bool cond, const char *str)
+{
+	WARN_ONCE(cond,
+		  "%s"
+		  "out:%u sacked:%u lost:%u retrans:%u "
+		  "tlp_high_seq:%u sk_state:%u ca_state:%u "
+		  "advmss:%u mss_cache:%u pmtu:%u\n",
+		  str,
+		  tcp_sk(sk)->packets_out, tcp_sk(sk)->sacked_out,
+		  tcp_sk(sk)->lost_out, tcp_sk(sk)->retrans_out,
+		  tcp_sk(sk)->tlp_high_seq, sk->sk_state,
+		  inet_csk(sk)->icsk_ca_state,
+		  tcp_sk(sk)->advmss, tcp_sk(sk)->mss_cache,
+		  inet_csk(sk)->icsk_pmtu_cookie);
+}
+
 /* At how many usecs into the future should the RTO fire? */
 static inline s64 tcp_rto_delta_us(const struct sock *sk)
 {
@@ -2441,17 +2457,7 @@  static inline s64 tcp_rto_delta_us(const struct sock *sk)
 
 		return rto_time_stamp_us - tcp_sk(sk)->tcp_mstamp;
 	} else {
-		WARN_ONCE(1,
-			"rtx queue empty: "
-			"out:%u sacked:%u lost:%u retrans:%u "
-			"tlp_high_seq:%u sk_state:%u ca_state:%u "
-			"advmss:%u mss_cache:%u pmtu:%u\n",
-			tcp_sk(sk)->packets_out, tcp_sk(sk)->sacked_out,
-			tcp_sk(sk)->lost_out, tcp_sk(sk)->retrans_out,
-			tcp_sk(sk)->tlp_high_seq, sk->sk_state,
-			inet_csk(sk)->icsk_ca_state,
-			tcp_sk(sk)->advmss, tcp_sk(sk)->mss_cache,
-			inet_csk(sk)->icsk_pmtu_cookie);
+		tcp_warn_once(sk, 1, "rtx queue empty: ");
 		return jiffies_to_usecs(rto);
 	}