[PATCH]: suspicious unlikely usage in tcp_transmit_skb()

[PATCH]: suspicious unlikely usage in tcp_transmit_skb()

[Hua Zhong]

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 <hzhong@gmail.com>

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index a28ae59..743016b 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -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);

Re: [PATCH]: suspicious unlikely usage in tcp_transmit_skb()

[Herbert Xu]

Hua Zhong <hzhong@gmail.com> wrote:
> 
> 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,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH]: suspicious unlikely usage in tcp_transmit_skb()

[Stephen Hemminger]

On Mon, 24 Apr 2006 16:25:39 -0700
Hua Zhong <hzhong@gmail.com> 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 <hzhong@gmail.com>
> 
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index a28ae59..743016b 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -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.

Re: [PATCH]: suspicious unlikely usage in tcp_transmit_skb()

[David S. Miller]

From: Stephen Hemminger <shemminger@osdl.org>
Date: Tue, 25 Apr 2006 10:01:49 -0700

> >       # Hit    # miss Function:Filename@Line
> > !         0     50505 tcp_transmit_skb():net/ipv4/tcp_output.c@468
 ...
> 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?

Re: [PATCH]: suspicious unlikely usage in tcp_transmit_skb()

[Stephen Hemminger]

On Tue, 25 Apr 2006 14:46:49 -0700 (PDT)
"David S. Miller" <davem@davemloft.net> wrote:

> From: Stephen Hemminger <shemminger@osdl.org>
> Date: Tue, 25 Apr 2006 10:01:49 -0700
> 
> > >       # Hit    # miss Function:Filename@Line
> > > !         0     50505 tcp_transmit_skb():net/ipv4/tcp_output.c@468
>  ...
> > 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.

Re: [PATCH]: suspicious unlikely usage in tcp_transmit_skb()

[David S. Miller]

From: Stephen Hemminger <shemminger@osdl.org>
Date: Tue, 25 Apr 2006 15:16:35 -0700

> On Tue, 25 Apr 2006 14:46:49 -0700 (PDT)
> "David S. Miller" <davem@davemloft.net> wrote:
> 
> > From: Stephen Hemminger <shemminger@osdl.org>
> > Date: Tue, 25 Apr 2006 10:01:49 -0700
> > 
> > > >       # Hit    # miss Function:Filename@Line
> > > > !         0     50505 tcp_transmit_skb():net/ipv4/tcp_output.c@468
> >  ...
> > > 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? :-)

Re: [PATCH]: suspicious unlikely usage in tcp_transmit_skb()

[David S. Miller]

From: Hua Zhong <hzhong@gmail.com>
Date: Mon, 24 Apr 2006 16:25:39 -0700

> 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 <hzhong@gmail.com>

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.