netdev - Re: fib_trie: commit 95f60ea3e99a missing else?

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Fri, 03 Apr 2015 17:32:43 -0700
From:	Alexander Duyck <alexander.duyck@...il.com>
To:	Joe Perches <joe@...ches.com>,
	Alexander Duyck <alexander.h.duyck@...hat.com>
CC:	netdev <netdev@...r.kernel.org>
Subject: Re: fib_trie: commit 95f60ea3e99a missing else?

On 04/03/2015 01:53 PM, Joe Perches wrote:
> Hello Alexander.
> 
> commit 95f60ea3e99a ("fib_trie: Add collapse() and should_collapse() to resize")
> changed this block from:
> 
> 	if (a && !b)
> 		...
> 	else if (!a && b)
> 		...
> 
> to:
> 
> 	if (a && !b)
> 		...
> 	if (!a && b)
> 		... 
> 
> Was there a reason for the "else" removal?

Because it was ugly and unnecessary.  The two cases are already mutually
exclusive, and it isn't as if the value of a or b changes between the
tests.  GCC is usually smart enough to realize this so when it tests a
it will either continue with the !b check or switch to the other
statement and check for b being set.

> I notice that object code size increases a bit (x86-64)
> if the else is put back.

It is likely because it is checking for (a or !b) and then coming back
around and checking for (!a && b) when you add the else.  I have found
that GCC is fairly smart in terms of checks so there isn't much
advantage to using an else if two tests are mutually exclusive.

> net/ipv4/fib_trie.c
> []
> @@ -375,11 +388,11 @@ static void put_child(struct tnode *tn, unsigned long i, s
>  
>         BUG_ON(i >= tnode_child_length(tn));
>  
> -       /* update emptyChildren */
> +       /* update emptyChildren, overflow into fullChildren */
>         if (n == NULL && chi != NULL)
> -               tn->empty_children++;
> -       else if (n != NULL && chi == NULL)
> -               tn->empty_children--;
> +               empty_child_inc(tn);
> +       if (n != NULL && chi == NULL)
> +               empty_child_dec(tn);
> 

The "else" was an unnecessary optimization since this isn't really
fast-path code and the compiler is usually smart enough to understand
that depending on the value of n it is either going to test for
increment or decrement by testing the value of chi.

You could probably do an objdump of the function to better understand
the difference of one versus the other.  I suspect at worst what is
happening is that the check for !n is being dropped and the logic for
chi != NULL is just flowing through and performing one additional test
on chi before jumping past the empty_child_dec.

- Alex
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html