Re: [net-next PATCH 0/6] ipv4/fib_trie: Cleanups to prepare for introduction of key vector

[Alexander Duyck <alexander.h.duyck@redhat.com>]

On 03/03/2015 09:16 PM, David Miller wrote:
> From: Alexander Duyck <alexander.h.duyck@redhat.com>
> Date: Mon, 02 Mar 2015 13:32:16 -0800
>
>> This patch series is meant to mostly just clean up the fib_trie to prepare
>> it for the introduction of the key_vector.  As such there are a number of
>> minor clean-ups such as reformatting the tnode to match the format once the
>> key vector is introduced, some optimizations to drop the need for a leaf
>> parent pointer, and some changes to remove duplication of effort such as
>> the 2 look-ups that were essentially being done per node insertion.
> This doesn't compile with trie stats enabled, I see something sneaking
> in from the main/local table collapsing patch :-)
>
> net/ipv4/fib_trie.c: In function ‘__trie_free_rcu’:
> net/ipv4/fib_trie.c:1601:36: error: ‘struct fib_table’ has no member named ‘data’
>    struct trie *t = (struct trie *)tb->data;

Ugh, sorry about that I will fix it this morning.

> Also, two comments:
>
> 1) When you go "if (idx >> n->bits)", can n->bits be == 32?  If so, this
>     expression is undefined.

If bits can be 32 then idx should be an unsigned long which is 64 bits.  
I will double check to make sure that is still the case.

The general idea is if a long is 32 bits then we likely cannot allocate 
a 32 bit tnode since that would more than consume all of the available 
memory in the system.  I can look into adding a comment to that effect 
somewhere.

> 2) In your simplification of fib_find_node(), don't keep storing over
>     and over again into the on-stack variable '*tp', instead just maintain
>     a one behind pointer in a local variable for the parent, and store it
>     one time as your exit the function.
>
> Thanks.

Okay I will update it.  As I recall that is what the compiler ends up 
doing anyway since fib_find_node ends up being inlined leaving the 
parent as just a local variable.

- Alex