radix-tree: cleanup radix_tree_deref_slot() and _lookup_slot() comments

'_deref_slot() function, and adds more explanation of expected/required locking to the direct slot access functions. I separated it out, because it doesn't fix a serious bug, like the previous one.

Paul: do you agree that we don't need rcu_dereference() in the _deref_slot() as it can only be used while the tree is held [probably write] locked? Do the comments look OK?

Lee

Cleanup radix tree slot dereference and lookup comments - 2.6.18-rc4-mm2

radix_tree_deref_slot() was actually dereferencing the pointer in the assignment to the local variable 'slot' and then rcu_dereference()ing the results of an expression [return value of an inline function].

Because we must hold the tree locked across lookup_slot() and _deref_slot(), we don't need the rcu_dereference() at all.

Added comments specifying required locking for _lookup_slot() and _deref_slot().

Signed-off-by Lee Schermerhorn <[email protected]>

 include/linux/radix-tree.h |    9 +++++++--
 lib/radix-tree.c           |    5 ++++-
 2 files changed, 11 insertions(+), 3 deletions(-)

Index: linux-2.6.18-rc4-mm2/lib/radix-tree.c
===================================================================

lib/radix-tree.c +4 -1

@@ -336,7 +336,10 @@ EXPORT_SYMBOL(radix_tree_insert);
  *	@root. This is useful for update-if-exists operations.
  *
  *	This function cannot be called under rcu_read_lock, it must be
