Skip to contentSkip to conversation

: suspicious unlikely usage in tcp_transmit_skb()

PATCH In review Hua Zhong 4 participants 7 messages Apr 24, 2006 – Apr 26, 2006

Subsystems (3)
Networking Network drivers TCP
lore
Share & tools
lore

Hua Zhong

author
#0 lore raw

PATCH 1/1 : suspicious unlikely usage in tcp_transmit_skb()

Hi,

I am developing a profiling tool to check if likely/unlikely usages are wise. I find that the following one is always a miss:

# Hit # miss Function:Filename@Line
! 0 50505 tcp_transmit_skb():net/ipv4/tcp_output.c@468

There is a chance that my tool is buggy, but I just want to confirm with you whether this does look suspicious and what your opinion is.

Signed-off-by Hua Zhong <[email protected]>
net/ipv4/tcp_output.c +1 -1
index a28ae59..743016b 100644
@@ -465,7 +465,7 @@ #define SYSCTL_FLAG_SACK    0x4
       TCP_INC_STATS(TCP_MIB_OUTSEGS);
 
       err = icsk->icsk_af_ops->queue_xmit(skb, 0);
-       if (unlikely(err <= 0))
+       if (likely(err <= 0))
               return err;
 
       tcp_enter_cwr(sk);
#1 lore raw
Quoted 7 lines · Hua Zhong

I am developing a profiling tool to check if likely/unlikely usages are wise. I find that the following one is always a miss:

# Hit # miss Function:Filename@Line
! 0 50505 tcp_transmit_skb():net/ipv4/tcp_output.c@468

There is a chance that my tool is buggy, but I just want to confirm with you whether this does look suspicious and what your opinion is.

Your tool is correct. The only way for it to fall through is if we encounter local congestion.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#2 lore raw
In reply to Hua Zhong
Quoted 16 lines · On Mon, 24 Apr 2006 16:25:39 -0700 Hua Zhong <[email protected]> wrote:

Hi,

I am developing a profiling tool to check if likely/unlikely usages are wise. I find that the following one is always a miss:

# Hit # miss Function:Filename@Line
! 0 50505 tcp_transmit_skb():net/ipv4/tcp_output.c@468

There is a chance that my tool is buggy, but I just want to confirm with you whether this does look suspicious and what your opinion is.

Signed-off-by Hua Zhong <[email protected]>
net/ipv4/tcp_output.c +0 -0
index a28ae59..743016b 100644

net/ipv4/tcp_output.c #define SYSCTL_FLAG_SACK 0x4 · line 471

Quoted 8 lines
net/ipv4/tcp_output.c +1 -1
@@ -465,7 +465,7 @@ #define SYSCTL_FLAG_SACK    0x4
        TCP_INC_STATS(TCP_MIB_OUTSEGS);
 
        err = icsk->icsk_af_ops->queue_xmit(skb, 0);
-       if (unlikely(err <= 0))
+       if (likely(err <= 0))
                return err;
 
        tcp_enter_cwr(sk);

How about just taking off the likely/unlikely in this case.

maintainer
#3 lore raw

From: Stephen Hemminger <[email protected]>
Date: Tue, 25 Apr 2006 10:01:49 -0700

Quoted 2 lines
Quoted 2 lines

# Hit # miss Function:Filename@Line
! 0 50505 tcp_transmit_skb():net/ipv4/tcp_output.c@468

...

Quoted 1 line

How about just taking off the likely/unlikely in this case.

Why remove it when we'll now get a 50505 to 0 hit rate?

#4 lore raw
Quoted 9 lines · On Tue, 25 Apr 2006 14:46:49 -0700 (PDT) "David S. Miller"…

From: Stephen Hemminger <[email protected]>
Date: Tue, 25 Apr 2006 10:01:49 -0700

Quoted 2 lines
Quoted 2 lines

# Hit # miss Function:Filename@Line
! 0 50505 tcp_transmit_skb():net/ipv4/tcp_output.c@468

...

Quoted 1 line

How about just taking off the likely/unlikely in this case.

Why remove it when we'll now get a 50505 to 0 hit rate?

Depends on the data stream, but I guess if we are seeing high loss we really don't care about the CPU branch prediction.

maintainer
#5 lore raw

From: Stephen Hemminger <[email protected]>
Date: Tue, 25 Apr 2006 15:16:35 -0700

Quoted 8 lines
Quoted 5 lines · On Tue, 25 Apr 2006 14:46:49 -0700 (PDT) "David S. Miller"…

From: Stephen Hemminger <[email protected]>
Date: Tue, 25 Apr 2006 10:01:49 -0700

Quoted 2 lines
Quoted 2 lines

# Hit # miss Function:Filename@Line
! 0 50505 tcp_transmit_skb():net/ipv4/tcp_output.c@468

Quoted 7 lines
Quoted 4 lines

...

Quoted 1 line

How about just taking off the likely/unlikely in this case.

Why remove it when we'll now get a 50505 to 0 hit rate?

Depends on the data stream, but I guess if we are seeing high loss we really don't care about the CPU branch prediction.

I disagree, this is a hard error condition, and happens only when af_ops->queue_xmit() cannot send the packet successfully. Under any normal circumstances whatsoever, it will succeed.

Are you actually looking at the right piece of code? :-)

maintainer
#6 lore raw
In reply to Hua Zhong

From: Hua Zhong <[email protected]>
Date: Mon, 24 Apr 2006 16:25:39 -0700

Quoted 10 lines

Hi,

I am developing a profiling tool to check if likely/unlikely usages are wise. I find that the following one is always a miss:

# Hit # miss Function:Filename@Line
! 0 50505 tcp_transmit_skb():net/ipv4/tcp_output.c@468

There is a chance that my tool is buggy, but I just want to confirm with you whether this does look suspicious and what your opinion is.

Signed-off-by Hua Zhong <[email protected]>

Your patch is semantically correct but does not apply, because your email client has turned all of the tab characters in the patch into spaces. This corrupts the patch and makes it unusable.

This problem is hit by pretty much every single gmail user that tries to send a patch for the first time. I wish gmail would not mangle ascii text by default.

Please fix this and repost, retaining a proper changelog entry and signed off line, thank you.