2006-12-03 17:02:33

by Frank van Maarseveen

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

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

2006-12-04 02:26:53

by Wu Fengguang

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

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 <[email protected]>

2006-12-04 10:19:33

by Nick Piggin

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

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

2006-12-04 10:37:04

by Wu Fengguang

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

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