[PATCH] rculist: unify documentation about missing list_empty_rcu()

[PATCH] rculist: unify documentation about missing list_empty_rcu()

[Julian Wiedmann]

We have two separate sections that talk about why list_empty_rcu()
is not needed, consolidate them.

Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com>
---
 include/linux/rculist.h | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/include/linux/rculist.h b/include/linux/rculist.h
index f8633d37e358..f8f42ce04382 100644
--- a/include/linux/rculist.h
+++ b/include/linux/rculist.h
@@ -10,15 +10,6 @@
 #include <linux/list.h>
 #include <linux/rcupdate.h>
 
-/*
- * Why is there no list_empty_rcu()?  Because list_empty() serves this
- * purpose.  The list_empty() function fetches the RCU-protected pointer
- * and compares it to the address of the list head, but neither dereferences
- * this pointer itself nor provides this pointer to the caller.  Therefore,
- * it is not necessary to use rcu_dereference(), so that list_empty() can
- * be used anywhere you would want to use a list_empty_rcu().
- */
-
 /*
  * INIT_LIST_HEAD_RCU - Initialize a list_head visible to RCU readers
  * @list: list to be initialized
@@ -334,6 +325,12 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
  * list_first_entry() because they would be protected by a lock that blocks
  * writers.
  *
+ * list_empty() fetches the RCU-protected pointer and compares it to the address
+ * of the list head. But it neither dereferences this pointer itself, nor
+ * provides this pointer to the caller. Therefore it is not necessary to use
+ * rcu_dereference(), and list_empty() can be used anywhere you would want to
+ * use a list_empty_rcu().
+ *
  * See list_first_or_null_rcu for an alternative.
  */
 
-- 
2.25.1

Re: [PATCH] rculist: unify documentation about missing list_empty_rcu()

[Paul E. McKenney]

On Fri, May 21, 2021 at 12:08:29PM +0200, Julian Wiedmann wrote:
> We have two separate sections that talk about why list_empty_rcu()
> is not needed, consolidate them.
> 
> Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com>

Good catch, thank you!  As usual, I could not resist the urge to further
wordsmith, resulting in the following.  Please let me know if I messed
anything up.

							Thanx, Paul

------------------------------------------------------------------------

commit 6e9da58a4b391035e1ce77b8d867cdcdc73521b2
Author: Julian Wiedmann <jwi@linux.ibm.com>
Date:   Fri May 21 12:08:29 2021 +0200

    rculist: Unify documentation about missing list_empty_rcu()
    
    We have two separate sections that talk about why list_empty_rcu()
    is not needed, so this commit consolidates them.
    
    Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com>
    [ paulmck: The usual wordsmithing. ]
    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

diff --git a/include/linux/rculist.h b/include/linux/rculist.h
index f8633d37e358..d29740be4833 100644
--- a/include/linux/rculist.h
+++ b/include/linux/rculist.h
@@ -10,15 +10,6 @@
 #include <linux/list.h>
 #include <linux/rcupdate.h>
 
-/*
- * Why is there no list_empty_rcu()?  Because list_empty() serves this
- * purpose.  The list_empty() function fetches the RCU-protected pointer
- * and compares it to the address of the list head, but neither dereferences
- * this pointer itself nor provides this pointer to the caller.  Therefore,
- * it is not necessary to use rcu_dereference(), so that list_empty() can
- * be used anywhere you would want to use a list_empty_rcu().
- */
-
 /*
  * INIT_LIST_HEAD_RCU - Initialize a list_head visible to RCU readers
  * @list: list to be initialized
@@ -318,21 +309,29 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
 /*
  * Where are list_empty_rcu() and list_first_entry_rcu()?
  *
- * Implementing those functions following their counterparts list_empty() and
- * list_first_entry() is not advisable because they lead to subtle race
- * conditions as the following snippet shows:
+ * They do not exist because they would lead to subtle race conditions:
  *
  * if (!list_empty_rcu(mylist)) {
  *	struct foo *bar = list_first_entry_rcu(mylist, struct foo, list_member);
  *	do_something(bar);
  * }
  *
- * The list may not be empty when list_empty_rcu checks it, but it may be when
- * list_first_entry_rcu rereads the ->next pointer.
- *
- * Rereading the ->next pointer is not a problem for list_empty() and
- * list_first_entry() because they would be protected by a lock that blocks
- * writers.
+ * The list might be non-empty when list_empty_rcu() checks it, but it
+ * might have become empty by the time that list_first_entry_rcu() rereads
+ * the ->next pointer, which would result in a SEGV.
+ *
+ * When not using RCU, it is OK for list_first_entry() to re-read that
+ * pointer because both functions should be protected by some lock that
+ * blocks writers.
+ *
+ * When using RCU, list_empty() uses READ_ONCE() to fetch the
+ * RCU-protected ->next pointer and then compares it to the address of the
+ * list head.  However, it neither dereferences this pointer nor provides
+ * this pointer to its caller.  Thus, READ_ONCE() suffices (that is,
+ * rcu_dereference() is not needed), which means that list_empty() can be
+ * used anywhere you would want to use list_empty_rcu().  Just don't
+ * expect anything useful to happen if you do a subsequent lockless
+ * call to list_first_entry_rcu()!!!
  *
  * See list_first_or_null_rcu for an alternative.
  */

