radix-tree.c:__lookup_slot() dead code removal

radix-tree.c:__lookup_slot() dead code removal

[Frank van Maarseveen]

Most of the code suggests that it is valid to insert a NULL item,
possibly a zero item with pointer cast. However, in __lookup_slot()
whether or not the slot is found seems to depend on the actual value
of the item in one special case. But further on it doesn't make any
difference so to remove some dead code:

--- a/lib/radix-tree.c	2006-12-03 13:23:00.000000000 +0100
+++ b/lib/radix-tree.c	2006-12-03 17:57:03.000000000 +0100
@@ -319,9 +319,6 @@ static inline void **__lookup_slot(struc
 	if (index > radix_tree_maxindex(height))
 		return NULL;
 
-	if (height == 0 && root->rnode)
-		return (void **)&root->rnode;
-
 	shift = (height-1) * RADIX_TREE_MAP_SHIFT;
 	slot = &root->rnode;
 

-- 
Frank

Re: radix-tree.c:__lookup_slot() dead code removal

[Fengguang Wu]

On Sun, Dec 03, 2006 at 06:02:31PM +0100, Frank van Maarseveen wrote:
> --- a/lib/radix-tree.c	2006-12-03 13:23:00.000000000 +0100
> +++ b/lib/radix-tree.c	2006-12-03 17:57:03.000000000 +0100
> @@ -319,9 +319,6 @@ static inline void **__lookup_slot(struc
>  	if (index > radix_tree_maxindex(height))
>  		return NULL;
>  
> -	if (height == 0 && root->rnode)
> -		return (void **)&root->rnode;
> -
>  	shift = (height-1) * RADIX_TREE_MAP_SHIFT;
>  	slot = &root->rnode;

Acked-by: Fengguang Wu <wfg@mail.ustc.edu.cn>

Re: radix-tree.c:__lookup_slot() dead code removal

[Nick Piggin]

Frank van Maarseveen wrote:
> Most of the code suggests that it is valid to insert a NULL item,
> possibly a zero item with pointer cast. However, in __lookup_slot()
> whether or not the slot is found seems to depend on the actual value
> of the item in one special case. But further on it doesn't make any
> difference so to remove some dead code:
> 
> --- a/lib/radix-tree.c	2006-12-03 13:23:00.000000000 +0100
> +++ b/lib/radix-tree.c	2006-12-03 17:57:03.000000000 +0100
> @@ -319,9 +319,6 @@ static inline void **__lookup_slot(struc
>  	if (index > radix_tree_maxindex(height))
>  		return NULL;
>  
> -	if (height == 0 && root->rnode)
> -		return (void **)&root->rnode;
> -
>  	shift = (height-1) * RADIX_TREE_MAP_SHIFT;
>  	slot = &root->rnode;

I would say it is not valid to insert a NULL item (because NULL
means an unsuccessful lookup, you may as well just delete the
item).

Also, I don't see how this is dead code anyway. height == 0
radix-trees are a special case and do not have a radix_tree_node
at ->rnode.

-- 
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: radix-tree.c:__lookup_slot() dead code removal

[WU Fengguang]

On Mon, Dec 04, 2006 at 09:18:40PM +1100, Nick Piggin wrote:
> Frank van Maarseveen wrote:
> >Most of the code suggests that it is valid to insert a NULL item,
> >possibly a zero item with pointer cast. However, in __lookup_slot()
> >whether or not the slot is found seems to depend on the actual value
> >of the item in one special case. But further on it doesn't make any
> >difference so to remove some dead code:
> >
> >--- a/lib/radix-tree.c	2006-12-03 13:23:00.000000000 +0100
> >+++ b/lib/radix-tree.c	2006-12-03 17:57:03.000000000 +0100
> >@@ -319,9 +319,6 @@ static inline void **__lookup_slot(struc
> > 	if (index > radix_tree_maxindex(height))
> > 		return NULL;
> >
> >-	if (height == 0 && root->rnode)
> >-		return (void **)&root->rnode;
> >-
> > 	shift = (height-1) * RADIX_TREE_MAP_SHIFT;
> > 	slot = &root->rnode;
>
> I would say it is not valid to insert a NULL item (because NULL
> means an unsuccessful lookup, you may as well just delete the
> item).
>
> Also, I don't see how this is dead code anyway. height == 0
> radix-trees are a special case and do not have a radix_tree_node
> at ->rnode.

Sorry.

For linux-2.6.19 it was redundant code:

        if (height == 0 && root->rnode)
                return (void **)&root->rnode;

        shift = (height-1) * RADIX_TREE_MAP_SHIFT;
        slot = &root->rnode;

        while (height > 0) {
                ......
        }

        return (void **)slot;

I just find out it is no longer the case for -mm: the function has been heavily
reworked.

So, the patch is no longer valid.

Fengguang Wu