- *	excluded from writers, as must the returned slot.
+ *	excluded from writers, as must the returned slot for subsequent
+ *	use by radix_tree_deref_slot() and radix_tree_replace slot.
+ *	Caller must hold tree write locked across slot lookup and
+ *	replace.
  */
 void **radix_tree_lookup_slot(struct radix_tree_root *root, unsigned long index)
 {

Index: linux-2.6.18-rc4-mm2/include/linux/radix-tree.h
===================================================================

Quoted 51 lines · On Tue, 22 Aug 2006 16:32:52 -0400 Lee Schermerhorn wrote:

'_deref_slot() function, and adds more explanation of expected/required locking to the direct slot access functions. I separated it out, because it doesn't fix a serious bug, like the previous one.

Paul: do you agree that we don't need rcu_dereference() in the _deref_slot() as it can only be used while the tree is held [probably write] locked? Do the comments look OK?

Lee

Cleanup radix tree slot dereference and lookup comments - 2.6.18-rc4-mm2

radix_tree_deref_slot() was actually dereferencing the pointer in the assignment to the local variable 'slot' and then rcu_dereference()ing the results of an expression [return value of an inline function].

Because we must hold the tree locked across lookup_slot() and _deref_slot(), we don't need the rcu_dereference() at all.

Added comments specifying required locking for _lookup_slot() and _deref_slot().

Signed-off-by Lee Schermerhorn <[email protected]>

 include/linux/radix-tree.h |    9 +++++++--
 lib/radix-tree.c           |    5 ++++-
 2 files changed, 11 insertions(+), 3 deletions(-)

Index: linux-2.6.18-rc4-mm2/lib/radix-tree.c
===================================================================

lib/radix-tree.c +4 -1

@@ -336,7 +336,10 @@ EXPORT_SYMBOL(radix_tree_insert);
  *	@root. This is useful for update-if-exists operations.
  *
  *	This function cannot be called under rcu_read_lock, it must be
- *	excluded from writers, as must the returned slot.
+ *	excluded from writers, as must the returned slot for subsequent
+ *	use by radix_tree_deref_slot() and radix_tree_replace slot.
+ *	Caller must hold tree write locked across slot lookup and
+ *	replace.
  */
 void **radix_tree_lookup_slot(struct radix_tree_root *root, unsigned long index)
 {

Index: linux-2.6.18-rc4-mm2/include/linux/radix-tree.h
===================================================================

include/linux/radix-tree.h do { \ · line 126

Quoted 6 lines

include/linux/radix-tree.h +2 -1

@@ -122,12 +122,17 @@ do {									\
 /**
  * radix_tree_deref_slot	- dereference a slot
  * @pslot:	pointer to slot, returned by radix_tree_lookup_slot
- * @returns:	item that was stored in that slot.
+ * @returns:	item that was stored in that slot with any direct pointer flag
+ *		removed.

I realize that you are just changing what was already there, but the @returns kernel-doc notation indicates that <returns> is a function parameter, but it's not, so please just make it like so:

* Returns: <text>

How about the attached patch? Applies atop my previous 2 radix tree patches [fix and cleanup]. Addresses Christoph's comment, as well.

Lee

Cleanup radix_tree_{deref|replace}_slot() calling conventions.

Adopt Christoph's suggestion to change calling conventions of radix_tree_replace_slot(). Make similar change to radix_tree_defer_slot().

Both now take a 'void **pslot' argument. Not only does this simplify the code of these two functions, but the arg type now matches the return type of radix_tree_lookup_slot().

Tested with reverted page migration, but I don't know that I was exercising any direct pointers.

Note: this will require changes to migrate_page_move_mapping() to avoid compiler warnings if/when we back out the work around patch for the problem in '_replace_slot() fixed by a prior patch. The migrate code is [will then be] the only user of the direct slot replacement APIs.

Also, fix '_deref_slot() and '_lookup_slot() return comments for Randy.

Signed-off-by Lee Schermerhorn <[email protected]>

 include/linux/radix-tree.h |   14 ++++++--------
 lib/radix-tree.c           |    4 ++--
 2 files changed, 8 insertions(+), 10 deletions(-)

Index: linux-2.6.18-rc4-mm2/include/linux/radix-tree.h
===================================================================

include/linux/radix-tree.h +6 -8

@@ -122,17 +122,16 @@ do {									\
 /**
  * radix_tree_deref_slot	- dereference a slot
  * @pslot:	pointer to slot, returned by radix_tree_lookup_slot
- * @returns:	item that was stored in that slot with any direct pointer flag
+ * Returns:	item that was stored in that slot with any direct pointer flag
  *		removed.
  *
  * For use with radix_tree_lookup_slot().  Caller must hold tree at least read
  * locked across slot lookup and dereference.  More likely, will be used with
  * radix_tree_replace_slot(), as well, so caller will hold tree write locked.
  */
-static inline void *radix_tree_deref_slot(void *pslot)
+static inline void *radix_tree_deref_slot(void **pslot)
 {
-	void *slot = *(void **)pslot;
-	return radix_tree_direct_to_ptr(slot);
+	return radix_tree_direct_to_ptr(*pslot);
 }
 /**
  * radix_tree_replace_slot	- replace item in a slot

@@ -142,13 +141,12 @@ static inline void *radix_tree_deref_slo
  * For use with radix_tree_lookup_slot().  Caller must hold tree write locked
  * across slot lookup and replacement.
  */
-static inline void radix_tree_replace_slot(void *pslot, void *item)
+static inline void radix_tree_replace_slot(void **pslot, void *item)
 {
-	void *slot = *(void **)pslot;
 	BUG_ON(radix_tree_is_direct_ptr(item));
-	rcu_assign_pointer(*(void **)pslot,
+	rcu_assign_pointer(*pslot,
 		(void *)((unsigned long)item |
-			((unsigned long)slot & RADIX_TREE_DIRECT_PTR)));
+			((unsigned long)*pslot & RADIX_TREE_DIRECT_PTR)));
 }
 
 int radix_tree_insert(struct radix_tree_root *, unsigned long, void *);

Index: linux-2.6.18-rc4-mm2/lib/radix-tree.c
===================================================================

Andrew: here is a second patch that just cleans up [I think] the '_deref_slot() function, and adds more explanation of expected/required locking to the direct slot access functions. I separated it out, because it doesn't fix a serious bug, like the previous one.

Paul: do you agree that we don't need rcu_dereference() in the _deref_slot() as it can only be used while the tree is held [probably write] locked? Do the comments look OK?

Lee

Cleanup radix tree slot dereference and lookup comments - 2.6.18-rc4-mm2

radix_tree_deref_slot() was actually dereferencing the pointer in the assignment to the local variable 'slot' and then rcu_dereference()ing the results of an expression [return value of an inline function].

Because we must hold the tree locked across lookup_slot() and _deref_slot(), we don't need the rcu_dereference() at all.

Added comments specifying required locking for _lookup_slot() and _deref_slot().

Signed-off-by Lee Schermerhorn <[email protected]>

 include/linux/radix-tree.h |    9 +++++++--
 lib/radix-tree.c           |    5 ++++-
 2 files changed, 11 insertions(+), 3 deletions(-)

Index: linux-2.6.18-rc4-mm2/lib/radix-tree.c
===================================================================

lib/radix-tree.c +4 -1

@@ -336,7 +336,10 @@ EXPORT_SYMBOL(radix_tree_insert);
  *	@root. This is useful for update-if-exists operations.
  *
  *	This function cannot be called under rcu_read_lock, it must be
- *	excluded from writers, as must the returned slot.
+ *	excluded from writers, as must the returned slot for subsequent
+ *	use by radix_tree_deref_slot() and radix_tree_replace slot.
+ *	Caller must hold tree write locked across slot lookup and
+ *	replace.
  */
 void **radix_tree_lookup_slot(struct radix_tree_root *root, unsigned long index)
 {

Index: linux-2.6.18-rc4-mm2/include/linux/radix-tree.h
===================================================================

include/linux/radix-tree.h +5 -1

@@ -122,12 +122,17 @@ do {									\
 /**
  * radix_tree_deref_slot	- dereference a slot
  * @pslot:	pointer to slot, returned by radix_tree_lookup_slot
- * @returns:	item that was stored in that slot.
+ * @returns:	item that was stored in that slot with any direct pointer flag
+ *		removed.
+ *
+ * For use with radix_tree_lookup_slot().  Caller must hold tree at least read
+ * locked across slot lookup and dereference.  More likely, will be used with

Quoted 5 lines · Lee Schermerhorn

Andrew: here is a second patch that just cleans up [I think] the '_deref_slot() function, and adds more explanation of expected/required locking to the direct slot access functions. I separated it out, because it doesn't fix a serious bug, like the previous one.

Quoted 3 lines

Paul: do you agree that we don't need rcu_dereference() in the _deref_slot() as it can only be used while the tree is held [probably write] locked? Do the comments look OK?

Yep, rcu_dereference() is not needed if the tree is prevented from changing. That said, rcu_dereference() is zero cost on all but Alpha, so there is little benefit to be had from removing it.

The comments look much improved.

Quoted 10 lines · Paul E. McKenney

Quoted 8 lines · Lee Schermerhorn

Andrew: here is a second patch that just cleans up [I think] the '_deref_slot() function, and adds more explanation of expected/required locking to the direct slot access functions. I separated it out, because it doesn't fix a serious bug, like the previous one.

Paul: do you agree that we don't need rcu_dereference() in the _deref_slot() as it can only be used while the tree is held [probably write] locked? Do the comments look OK?

Quoted 3 lines

Yep, rcu_dereference() is not needed if the tree is prevented from changing. That said, rcu_dereference() is zero cost on all but Alpha, so there is little benefit to be had from removing it.

I wasn't concerned about the cost. I just thought it would be "misleading" if, as you have verified, that it's not required, because the comment on rcu_dereference() says that one important aspect of using rcu_dereference() is to document which pointers are protected by RCU.

Quoted 1 line

The comments look much improved.

Thanks,
Lee