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

On 03/03/2015 09:16 PM, David Miller wrote:

From: Alexander Duyck <redacted>
Date: Mon, 02 Mar 2015 13:32:16 -0800

quoted

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