Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1734465 > unrolled thread

[PATCH] tcp: avoid bogus warning in tcp_clean_rtx_queue

Back to article view | Back to linux.kernel

Contents

  [PATCH] tcp: avoid bogus warning in tcp_clean_rtx_queue Arnd Bergmann <arnd@arndb.de> - 2017-09-18 22:50 +0200
    Re: [PATCH] tcp: avoid bogus warning in tcp_clean_rtx_queue David Miller <davem@davemloft.net> - 2017-09-19 23:10 +0200
      Re: [PATCH] tcp: avoid bogus warning in tcp_clean_rtx_queue Arnd Bergmann <arnd@arndb.de> - 2017-09-19 23:40 +0200
        Re: [PATCH] tcp: avoid bogus warning in tcp_clean_rtx_queue David Miller <davem@davemloft.net> - 2017-09-20 00:10 +0200

#1734465 — [PATCH] tcp: avoid bogus warning in tcp_clean_rtx_queue

gcc-4.9 warns that it cannot trace the state of the 'last_ackt'
variable since the change to the TCP timestamping code, when
CONFIG_PROFILE_ANNOTATED_BRANCHES is set:

net/ipv4/tcp_input.c: In function 'tcp_clean_rtx_queue':
include/net/tcp.h:757:23: error: 'last_ackt' may be used uninitialized in this function [-Werror=maybe-uninitialized]

Other gcc versions, both older and newer do now show this
warning. Removing the 'likely' annotation makes it go away,
and has no effect on the object code without
CONFIG_PROFILE_ANNOTATED_BRANCHES, as tested with gcc-4.9
and gcc-7.1.1, so this seems to be a safe workaround.

Fixes: 9a568de4818d ("tcp: switch TCP TS option (RFC 7323) to 1ms clock")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 net/ipv4/tcp_input.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index c5d7656beeee..c52bc8e35d4d 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -3173,7 +3173,7 @@ static int tcp_clean_rtx_queue(struct sock *sk, int prior_fackets,
 	if (skb && (TCP_SKB_CB(skb)->sacked & TCPCB_SACKED_ACKED))
 		flag |= FLAG_SACK_RENEGING;
 
-	if (likely(first_ackt) && !(flag & FLAG_RETRANS_DATA_ACKED)) {
+	if (first_ackt && !(flag & FLAG_RETRANS_DATA_ACKED)) {
 		seq_rtt_us = tcp_stamp_us_delta(tp->tcp_mstamp, first_ackt);
 		ca_rtt_us = tcp_stamp_us_delta(tp->tcp_mstamp, last_ackt);
 	}
-- 
2.9.0

[toc] | [next] | [standalone]

#1735266

From: Arnd Bergmann <arnd@arndb.de>
Date: Mon, 18 Sep 2017 22:48:47 +0200

> gcc-4.9 warns that it cannot trace the state of the 'last_ackt'
> variable since the change to the TCP timestamping code, when
> CONFIG_PROFILE_ANNOTATED_BRANCHES is set:
> 
> net/ipv4/tcp_input.c: In function 'tcp_clean_rtx_queue':
> include/net/tcp.h:757:23: error: 'last_ackt' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> 
> Other gcc versions, both older and newer do now show this
> warning. Removing the 'likely' annotation makes it go away,
> and has no effect on the object code without
> CONFIG_PROFILE_ANNOTATED_BRANCHES, as tested with gcc-4.9
> and gcc-7.1.1, so this seems to be a safe workaround.
> 
> Fixes: 9a568de4818d ("tcp: switch TCP TS option (RFC 7323) to 1ms clock")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

This reaches the limits at which I am willing to work around compiler
stuff.

What cpu did you test the object code generation upon and does that
cpu have branch prediction hints in the target you are building for?

[toc] | [prev] | [next] | [standalone]

#1735274

On Tue, Sep 19, 2017 at 11:02 PM, David Miller <davem@davemloft.net> wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> Date: Mon, 18 Sep 2017 22:48:47 +0200
>
>> gcc-4.9 warns that it cannot trace the state of the 'last_ackt'
>> variable since the change to the TCP timestamping code, when
>> CONFIG_PROFILE_ANNOTATED_BRANCHES is set:
>>
>> net/ipv4/tcp_input.c: In function 'tcp_clean_rtx_queue':
>> include/net/tcp.h:757:23: error: 'last_ackt' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>
>> Other gcc versions, both older and newer do now show this
>> warning. Removing the 'likely' annotation makes it go away,
>> and has no effect on the object code without
>> CONFIG_PROFILE_ANNOTATED_BRANCHES, as tested with gcc-4.9
>> and gcc-7.1.1, so this seems to be a safe workaround.
>>
>> Fixes: 9a568de4818d ("tcp: switch TCP TS option (RFC 7323) to 1ms clock")
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> This reaches the limits at which I am willing to work around compiler
> stuff.

I see. It is a definitely a really obscure case, so if there is any doubt
that the workaround is harmless, then we shouldn't take it. The warning
only shows up on gcc-4.9 but not anything newer, and we disable
-Wmaybe-uninitialized on all older versions because of the false
positives.

It's also possible that it needed a combination of multiple other options,
not just CONFIG_PROFILE_ANNOTATED_BRANCHES. I build-tested
with gcc-4.9 to see if anything would show up that we don't also get a
warning for in gcc-7, and this came up once in several hundred randconfig
builds across multiple architectures (no other new warnings appeared
with gcc-4.9).

> What cpu did you test the object code generation upon and does that
> cpu have branch prediction hints in the target you are building for?

This was a randconfig build targetting ARMv5. I'm pretty sure that has
no such hint instructions.

       Arnd

[toc] | [prev] | [next] | [standalone]

#1735292

From: Arnd Bergmann <arnd@arndb.de>
Date: Tue, 19 Sep 2017 23:32:33 +0200

> On Tue, Sep 19, 2017 at 11:02 PM, David Miller <davem@davemloft.net> wrote:
>> What cpu did you test the object code generation upon and does that
>> cpu have branch prediction hints in the target you are building for?
> 
> This was a randconfig build targetting ARMv5. I'm pretty sure that has
> no such hint instructions.

I just tested on sparc64 and it changed the branch prediction:

 .L2157:
-       brz,pn  %i3, .L1898     ! first_ackt,
+       brz,pt  %i2, .L1898     ! first_ackt,
         mov    -1, %o2 !, seq_rtt_us

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web