Re: [PATCH] rculist: unify documentation about missing list_empty_rcu()

[Julian Wiedmann]

On 21.05.21 20:56, Paul E. McKenney wrote:
> On Fri, May 21, 2021 at 12:08:29PM +0200, Julian Wiedmann wrote:
>> We have two separate sections that talk about why list_empty_rcu()
>> is not needed, consolidate them.
>>
>> Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com>
> 
> Good catch, thank you!  As usual, I could not resist the urge to further
> wordsmith, resulting in the following.  Please let me know if I messed
> anything up.
> 
> 							Thanx, Paul
> 

I expected no different ;). LGTM, and clearly emphasizing that one shall
not mix list_empty() with list_first_entry_rcu() is a nice improvement.


> ------------------------------------------------------------------------
> 
> commit 6e9da58a4b391035e1ce77b8d867cdcdc73521b2
> Author: Julian Wiedmann <jwi@linux.ibm.com>
> Date:   Fri May 21 12:08:29 2021 +0200
> 
>     rculist: Unify documentation about missing list_empty_rcu()
>     
>     We have two separate sections that talk about why list_empty_rcu()
>     is not needed, so this commit consolidates them.
>     
>     Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com>
>     [ paulmck: The usual wordsmithing. ]
>     Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> 
> diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> index f8633d37e358..d29740be4833 100644
> --- a/include/linux/rculist.h
> +++ b/include/linux/rculist.h
> @@ -10,15 +10,6 @@
>  #include <linux/list.h>
>  #include <linux/rcupdate.h>
>  
> -/*
> - * Why is there no list_empty_rcu()?  Because list_empty() serves this
> - * purpose.  The list_empty() function fetches the RCU-protected pointer
> - * and compares it to the address of the list head, but neither dereferences
> - * this pointer itself nor provides this pointer to the caller.  Therefore,
> - * it is not necessary to use rcu_dereference(), so that list_empty() can
> - * be used anywhere you would want to use a list_empty_rcu().
> - */
> -
>  /*
>   * INIT_LIST_HEAD_RCU - Initialize a list_head visible to RCU readers
>   * @list: list to be initialized
> @@ -318,21 +309,29 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
>  /*
>   * Where are list_empty_rcu() and list_first_entry_rcu()?
>   *
> - * Implementing those functions following their counterparts list_empty() and
> - * list_first_entry() is not advisable because they lead to subtle race
> - * conditions as the following snippet shows:
> + * They do not exist because they would lead to subtle race conditions:
>   *
>   * if (!list_empty_rcu(mylist)) {
>   *	struct foo *bar = list_first_entry_rcu(mylist, struct foo, list_member);
>   *	do_something(bar);
>   * }
>   *
> - * The list may not be empty when list_empty_rcu checks it, but it may be when
> - * list_first_entry_rcu rereads the ->next pointer.
> - *
> - * Rereading the ->next pointer is not a problem for list_empty() and
> - * list_first_entry() because they would be protected by a lock that blocks
> - * writers.
> + * The list might be non-empty when list_empty_rcu() checks it, but it
> + * might have become empty by the time that list_first_entry_rcu() rereads
> + * the ->next pointer, which would result in a SEGV.
> + *
> + * When not using RCU, it is OK for list_first_entry() to re-read that
> + * pointer because both functions should be protected by some lock that
> + * blocks writers.
> + *
> + * When using RCU, list_empty() uses READ_ONCE() to fetch the
> + * RCU-protected ->next pointer and then compares it to the address of the
> + * list head.  However, it neither dereferences this pointer nor provides
> + * this pointer to its caller.  Thus, READ_ONCE() suffices (that is,
> + * rcu_dereference() is not needed), which means that list_empty() can be
> + * used anywhere you would want to use list_empty_rcu().  Just don't
> + * expect anything useful to happen if you do a subsequent lockless
> + * call to list_first_entry_rcu()!!!
>   *
>   * See list_first_or_null_rcu for an alternative.
>   */
>