* radix-tree.c:__lookup_slot() dead code removal
@ 2006-12-03 17:02 Frank van Maarseveen
2006-12-04 2:26 ` Fengguang Wu
2006-12-04 10:18 ` Nick Piggin
0 siblings, 2 replies; 4+ messages in thread
From: Frank van Maarseveen @ 2006-12-03 17:02 UTC (permalink / raw)
To: linux-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: radix-tree.c:__lookup_slot() dead code removal
@ 2006-12-04 2:26 ` Fengguang Wu
0 siblings, 0 replies; 4+ messages in thread
From: Fengguang Wu @ 2006-12-04 2:26 UTC (permalink / raw)
To: Frank van Maarseveen; +Cc: linux-kernel, Nick Piggin
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>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: radix-tree.c:__lookup_slot() dead code removal
@ 2006-12-04 2:26 ` Fengguang Wu
0 siblings, 0 replies; 4+ messages in thread
From: Fengguang Wu @ 2006-12-04 2:26 UTC (permalink / raw)
To: Frank van Maarseveen; +Cc: linux-kernel, Nick Piggin
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>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: radix-tree.c:__lookup_slot() dead code removal
2006-12-03 17:02 radix-tree.c:__lookup_slot() dead code removal Frank van Maarseveen
2006-12-04 2:26 ` Fengguang Wu
@ 2006-12-04 10:18 ` Nick Piggin
2006-12-04 10:37 ` WU Fengguang
1 sibling, 1 reply; 4+ messages in thread
From: Nick Piggin @ 2006-12-04 10:18 UTC (permalink / raw)
To: Frank van Maarseveen; +Cc: linux-kernel, WU Fengguang, Andrew Morton
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: radix-tree.c:__lookup_slot() dead code removal
@ 2006-12-04 10:37 ` WU Fengguang
0 siblings, 0 replies; 4+ messages in thread
From: WU Fengguang @ 2006-12-04 10:37 UTC (permalink / raw)
To: Nick Piggin; +Cc: Frank van Maarseveen, linux-kernel, Andrew Morton
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: radix-tree.c:__lookup_slot() dead code removal
@ 2006-12-04 10:37 ` WU Fengguang
0 siblings, 0 replies; 4+ messages in thread
From: WU Fengguang @ 2006-12-04 10:37 UTC (permalink / raw)
To: Nick Piggin; +Cc: Frank van Maarseveen, linux-kernel, Andrew Morton
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2006-12-04 10:37 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-12-03 17:02 radix-tree.c:__lookup_slot() dead code removal Frank van Maarseveen
2006-12-04 2:26 ` Fengguang Wu
2006-12-04 2:26 ` Fengguang Wu
2006-12-04 10:18 ` Nick Piggin
2006-12-04 10:37 ` WU Fengguang
2006-12-04 10:37 ` WU Fengguang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.