Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
Date:   Thu, 9 Jul 2020 22:36:18 +0530
To:     joel@joelfernandes.org
Cc:     madhuparnabhowmik10@gmail.com, paulmck@kernel.org,
        rcu@vger.kernel.org, linux-kernel@vger.kernel.org,
        pbonzini@redhat.com, frextrite@gmail.com
Subject: Re: [PATCH] rculist : Introduce list/hlist_for_each_entry_srcu()
 macros
Message-ID: <20200709170617.GA19062@madhuparna-HP-Notebook>
References: <20200703140828.7016-1-madhuparnabhowmik10@gmail.com>
 <D24A0BBD-9A1A-4480-B36A-691C3AD1ADD5@joelfernandes.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <D24A0BBD-9A1A-4480-B36A-691C3AD1ADD5@joelfernandes.org>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Jul 08, 2020 at 08:33:49AM -0400, joel@joelfernandes.org wrote:
> 
> 
> On July 3, 2020 10:08:28 AM EDT, madhuparnabhowmik10@gmail.com wrote:
> >From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
> >
> >list/hlist_for_each_entry_rcu() provides an optional cond argument
> >to specify the lock held in the updater side.
> >However for SRCU read side, not providing the cond argument results
> >into false positive as whether srcu_read_lock is held or not is not
> >checked implicitly. Therefore, on read side the lockdep expression
> >srcu_read_lock_held(srcu struct) can solve this issue.
> >
> >However, the function still fails to check the cases where srcu
> >protected list is traversed with rcu_read_lock() instead of
> >srcu_read_lock(). Therefore, to remove the false negative,
> >this patch introduces two new list traversal primitives :
> >list_for_each_entry_srcu() and hlist_for_each_entry_srcu().
> >
> >Both of the functions have non-optional cond argument
> >as it is required for both read and update side, and simply checks
> >if the cond is true.
> 
> Looks ok to me. Could you update the comment below to also clarify that in regular read side usage, the traversal can be done by also passing the expression srcu_read_lock_held which is also a lockdep expression.
> 
> Could you post the user-patches along with it? That gives more context to reviewers.
>
Thank you for reviewing the patch, I will make the changes suggested and
send the patch along with the user patches.

Regards,
Madhuparna

> Thanks!
> 
> - Joel
> 
> 
> >
> >Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> >Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
> >---
> > include/linux/rculist.h | 44 +++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 44 insertions(+)
> >
> >diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> >index df587d181844..04a7e5791c39 100644
> >--- a/include/linux/rculist.h
> >+++ b/include/linux/rculist.h
> >@@ -63,9 +63,17 @@ static inline void INIT_LIST_HEAD_RCU(struct
> >list_head *list)
> > 	RCU_LOCKDEP_WARN(!(cond) && !rcu_read_lock_any_held(),		\
> > 			 "RCU-list traversed in non-reader section!");	\
> > 	})
> >+
> >+#define __list_check_srcu(cond)					 \
> >+	({								 \
> >+	RCU_LOCKDEP_WARN(!(cond),					 \
> >+		"RCU-list traversed without holding the required lock!");\
> >+	})
> > #else
> > #define __list_check_rcu(dummy, cond, extra...)				\
> > 	({ check_arg_count_one(extra); })
> >+
> >+#define __list_check_srcu(cond)
> > #endif
> > 
> > /*
> >@@ -383,6 +391,23 @@ static inline void
> >list_splice_tail_init_rcu(struct list_head *list,
> > 		&pos->member != (head);					\
> > 		pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
> > 
> >+/**
> >+ * list_for_each_entry_srcu	-	iterate over rcu list of given type
> >+ * @pos:	the type * to use as a loop cursor.
> >+ * @head:	the head for your list.
> >+ * @member:	the name of the list_head within the struct.
> >+ * @cond:	lockdep expression for the lock required to traverse the
> >list.
> >+ *
> >+ * This list-traversal primitive may safely run concurrently with
> >+ * the _rcu list-mutation primitives such as list_add_rcu()
> >+ * as long as the traversal is guarded by srcu_read_lock().
> >+ */
> >+#define list_for_each_entry_srcu(pos, head, member, cond)		\
> >+	for (__list_check_srcu(cond),					\
> >+	     pos = list_entry_rcu((head)->next, typeof(*pos), member);	\
> >+		&pos->member != (head);					\
> >+		pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
> >+
> > /**
> >  * list_entry_lockless - get the struct for this entry
> >  * @ptr:        the &struct list_head pointer.
> >@@ -681,6 +706,25 @@ static inline void hlist_add_behind_rcu(struct
> >hlist_node *n,
> > 		pos = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu(\
> > 			&(pos)->member)), typeof(*(pos)), member))
> > 
> >+/**
> >+ * hlist_for_each_entry_srcu - iterate over rcu list of given type
> >+ * @pos:	the type * to use as a loop cursor.
> >+ * @head:	the head for your list.
> >+ * @member:	the name of the hlist_node within the struct.
> >+ * @cond:	lockdep expression for the lock required to traverse the
> >list.
> >+ *
> >+ * This list-traversal primitive may safely run concurrently with
> >+ * the _rcu list-mutation primitives such as hlist_add_head_rcu()
> >+ * as long as the traversal is guarded by srcu_read_lock().
> >+ */
> >+#define hlist_for_each_entry_srcu(pos, head, member, cond)		\
> >+	for (__list_check_srcu(cond),					\
> >+	     pos =
> >hlist_entry_safe(rcu_dereference_raw(hlist_first_rcu(head)),\
> >+			typeof(*(pos)), member);			\
> >+		pos;							\
> >+		pos = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu(\
> >+			&(pos)->member)), typeof(*(pos)), member))
> >+
> > /**
> >* hlist_for_each_entry_rcu_notrace - iterate over rcu list of given
> >type (for tracing)
> >  * @pos:	the type * to use as a loop cursor.
> 
> -- 
> Sent from my Android device with K-9 Mail. Please excuse my brevity.