diff mbox series

[net-next,2/2] tcp: add more warn of socket in tcp_send_loss_probe()

Message ID 20241020145029.27725-3-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 fail Errors and warnings before: 12 this patch: 20
netdev/build_tools success Errors and warnings before: 0 (+1) this patch: 0 (+1)
netdev/cc_maintainers success CCed 5 of 5 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 fail Errors and warnings before: 909 this patch: 917
netdev/checkpatch warning WARNING: break quoted strings at a space character 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: 3 this patch: 3
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>

Add two fields to print in the helper which here covers tcp_send_loss_probe().

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     | 5 ++++-
 net/ipv4/tcp_output.c | 4 +---
 2 files changed, 5 insertions(+), 4 deletions(-)

Comments

Jason Xing Oct. 21, 2024, 2:37 a.m. UTC | #1 
On Sun, Oct 20, 2024 at 10:50 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> From: Jason Xing <kernelxing@tencent.com>
>
> Add two fields to print in the helper which here covers tcp_send_loss_probe().
>
> 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     | 5 ++++-
>  net/ipv4/tcp_output.c | 4 +---
>  2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index cac7bbff61ce..68eb03758950 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -2434,14 +2434,17 @@ static inline void tcp_warn_once(const struct sock *sk, bool cond, const char *s
>  {
>         WARN_ONCE(cond,
>                   "%s"
> +                 "cwnd:%u "
>                   "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",
> +                 "mss:%u advmss:%u mss_cache:%u pmtu:%u\n",
>                   str,
> +                 tcp_snd_cwnd(tcp_sk(sk)),
>                   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_current_mss((struct sock *)sk),
>                   tcp_sk(sk)->advmss, tcp_sk(sk)->mss_cache,
>                   inet_csk(sk)->icsk_pmtu_cookie);
>  }
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 054244ce5117..295bc0741772 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -2954,9 +2954,7 @@ void tcp_send_loss_probe(struct sock *sk)
>         }
>         skb = skb_rb_last(&sk->tcp_rtx_queue);
>         if (unlikely(!skb)) {
> -               WARN_ONCE(tp->packets_out,
> -                         "invalid inflight: %u state %u cwnd %u mss %d\n",
> -                         tp->packets_out, sk->sk_state, tcp_snd_cwnd(tp), mss);
> +               tcp_warn_once(sk, tp->packets_out, NULL);

Sorry, I noticed the warning:
In function ‘tcp_warn_once’,
    inlined from ‘tcp_send_loss_probe’ at ../net/ipv4/tcp_output.c:2957:3:
../include/net/tcp.h:2436:19: warning: ‘%s’ directive argument is null
[-Wformat-overflow=]
 2436 |                   "%s"
      |                   ^~~~

I think It should be:
tcp_warn_once(sk, tp->packets_out, "");

Will handle this soon.
kernel test robot Oct. 21, 2024, 5:52 a.m. UTC | #2 
Hi Jason,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Jason-Xing/tcp-add-a-common-helper-to-debug-the-underlying-issue/20241020-225356
base:   net-next/main
patch link:    https://lore.kernel.org/r/20241020145029.27725-3-kerneljasonxing%40gmail.com
patch subject: [PATCH net-next 2/2] tcp: add more warn of socket in tcp_send_loss_probe()
config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20241021/202410211313.7YBoFLNS-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241021/202410211313.7YBoFLNS-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410211313.7YBoFLNS-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from arch/x86/include/asm/bug.h:99,
                    from include/linux/bug.h:5,
                    from include/linux/fortify-string.h:6,
                    from include/linux/string.h:390,
                    from include/linux/bitmap.h:13,
                    from include/linux/cpumask.h:12,
                    from arch/x86/include/asm/cpumask.h:5,
                    from arch/x86/include/asm/msr.h:11,
                    from arch/x86/include/asm/tsc.h:10,
                    from arch/x86/include/asm/timex.h:6,
                    from include/linux/timex.h:67,
                    from include/linux/time32.h:13,
                    from include/linux/time.h:60,
                    from include/linux/skbuff.h:15,
                    from include/linux/tcp.h:17,
                    from include/net/tcp.h:20,
                    from net/ipv4/tcp_output.c:40:
   In function 'tcp_warn_once',
       inlined from 'tcp_warn_once' at include/net/tcp.h:2433:20,
       inlined from 'tcp_send_loss_probe' at net/ipv4/tcp_output.c:2957:3:
>> include/net/tcp.h:2436:19: warning: '%s' directive argument is null [-Wformat-overflow=]
    2436 |                   "%s"
         |                   ^~~~
   include/asm-generic/bug.h:106:31: note: in definition of macro '__WARN_printf'
     106 |                 __warn_printk(arg);                                     \
         |                               ^~~
   include/linux/once_lite.h:31:25: note: in expansion of macro 'WARN'
      31 |                         func(__VA_ARGS__);                              \
         |                         ^~~~
   include/asm-generic/bug.h:152:9: note: in expansion of macro 'DO_ONCE_LITE_IF'
     152 |         DO_ONCE_LITE_IF(condition, WARN, 1, format)
         |         ^~~~~~~~~~~~~~~
   include/net/tcp.h:2435:9: note: in expansion of macro 'WARN_ONCE'
    2435 |         WARN_ONCE(cond,
         |         ^~~~~~~~~
   include/net/tcp.h: In function 'tcp_send_loss_probe':
   include/net/tcp.h:2436:20: note: format string is defined here
    2436 |                   "%s"
         |                    ^~


vim +2436 include/net/tcp.h

1a91bb7c3ebf95 Mubashir Adnan Qureshi 2022-10-26  2432  
4fe1493c15028c Jason Xing             2024-10-20  2433  static inline void tcp_warn_once(const struct sock *sk, bool cond, const char *str)
4fe1493c15028c Jason Xing             2024-10-20  2434  {
4fe1493c15028c Jason Xing             2024-10-20  2435  	WARN_ONCE(cond,
4fe1493c15028c Jason Xing             2024-10-20 @2436  		  "%s"
d21098f05ea727 Jason Xing             2024-10-20  2437  		  "cwnd:%u "
4fe1493c15028c Jason Xing             2024-10-20  2438  		  "out:%u sacked:%u lost:%u retrans:%u "
4fe1493c15028c Jason Xing             2024-10-20  2439  		  "tlp_high_seq:%u sk_state:%u ca_state:%u "
d21098f05ea727 Jason Xing             2024-10-20  2440  		  "mss:%u advmss:%u mss_cache:%u pmtu:%u\n",
4fe1493c15028c Jason Xing             2024-10-20  2441  		  str,
d21098f05ea727 Jason Xing             2024-10-20  2442  		  tcp_snd_cwnd(tcp_sk(sk)),
4fe1493c15028c Jason Xing             2024-10-20  2443  		  tcp_sk(sk)->packets_out, tcp_sk(sk)->sacked_out,
4fe1493c15028c Jason Xing             2024-10-20  2444  		  tcp_sk(sk)->lost_out, tcp_sk(sk)->retrans_out,
4fe1493c15028c Jason Xing             2024-10-20  2445  		  tcp_sk(sk)->tlp_high_seq, sk->sk_state,
4fe1493c15028c Jason Xing             2024-10-20  2446  		  inet_csk(sk)->icsk_ca_state,
d21098f05ea727 Jason Xing             2024-10-20  2447  		  tcp_current_mss((struct sock *)sk),
4fe1493c15028c Jason Xing             2024-10-20  2448  		  tcp_sk(sk)->advmss, tcp_sk(sk)->mss_cache,
4fe1493c15028c Jason Xing             2024-10-20  2449  		  inet_csk(sk)->icsk_pmtu_cookie);
4fe1493c15028c Jason Xing             2024-10-20  2450  }
4fe1493c15028c Jason Xing             2024-10-20  2451
diff mbox series

Patch

diff --git a/include/net/tcp.h b/include/net/tcp.h
index cac7bbff61ce..68eb03758950 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -2434,14 +2434,17 @@  static inline void tcp_warn_once(const struct sock *sk, bool cond, const char *s
 {
 	WARN_ONCE(cond,
 		  "%s"
+		  "cwnd:%u "
 		  "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",
+		  "mss:%u advmss:%u mss_cache:%u pmtu:%u\n",
 		  str,
+		  tcp_snd_cwnd(tcp_sk(sk)),
 		  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_current_mss((struct sock *)sk),
 		  tcp_sk(sk)->advmss, tcp_sk(sk)->mss_cache,
 		  inet_csk(sk)->icsk_pmtu_cookie);
 }
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 054244ce5117..295bc0741772 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -2954,9 +2954,7 @@  void tcp_send_loss_probe(struct sock *sk)
 	}
 	skb = skb_rb_last(&sk->tcp_rtx_queue);
 	if (unlikely(!skb)) {
-		WARN_ONCE(tp->packets_out,
-			  "invalid inflight: %u state %u cwnd %u mss %d\n",
-			  tp->packets_out, sk->sk_state, tcp_snd_cwnd(tp), mss);
+		tcp_warn_once(sk, tp->packets_out, NULL);
 		smp_store_release(&inet_csk(sk)->icsk_pending, 0);
 		return;
 	}