Re: [PATCH] net: code cleanups

On Thu, Sep 30, 2010 at 1:16 PM, Eric Dumazet [off-list ref] wrote:

Le jeudi 30 septembre 2010 à 10:24 +0800, Changli Gao a écrit :

quoted

Compare operations are more readable, and compilers generate the same code
for the both.

You have a buggy compiler then.

gcc version 4.4.3 (Gentoo 4.4.3-r2 p1.2)

             rth = rcu_dereference(rth->dst.rt_next)) {
                if ((((__force u32)rth->fl.fl4_dst ^ (__force u32)daddr) |
                     ((__force u32)rth->fl.fl4_src ^ (__force u32)saddr) |
                     (rth->fl.iif ^ iif) |
    2f12:       44 3b 80 dc 00 00 00    cmp    0xdc(%rax),%r8d
    2f19:       0f 85 a2 00 00 00       jne    2fc1 <ip_route_input_common+0x145

                     rth->fl.oif |
    2f1f:       83 b8 d8 00 00 00 00    cmpl   $0x0,0xd8(%rax)
    2f26:       0f 85 95 00 00 00       jne    2fc1 <ip_route_input_common+0x145

        tos &= IPTOS_RT_MASK;
        hash = rt_hash(daddr, saddr, iif, rt_genid(net));

        for (rth = rcu_dereference(rt_hash_table[hash].chain); rth;
             rth = rcu_dereference(rth->dst.rt_next)) {
                if ((((__force u32)rth->fl.fl4_dst ^ (__force u32)daddr) |
    2f2c:       44 3b b8 e4 00 00 00    cmp    0xe4(%rax),%r15d
    2f33:       0f 85 88 00 00 00       jne    2fc1
<ip_route_input_common+0x145>
                     ((__force u32)rth->fl.fl4_src ^ (__force u32)saddr) |
    2f39:       44 3b b0 e8 00 00 00    cmp    0xe8(%rax),%r14d
    2f40:       75 7f                   jne    2fc1
<ip_route_input_common+0x145>

I know this code is ugly, but please keep it as is, dont add conditional
branches on hot paths.

If the compiler doesn't generate conditional branches, we have to
touch every necessary field of all the cache entries in one hash
bucket. Is it better than condition branch? I think the compiler
developers know it better.

And the compiler reorders the conditional branches, is it expected?

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)