[PATCH tip/core/rcu 0/5] Provide SRCU polling grace-period interfaces

[PATCH tip/core/rcu 0/5] Provide SRCU polling grace-period interfaces

[Paul E. McKenney]

Hello!

This series provides a polling interface for SRCU grace periods.  The
API is as follows:

unsigned long start_poll_synchronize_srcu(struct srcu_struct *ssp)

	Returns a "cookie" that can be thought of as a snapshot of
	the specified SRCU instance's grace-period sequence number.
	Also ensures that enough future grace periods happen to eventually
	make the grace-period sequence number reach the cookie.

bool poll_state_synchronize_srcu(struct srcu_struct *ssp, unsigned long cookie)

	Given a cookie from start_poll_synchronize_srcu(), returns true if
	at least one full SRCU grace period has elapsed in the meantime.
	Given finite SRCU readers in a well-behaved kernel, the following
	code will complete in finite time:

		cookie = start_poll_synchronize_srcu(&my_srcu);
		while (!poll_state_synchronize_srcu(&my_srcu, cookie))
			schedule_timeout_uninterruptible(1);

unsigned long get_state_synchronize_srcu(struct srcu_struct *ssp)

	Like start_poll_synchronize_srcu(), except that it does not start
	any grace periods.  This means that the following code is -not-
	guaranteed to complete:

		cookie = get_state_synchronize_srcu(&my_srcu);
		while (!poll_state_synchronize_srcu(&my_srcu, cookie))
			schedule_timeout_uninterruptible(1);

	Use this if you know that something else will be starting the
	needed SRCU grace periods.  This might also be useful if you
	had items that were likely to be reused before the SRCU grace
	period elapsed, so that you avoid burning CPU on SRCU grace
	periods that prove to be unnecessary.  Or if you don't want
	to have more than (say) 100 SRCU grace periods per seconds,
	in which case you might use a timer to start the grace periods.
	Or maybe you don't bother starting the SRCU grace period until
	some sort of emergency situation has arisen.  Or...

	OK, maybe no one needs it, but rcutorture does need it, so here
	it is anyway.

The patches in this series are as follows:

1.	Make Tiny SRCU use multi-bit grace-period counter.

2.	Provide internal interface to start a Tiny SRCU grace period.

3.	Provide internal interface to start a Tree SRCU grace period.

4.	Provide polling interfaces for Tiny SRCU grace periods.

5.	Provide polling interfaces for Tree SRCU grace periods.

						Thanx, Paul

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

 include/linux/rcupdate.h |    2 
 include/linux/srcu.h     |    3 +
 include/linux/srcutiny.h |    5 +
 kernel/rcu/srcutiny.c    |   74 +++++++++++++++++++++++---
 kernel/rcu/srcutree.c    |  129 +++++++++++++++++++++++++++++++++++------------
 5 files changed, 169 insertions(+), 44 deletions(-)

[PATCH RFC tip/core/rcu 1/5] srcu: Make Tiny SRCU use multi-bit grace-period counter

[paulmck]

From: "Paul E. McKenney" <paulmck@kernel.org>

There is a need for a polling interface for SRCU grace periods.  This
polling needs to distinguish between an SRCU instance being idle on the
one hand or in the middle of a grace period on the other.  This commit
therefore converts the Tiny SRCU srcu_struct structure's srcu_idx from
a defacto boolean to a free-running counter, using the bottom bit to
indicate that a grace period is in progress.  The second-from-bottom
bit is thus used as the index returned by srcu_read_lock().

Link: https://lore.kernel.org/rcu/20201112201547.GF3365678@moria.home.lan/
Reported-by: Kent Overstreet <kent.overstreet@gmail.com>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
---
 include/linux/srcutiny.h | 4 ++--
 kernel/rcu/srcutiny.c    | 5 +++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/include/linux/srcutiny.h b/include/linux/srcutiny.h
index 5a5a194..fed4a2d 100644
--- a/include/linux/srcutiny.h
+++ b/include/linux/srcutiny.h
@@ -15,7 +15,7 @@
 
 struct srcu_struct {
 	short srcu_lock_nesting[2];	/* srcu_read_lock() nesting depth. */
-	short srcu_idx;			/* Current reader array element. */
+	unsigned short srcu_idx;	/* Current reader array element in bit 0x2. */
 	u8 srcu_gp_running;		/* GP workqueue running? */
 	u8 srcu_gp_waiting;		/* GP waiting for readers? */
 	struct swait_queue_head srcu_wq;
@@ -59,7 +59,7 @@ static inline int __srcu_read_lock(struct srcu_struct *ssp)
 {
 	int idx;
 
-	idx = READ_ONCE(ssp->srcu_idx);
+	idx = (READ_ONCE(ssp->srcu_idx) & 0x2) / 2;
 	WRITE_ONCE(ssp->srcu_lock_nesting[idx], ssp->srcu_lock_nesting[idx] + 1);
 	return idx;
 }
diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c
index 6208c1d..5598cf6 100644
--- a/kernel/rcu/srcutiny.c
+++ b/kernel/rcu/srcutiny.c
@@ -124,11 +124,12 @@ void srcu_drive_gp(struct work_struct *wp)
 	ssp->srcu_cb_head = NULL;
 	ssp->srcu_cb_tail = &ssp->srcu_cb_head;
 	local_irq_enable();
-	idx = ssp->srcu_idx;
-	WRITE_ONCE(ssp->srcu_idx, !ssp->srcu_idx);
+	idx = (ssp->srcu_idx & 0x2) / 2;
+	WRITE_ONCE(ssp->srcu_idx, ssp->srcu_idx + 1);
 	WRITE_ONCE(ssp->srcu_gp_waiting, true);  /* srcu_read_unlock() wakes! */
 	swait_event_exclusive(ssp->srcu_wq, !READ_ONCE(ssp->srcu_lock_nesting[idx]));
 	WRITE_ONCE(ssp->srcu_gp_waiting, false); /* srcu_read_unlock() cheap. */
+	WRITE_ONCE(ssp->srcu_idx, ssp->srcu_idx + 1);
 
 	/* Invoke the callbacks we removed above. */
 	while (lh) {
-- 
2.9.5

[PATCH RFC tip/core/rcu 2/5] srcu: Provide internal interface to start a Tiny SRCU grace period

[paulmck]

From: "Paul E. McKenney" <paulmck@kernel.org>

There is a need for a polling interface for SRCU grace periods.
This polling needs to initiate an SRCU grace period without
having to queue (and manage) a callback.  This commit therefore
splits the Tiny SRCU call_srcu() function into callback-queuing and
start-grace-period portions, with the latter in a new function named
srcu_gp_start_if_needed().

Link: https://lore.kernel.org/rcu/20201112201547.GF3365678@moria.home.lan/
Reported-by: Kent Overstreet <kent.overstreet@gmail.com>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
---
 kernel/rcu/srcutiny.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c
index 5598cf6..3bac1db 100644
--- a/kernel/rcu/srcutiny.c
+++ b/kernel/rcu/srcutiny.c
@@ -152,6 +152,16 @@ void srcu_drive_gp(struct work_struct *wp)
 }
 EXPORT_SYMBOL_GPL(srcu_drive_gp);
 
+static void srcu_gp_start_if_needed(struct srcu_struct *ssp)
+{
+	if (!READ_ONCE(ssp->srcu_gp_running)) {
+		if (likely(srcu_init_done))
+			schedule_work(&ssp->srcu_work);
+		else if (list_empty(&ssp->srcu_work.entry))
+			list_add(&ssp->srcu_work.entry, &srcu_boot_list);
+	}
+}
+
 /*
  * Enqueue an SRCU callback on the specified srcu_struct structure,
  * initiating grace-period processing if it is not already running.
@@ -167,12 +177,7 @@ void call_srcu(struct srcu_struct *ssp, struct rcu_head *rhp,
 	*ssp->srcu_cb_tail = rhp;
 	ssp->srcu_cb_tail = &rhp->next;
 	local_irq_restore(flags);
-	if (!READ_ONCE(ssp->srcu_gp_running)) {
-		if (likely(srcu_init_done))
-			schedule_work(&ssp->srcu_work);
-		else if (list_empty(&ssp->srcu_work.entry))
-			list_add(&ssp->srcu_work.entry, &srcu_boot_list);
-	}
+	srcu_gp_start_if_needed(ssp);
 }
 EXPORT_SYMBOL_GPL(call_srcu);
 
-- 
2.9.5

[PATCH RFC tip/core/rcu 3/5] srcu: Provide internal interface to start a Tree SRCU grace period

[paulmck]

From: "Paul E. McKenney" <paulmck@kernel.org>

There is a need for a polling interface for SRCU grace periods.
This polling needs to initiate an SRCU grace period without having
to queue (and manage) a callback.  This commit therefore splits the
Tree SRCU __call_srcu() function into callback-initialization and
queuing/start-grace-period portions, with the latter in a new function
named srcu_gp_start_if_needed().  This function may be passed a NULL
callback pointer, in which case it will refrain from queuing anything.

Why have the new function mess with queuing?  Locking considerations,
of course!

Link: https://lore.kernel.org/rcu/20201112201547.GF3365678@moria.home.lan/
Reported-by: Kent Overstreet <kent.overstreet@gmail.com>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
---
 kernel/rcu/srcutree.c | 66 +++++++++++++++++++++++++++++----------------------
 1 file changed, 37 insertions(+), 29 deletions(-)

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 79b7081..d930ece 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -808,6 +808,42 @@ static void srcu_leak_callback(struct rcu_head *rhp)
 }
 
 /*
+ * Start an SRCU grace period, and also queue the callback if non-NULL.
+ */
+static void srcu_gp_start_if_needed(struct srcu_struct *ssp, struct rcu_head *rhp, bool do_norm)
+{
+	unsigned long flags;
+	int idx;
+	bool needexp = false;
+	bool needgp = false;
+	unsigned long s;
+	struct srcu_data *sdp;
+
+	idx = srcu_read_lock(ssp);
+	sdp = raw_cpu_ptr(ssp->sda);
+	spin_lock_irqsave_rcu_node(sdp, flags);
+	rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp);
+	rcu_segcblist_advance(&sdp->srcu_cblist,
+			      rcu_seq_current(&ssp->srcu_gp_seq));
+	s = rcu_seq_snap(&ssp->srcu_gp_seq);
+	(void)rcu_segcblist_accelerate(&sdp->srcu_cblist, s);
+	if (ULONG_CMP_LT(sdp->srcu_gp_seq_needed, s)) {
+		sdp->srcu_gp_seq_needed = s;
+		needgp = true;
+	}
+	if (!do_norm && ULONG_CMP_LT(sdp->srcu_gp_seq_needed_exp, s)) {
+		sdp->srcu_gp_seq_needed_exp = s;
+		needexp = true;
+	}
+	spin_unlock_irqrestore_rcu_node(sdp, flags);
+	if (needgp)
+		srcu_funnel_gp_start(ssp, sdp, s, do_norm);
+	else if (needexp)
+		srcu_funnel_exp_start(ssp, sdp->mynode, s);
+	srcu_read_unlock(ssp, idx);
+}
+
+/*
  * Enqueue an SRCU callback on the srcu_data structure associated with
  * the current CPU and the specified srcu_struct structure, initiating
  * grace-period processing if it is not already running.
@@ -838,13 +874,6 @@ static void srcu_leak_callback(struct rcu_head *rhp)
 static void __call_srcu(struct srcu_struct *ssp, struct rcu_head *rhp,
 			rcu_callback_t func, bool do_norm)
 {
-	unsigned long flags;
-	int idx;
-	bool needexp = false;
-	bool needgp = false;
-	unsigned long s;
-	struct srcu_data *sdp;
-
 	check_init_srcu_struct(ssp);
 	if (debug_rcu_head_queue(rhp)) {
 		/* Probable double call_srcu(), so leak the callback. */
@@ -853,28 +882,7 @@ static void __call_srcu(struct srcu_struct *ssp, struct rcu_head *rhp,
 		return;
 	}
 	rhp->func = func;
-	idx = srcu_read_lock(ssp);
-	sdp = raw_cpu_ptr(ssp->sda);
-	spin_lock_irqsave_rcu_node(sdp, flags);
-	rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp);
-	rcu_segcblist_advance(&sdp->srcu_cblist,
-			      rcu_seq_current(&ssp->srcu_gp_seq));
-	s = rcu_seq_snap(&ssp->srcu_gp_seq);
-	(void)rcu_segcblist_accelerate(&sdp->srcu_cblist, s);
-	if (ULONG_CMP_LT(sdp->srcu_gp_seq_needed, s)) {
-		sdp->srcu_gp_seq_needed = s;
-		needgp = true;
-	}
-	if (!do_norm && ULONG_CMP_LT(sdp->srcu_gp_seq_needed_exp, s)) {
-		sdp->srcu_gp_seq_needed_exp = s;
-		needexp = true;
-	}
-	spin_unlock_irqrestore_rcu_node(sdp, flags);
-	if (needgp)
-		srcu_funnel_gp_start(ssp, sdp, s, do_norm);
-	else if (needexp)
-		srcu_funnel_exp_start(ssp, sdp->mynode, s);
-	srcu_read_unlock(ssp, idx);
+	srcu_gp_start_if_needed(ssp, rhp, do_norm);
 }
 
 /**
-- 
2.9.5

[PATCH RFC tip/core/rcu 4/5] srcu: Provide polling interfaces for Tiny SRCU grace periods

[paulmck]

From: "Paul E. McKenney" <paulmck@kernel.org>

There is a need for a polling interface for SRCU grace
periods, so this commit supplies get_state_synchronize_srcu(),
start_poll_synchronize_srcu(), and poll_state_synchronize_srcu() for this
purpose.  The first can be used if future grace periods are inevitable
(perhaps due to a later call_srcu() invocation), the second if future
grace periods might not otherwise happen, and the third to check if a
grace period has elapsed since the corresponding call to either of the
first two.

As with get_state_synchronize_rcu() and cond_synchronize_rcu(),
the return value from either get_state_synchronize_srcu() or
start_poll_synchronize_srcu() must be passed in to a later call to
poll_state_synchronize_srcu().

Link: https://lore.kernel.org/rcu/20201112201547.GF3365678@moria.home.lan/
Reported-by: Kent Overstreet <kent.overstreet@gmail.com>
[ paulmck: Add EXPORT_SYMBOL_GPL() per kernel test robot feedback. ]
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
---
 include/linux/rcupdate.h |  2 ++
 include/linux/srcu.h     |  3 +++
 include/linux/srcutiny.h |  1 +
 kernel/rcu/srcutiny.c    | 52 ++++++++++++++++++++++++++++++++++++++++++++++--
 4 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index de08264..e09c0d8 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -33,6 +33,8 @@
 #define ULONG_CMP_GE(a, b)	(ULONG_MAX / 2 >= (a) - (b))
 #define ULONG_CMP_LT(a, b)	(ULONG_MAX / 2 < (a) - (b))
 #define ulong2long(a)		(*(long *)(&(a)))
+#define USHORT_CMP_GE(a, b)	(USHRT_MAX / 2 >= (unsigned short)((a) - (b)))
+#define USHORT_CMP_LT(a, b)	(USHRT_MAX / 2 < (unsigned short)((a) - (b)))
 
 /* Exported common interfaces */
 void call_rcu(struct rcu_head *head, rcu_callback_t func);
diff --git a/include/linux/srcu.h b/include/linux/srcu.h
index e432cc9..a0895bb 100644
--- a/include/linux/srcu.h
+++ b/include/linux/srcu.h
@@ -60,6 +60,9 @@ void cleanup_srcu_struct(struct srcu_struct *ssp);
 int __srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp);
 void __srcu_read_unlock(struct srcu_struct *ssp, int idx) __releases(ssp);
 void synchronize_srcu(struct srcu_struct *ssp);
+unsigned long get_state_synchronize_srcu(struct srcu_struct *ssp);
+unsigned long start_poll_synchronize_srcu(struct srcu_struct *ssp);
+bool poll_state_synchronize_srcu(struct srcu_struct *ssp, unsigned long cookie);
 
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 
diff --git a/include/linux/srcutiny.h b/include/linux/srcutiny.h
index fed4a2d..e9bd6fb 100644
--- a/include/linux/srcutiny.h
+++ b/include/linux/srcutiny.h
@@ -16,6 +16,7 @@
 struct srcu_struct {
 	short srcu_lock_nesting[2];	/* srcu_read_lock() nesting depth. */
 	unsigned short srcu_idx;	/* Current reader array element in bit 0x2. */
+	unsigned short srcu_idx_max;	/* Furthest future srcu_idx request. */
 	u8 srcu_gp_running;		/* GP workqueue running? */
 	u8 srcu_gp_waiting;		/* GP waiting for readers? */
 	struct swait_queue_head srcu_wq;
diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c
index 3bac1db..b405811 100644
--- a/kernel/rcu/srcutiny.c
+++ b/kernel/rcu/srcutiny.c
@@ -34,6 +34,7 @@ static int init_srcu_struct_fields(struct srcu_struct *ssp)
 	ssp->srcu_gp_running = false;
 	ssp->srcu_gp_waiting = false;
 	ssp->srcu_idx = 0;
+	ssp->srcu_idx_max = 0;
 	INIT_WORK(&ssp->srcu_work, srcu_drive_gp);
 	INIT_LIST_HEAD(&ssp->srcu_work.entry);
 	return 0;
@@ -114,7 +115,7 @@ void srcu_drive_gp(struct work_struct *wp)
 	struct srcu_struct *ssp;
 
 	ssp = container_of(wp, struct srcu_struct, srcu_work);
-	if (ssp->srcu_gp_running || !READ_ONCE(ssp->srcu_cb_head))
+	if (ssp->srcu_gp_running || USHORT_CMP_GE(ssp->srcu_idx, READ_ONCE(ssp->srcu_idx_max)))
 		return; /* Already running or nothing to do. */
 
 	/* Remove recently arrived callbacks and wait for readers. */
@@ -147,14 +148,19 @@ void srcu_drive_gp(struct work_struct *wp)
 	 * straighten that out.
 	 */
 	WRITE_ONCE(ssp->srcu_gp_running, false);
-	if (READ_ONCE(ssp->srcu_cb_head))
+	if (USHORT_CMP_GE(ssp->srcu_idx, READ_ONCE(ssp->srcu_idx_max)))
 		schedule_work(&ssp->srcu_work);
 }
 EXPORT_SYMBOL_GPL(srcu_drive_gp);
 
 static void srcu_gp_start_if_needed(struct srcu_struct *ssp)
 {
+	unsigned short cookie;
+
 	if (!READ_ONCE(ssp->srcu_gp_running)) {
+		cookie = get_state_synchronize_srcu(ssp);
+		if (USHORT_CMP_LT(READ_ONCE(ssp->srcu_idx_max), cookie))
+			WRITE_ONCE(ssp->srcu_idx_max, cookie);
 		if (likely(srcu_init_done))
 			schedule_work(&ssp->srcu_work);
 		else if (list_empty(&ssp->srcu_work.entry))
@@ -196,6 +202,48 @@ void synchronize_srcu(struct srcu_struct *ssp)
 }
 EXPORT_SYMBOL_GPL(synchronize_srcu);
 
+/*
+ * get_state_synchronize_srcu - Provide an end-of-grace-period cookie
+ */
+unsigned long get_state_synchronize_srcu(struct srcu_struct *ssp)
+{
+	unsigned long ret;
+
+	barrier();
+	ret = (READ_ONCE(ssp->srcu_idx) + 3) & ~0x1;
+	barrier();
+	return ret & USHRT_MAX;
+}
+EXPORT_SYMBOL_GPL(get_state_synchronize_srcu);
+
+/*
+ * start_poll_synchronize_srcu - Provide cookie and start grace period
+ *
+ * The difference between this and get_state_synchronize_srcu() is that
+ * this function ensures that the poll_state_synchronize_srcu() will
+ * eventually return the value true.
+ */
+unsigned long start_poll_synchronize_srcu(struct srcu_struct *ssp)
+{
+	unsigned long ret = get_state_synchronize_srcu(ssp);
+
+	srcu_gp_start_if_needed(ssp);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(start_poll_synchronize_srcu);
+
+/*
+ * poll_state_synchronize_srcu - Has cookie's grace period ended?
+ */
+bool poll_state_synchronize_srcu(struct srcu_struct *ssp, unsigned long cookie)
+{
+	bool ret = USHORT_CMP_GE(READ_ONCE(ssp->srcu_idx), cookie);
+
+	barrier();
+	return ret;
+}
+EXPORT_SYMBOL_GPL(poll_state_synchronize_srcu);
+
 /* Lockdep diagnostics.  */
 void __init rcu_scheduler_starting(void)
 {
-- 
2.9.5

[PATCH RFC tip/core/rcu 5/5] srcu: Provide polling interfaces for Tree SRCU grace periods

[paulmck]

From: "Paul E. McKenney" <paulmck@kernel.org>

There is a need for a polling interface for SRCU grace
periods, so this commit supplies get_state_synchronize_srcu(),
start_poll_synchronize_srcu(), and poll_state_synchronize_srcu() for this
purpose.  The first can be used if future grace periods are inevitable
(perhaps due to a later call_srcu() invocation), the second if future
grace periods might not otherwise happen, and the third to check if a
grace period has elapsed since the corresponding call to either of the
first two.

As with get_state_synchronize_rcu() and cond_synchronize_rcu(),
the return value from either get_state_synchronize_srcu() or
start_poll_synchronize_srcu() must be passed in to a later call to
poll_state_synchronize_srcu().

Link: https://lore.kernel.org/rcu/20201112201547.GF3365678@moria.home.lan/
Reported-by: Kent Overstreet <kent.overstreet@gmail.com>
[ paulmck: Add EXPORT_SYMBOL_GPL() per kernel test robot feedback. ]
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
---
 kernel/rcu/srcutree.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 60 insertions(+), 3 deletions(-)

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index d930ece..015d80e 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -810,7 +810,8 @@ static void srcu_leak_callback(struct rcu_head *rhp)
 /*
  * Start an SRCU grace period, and also queue the callback if non-NULL.
  */
-static void srcu_gp_start_if_needed(struct srcu_struct *ssp, struct rcu_head *rhp, bool do_norm)
+static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
+					     struct rcu_head *rhp, bool do_norm)
 {
 	unsigned long flags;
 	int idx;
@@ -822,7 +823,8 @@ static void srcu_gp_start_if_needed(struct srcu_struct *ssp, struct rcu_head *rh
 	idx = srcu_read_lock(ssp);
 	sdp = raw_cpu_ptr(ssp->sda);
 	spin_lock_irqsave_rcu_node(sdp, flags);
-	rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp);
+	if (rhp)
+		rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp);
 	rcu_segcblist_advance(&sdp->srcu_cblist,
 			      rcu_seq_current(&ssp->srcu_gp_seq));
 	s = rcu_seq_snap(&ssp->srcu_gp_seq);
@@ -841,6 +843,7 @@ static void srcu_gp_start_if_needed(struct srcu_struct *ssp, struct rcu_head *rh
 	else if (needexp)
 		srcu_funnel_exp_start(ssp, sdp->mynode, s);
 	srcu_read_unlock(ssp, idx);
+	return s;
 }
 
 /*
@@ -882,7 +885,7 @@ static void __call_srcu(struct srcu_struct *ssp, struct rcu_head *rhp,
 		return;
 	}
 	rhp->func = func;
-	srcu_gp_start_if_needed(ssp, rhp, do_norm);
+	(void)srcu_gp_start_if_needed(ssp, rhp, do_norm);
 }
 
 /**
@@ -1011,6 +1014,60 @@ void synchronize_srcu(struct srcu_struct *ssp)
 }
 EXPORT_SYMBOL_GPL(synchronize_srcu);
 
+/**
+ * get_state_synchronize_srcu - Provide an end-of-grace-period cookie
+ * @ssp: srcu_struct to provide cookie for.
+ *
+ * This function returns a cookie that can be passed to
+ * poll_state_synchronize_srcu(), which will return true if a full grace
+ * period has elapsed in the meantime.  It is the caller's responsibility
+ * to make sure that grace period happens, for example, by invoking
+ * call_srcu() after return from get_state_synchronize_srcu().
+ */
+unsigned long get_state_synchronize_srcu(struct srcu_struct *ssp)
+{
+	// Any prior manipulation of SRCU-protected data must happen
+        // before the load from ->srcu_gp_seq.
+	smp_mb();
+	return rcu_seq_snap(&ssp->srcu_gp_seq);
+}
+EXPORT_SYMBOL_GPL(get_state_synchronize_srcu);
+
+/**
+ * start_poll_synchronize_srcu - Provide cookie and start grace period
+ * @ssp: srcu_struct to provide cookie for.
+ *
+ * This function returns a cookie that can be passed to
+ * poll_state_synchronize_srcu(), which will return true if a full grace
+ * period has elapsed in the meantime.  Unlike get_state_synchronize_srcu(),
+ * this function also ensures that any needed SRCU grace period will be
+ * started.  This convenience does come at a cost in terms of CPU overhead.
+ */
+unsigned long start_poll_synchronize_srcu(struct srcu_struct *ssp)
+{
+	return srcu_gp_start_if_needed(ssp, NULL, true);
+}
+EXPORT_SYMBOL_GPL(start_poll_synchronize_srcu);
+
+/**
+ * poll_state_synchronize_srcu - Has cookie's grace period ended?
+ * @ssp: srcu_struct to provide cookie for.
+ * @cookie: Return value from get_state_synchronize_srcu() or start_poll_synchronize_srcu().
+ *
+ * This function takes the cookie that was returned from either
+ * get_state_synchronize_srcu() or start_poll_synchronize_srcu(), and
+ * returns @true if an SRCU grace period elapsed since the time that the
+ * cookie was created.
+ */
+bool poll_state_synchronize_srcu(struct srcu_struct *ssp, unsigned long cookie)
+{
+	if (!rcu_seq_done(&ssp->srcu_gp_seq, cookie))
+		return false;
+	smp_mb(); // ^^^
+	return true;
+}
+EXPORT_SYMBOL_GPL(poll_state_synchronize_srcu);
+
 /*
  * Callback function for srcu_barrier() use.
  */
-- 
2.9.5

Re: [PATCH RFC tip/core/rcu 1/5] srcu: Make Tiny SRCU use multi-bit grace-period counter

[Neeraj Upadhyay]

Hi Paul,

On 11/17/2020 6:10 AM, paulmck@kernel.org wrote:
> From: "Paul E. McKenney" <paulmck@kernel.org>
> 
> There is a need for a polling interface for SRCU grace periods.  This
> polling needs to distinguish between an SRCU instance being idle on the
> one hand or in the middle of a grace period on the other.  This commit
> therefore converts the Tiny SRCU srcu_struct structure's srcu_idx from
> a defacto boolean to a free-running counter, using the bottom bit to
> indicate that a grace period is in progress.  The second-from-bottom
> bit is thus used as the index returned by srcu_read_lock().
> 
> Link: https://lore.kernel.org/rcu/20201112201547.GF3365678@moria.home.lan/
> Reported-by: Kent Overstreet <kent.overstreet@gmail.com>
> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> ---
>   include/linux/srcutiny.h | 4 ++--
>   kernel/rcu/srcutiny.c    | 5 +++--
>   2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/srcutiny.h b/include/linux/srcutiny.h
> index 5a5a194..fed4a2d 100644
> --- a/include/linux/srcutiny.h
> +++ b/include/linux/srcutiny.h
> @@ -15,7 +15,7 @@
>   
>   struct srcu_struct {
>   	short srcu_lock_nesting[2];	/* srcu_read_lock() nesting depth. */
> -	short srcu_idx;			/* Current reader array element. */
> +	unsigned short srcu_idx;	/* Current reader array element in bit 0x2. */
>   	u8 srcu_gp_running;		/* GP workqueue running? */
>   	u8 srcu_gp_waiting;		/* GP waiting for readers? */
>   	struct swait_queue_head srcu_wq;
> @@ -59,7 +59,7 @@ static inline int __srcu_read_lock(struct srcu_struct *ssp)
>   {
>   	int idx;
>   
> -	idx = READ_ONCE(ssp->srcu_idx);
> +	idx = (READ_ONCE(ssp->srcu_idx) & 0x2) / 2;

Should we use bit 0x2 of (READ_ONCE(ssp->srcu_idx) + 1) , if GP 
(srcu_drive_gp()) is in progress? Or am I missing something here?

idx = ((READ_ONCE(ssp->srcu_idx) +1) & 0x2) / 2;

Also, any reason for using divison instead of shift; something to
do with 16-bit srcu_idx which I am missing here?

Thanks
Neeraj

>   	WRITE_ONCE(ssp->srcu_lock_nesting[idx], ssp->srcu_lock_nesting[idx] + 1);
>   	return idx;
>   }
> diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c
> index 6208c1d..5598cf6 100644
> --- a/kernel/rcu/srcutiny.c
> +++ b/kernel/rcu/srcutiny.c
> @@ -124,11 +124,12 @@ void srcu_drive_gp(struct work_struct *wp)
>   	ssp->srcu_cb_head = NULL;
>   	ssp->srcu_cb_tail = &ssp->srcu_cb_head;
>   	local_irq_enable();
> -	idx = ssp->srcu_idx;
> -	WRITE_ONCE(ssp->srcu_idx, !ssp->srcu_idx);
> +	idx = (ssp->srcu_idx & 0x2) / 2;
> +	WRITE_ONCE(ssp->srcu_idx, ssp->srcu_idx + 1);
>   	WRITE_ONCE(ssp->srcu_gp_waiting, true);  /* srcu_read_unlock() wakes! */
>   	swait_event_exclusive(ssp->srcu_wq, !READ_ONCE(ssp->srcu_lock_nesting[idx]));
>   	WRITE_ONCE(ssp->srcu_gp_waiting, false); /* srcu_read_unlock() cheap. */
> +	WRITE_ONCE(ssp->srcu_idx, ssp->srcu_idx + 1);
>   
>   	/* Invoke the callbacks we removed above. */
>   	while (lh) {
> 

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member of the Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH RFC tip/core/rcu 1/5] srcu: Make Tiny SRCU use multi-bit grace-period counter

[Paul E. McKenney]

On Thu, Nov 19, 2020 at 01:44:49PM +0530, Neeraj Upadhyay wrote:
> Hi Paul,
> 
> On 11/17/2020 6:10 AM, paulmck@kernel.org wrote:
> > From: "Paul E. McKenney" <paulmck@kernel.org>
> > 
> > There is a need for a polling interface for SRCU grace periods.  This
> > polling needs to distinguish between an SRCU instance being idle on the
> > one hand or in the middle of a grace period on the other.  This commit
> > therefore converts the Tiny SRCU srcu_struct structure's srcu_idx from
> > a defacto boolean to a free-running counter, using the bottom bit to
> > indicate that a grace period is in progress.  The second-from-bottom
> > bit is thus used as the index returned by srcu_read_lock().
> > 
> > Link: https://lore.kernel.org/rcu/20201112201547.GF3365678@moria.home.lan/
> > Reported-by: Kent Overstreet <kent.overstreet@gmail.com>
> > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > ---
> >   include/linux/srcutiny.h | 4 ++--
> >   kernel/rcu/srcutiny.c    | 5 +++--
> >   2 files changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/linux/srcutiny.h b/include/linux/srcutiny.h
> > index 5a5a194..fed4a2d 100644
> > --- a/include/linux/srcutiny.h
> > +++ b/include/linux/srcutiny.h
> > @@ -15,7 +15,7 @@
> >   struct srcu_struct {
> >   	short srcu_lock_nesting[2];	/* srcu_read_lock() nesting depth. */
> > -	short srcu_idx;			/* Current reader array element. */
> > +	unsigned short srcu_idx;	/* Current reader array element in bit 0x2. */
> >   	u8 srcu_gp_running;		/* GP workqueue running? */
> >   	u8 srcu_gp_waiting;		/* GP waiting for readers? */
> >   	struct swait_queue_head srcu_wq;
> > @@ -59,7 +59,7 @@ static inline int __srcu_read_lock(struct srcu_struct *ssp)
> >   {
> >   	int idx;
> > -	idx = READ_ONCE(ssp->srcu_idx);
> > +	idx = (READ_ONCE(ssp->srcu_idx) & 0x2) / 2;
> 
> Should we use bit 0x2 of (READ_ONCE(ssp->srcu_idx) + 1) , if GP
> (srcu_drive_gp()) is in progress? Or am I missing something here?
> 
> idx = ((READ_ONCE(ssp->srcu_idx) +1) & 0x2) / 2;

You miss nothing!  I am running about 200 hours of concurrent rcutorture
of the SRCU-t and SRCU-u scenarios, but I must admit that this race could
be hard to hit.  But it could of course result in too-short grace periods.
I will fold this into the original with attribution.

> Also, any reason for using divison instead of shift; something to
> do with 16-bit srcu_idx which I am missing here?

I just figure that the compiler is better at selecting instructions
than I am.  Either would work.

							Thanx, Paul

> Thanks
> Neeraj
> 
> >   	WRITE_ONCE(ssp->srcu_lock_nesting[idx], ssp->srcu_lock_nesting[idx] + 1);
> >   	return idx;
> >   }
> > diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c
> > index 6208c1d..5598cf6 100644
> > --- a/kernel/rcu/srcutiny.c
> > +++ b/kernel/rcu/srcutiny.c
> > @@ -124,11 +124,12 @@ void srcu_drive_gp(struct work_struct *wp)
> >   	ssp->srcu_cb_head = NULL;
> >   	ssp->srcu_cb_tail = &ssp->srcu_cb_head;
> >   	local_irq_enable();
> > -	idx = ssp->srcu_idx;
> > -	WRITE_ONCE(ssp->srcu_idx, !ssp->srcu_idx);
> > +	idx = (ssp->srcu_idx & 0x2) / 2;
> > +	WRITE_ONCE(ssp->srcu_idx, ssp->srcu_idx + 1);
> >   	WRITE_ONCE(ssp->srcu_gp_waiting, true);  /* srcu_read_unlock() wakes! */
> >   	swait_event_exclusive(ssp->srcu_wq, !READ_ONCE(ssp->srcu_lock_nesting[idx]));
> >   	WRITE_ONCE(ssp->srcu_gp_waiting, false); /* srcu_read_unlock() cheap. */
> > +	WRITE_ONCE(ssp->srcu_idx, ssp->srcu_idx + 1);
> >   	/* Invoke the callbacks we removed above. */
> >   	while (lh) {
> > 
> 
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of
> the Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH RFC tip/core/rcu 2/5] srcu: Provide internal interface to start a Tiny SRCU grace period

[Neeraj Upadhyay]

On 11/17/2020 6:10 AM, paulmck@kernel.org wrote:
> From: "Paul E. McKenney" <paulmck@kernel.org>
> 
> There is a need for a polling interface for SRCU grace periods.
> This polling needs to initiate an SRCU grace period without
> having to queue (and manage) a callback.  This commit therefore
> splits the Tiny SRCU call_srcu() function into callback-queuing and
> start-grace-period portions, with the latter in a new function named
> srcu_gp_start_if_needed().
> 
> Link: https://lore.kernel.org/rcu/20201112201547.GF3365678@moria.home.lan/
> Reported-by: Kent Overstreet <kent.overstreet@gmail.com>
> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> ---

Reviewed-by: Neeraj Upadhyay <neeraju@codeaurora.org>


Thanks
Neeraj

>   kernel/rcu/srcutiny.c | 17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c
> index 5598cf6..3bac1db 100644
> --- a/kernel/rcu/srcutiny.c
> +++ b/kernel/rcu/srcutiny.c
> @@ -152,6 +152,16 @@ void srcu_drive_gp(struct work_struct *wp)
>   }
>   EXPORT_SYMBOL_GPL(srcu_drive_gp);
>   
> +static void srcu_gp_start_if_needed(struct srcu_struct *ssp)
> +{
> +	if (!READ_ONCE(ssp->srcu_gp_running)) {
> +		if (likely(srcu_init_done))
> +			schedule_work(&ssp->srcu_work);
> +		else if (list_empty(&ssp->srcu_work.entry))
> +			list_add(&ssp->srcu_work.entry, &srcu_boot_list);
> +	}
> +}
> +
>   /*
>    * Enqueue an SRCU callback on the specified srcu_struct structure,
>    * initiating grace-period processing if it is not already running.
> @@ -167,12 +177,7 @@ void call_srcu(struct srcu_struct *ssp, struct rcu_head *rhp,
>   	*ssp->srcu_cb_tail = rhp;
>   	ssp->srcu_cb_tail = &rhp->next;
>   	local_irq_restore(flags);
> -	if (!READ_ONCE(ssp->srcu_gp_running)) {
> -		if (likely(srcu_init_done))
> -			schedule_work(&ssp->srcu_work);
> -		else if (list_empty(&ssp->srcu_work.entry))
> -			list_add(&ssp->srcu_work.entry, &srcu_boot_list);
> -	}
> +	srcu_gp_start_if_needed(ssp);
>   }
>   EXPORT_SYMBOL_GPL(call_srcu);
>   
> 

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member of the Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH RFC tip/core/rcu 3/5] srcu: Provide internal interface to start a Tree SRCU grace period

[Neeraj Upadhyay]

On 11/17/2020 6:10 AM, paulmck@kernel.org wrote:
> From: "Paul E. McKenney" <paulmck@kernel.org>
> 
> There is a need for a polling interface for SRCU grace periods.
> This polling needs to initiate an SRCU grace period without having
> to queue (and manage) a callback.  This commit therefore splits the
> Tree SRCU __call_srcu() function into callback-initialization and
> queuing/start-grace-period portions, with the latter in a new function
> named srcu_gp_start_if_needed().  This function may be passed a NULL
> callback pointer, in which case it will refrain from queuing anything.
> 
> Why have the new function mess with queuing?  Locking considerations,
> of course!
> 
> Link: https://lore.kernel.org/rcu/20201112201547.GF3365678@moria.home.lan/
> Reported-by: Kent Overstreet <kent.overstreet@gmail.com>
> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> ---

Reviewed-by: Neeraj Upadhyay <neeraju@codeaurora.org>


Thanks
Neeraj

>   kernel/rcu/srcutree.c | 66 +++++++++++++++++++++++++++++----------------------
>   1 file changed, 37 insertions(+), 29 deletions(-)
> 
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index 79b7081..d930ece 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -808,6 +808,42 @@ static void srcu_leak_callback(struct rcu_head *rhp)
>   }
>   
>   /*
> + * Start an SRCU grace period, and also queue the callback if non-NULL.
> + */
> +static void srcu_gp_start_if_needed(struct srcu_struct *ssp, struct rcu_head *rhp, bool do_norm)
> +{
> +	unsigned long flags;
> +	int idx;
> +	bool needexp = false;
> +	bool needgp = false;
> +	unsigned long s;
> +	struct srcu_data *sdp;
> +
> +	idx = srcu_read_lock(ssp);
> +	sdp = raw_cpu_ptr(ssp->sda);
> +	spin_lock_irqsave_rcu_node(sdp, flags);
> +	rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp);
> +	rcu_segcblist_advance(&sdp->srcu_cblist,
> +			      rcu_seq_current(&ssp->srcu_gp_seq));
> +	s = rcu_seq_snap(&ssp->srcu_gp_seq);
> +	(void)rcu_segcblist_accelerate(&sdp->srcu_cblist, s);
> +	if (ULONG_CMP_LT(sdp->srcu_gp_seq_needed, s)) {
> +		sdp->srcu_gp_seq_needed = s;
> +		needgp = true;
> +	}
> +	if (!do_norm && ULONG_CMP_LT(sdp->srcu_gp_seq_needed_exp, s)) {
> +		sdp->srcu_gp_seq_needed_exp = s;
> +		needexp = true;
> +	}
> +	spin_unlock_irqrestore_rcu_node(sdp, flags);
> +	if (needgp)
> +		srcu_funnel_gp_start(ssp, sdp, s, do_norm);
> +	else if (needexp)
> +		srcu_funnel_exp_start(ssp, sdp->mynode, s);
> +	srcu_read_unlock(ssp, idx);
> +}
> +
> +/*
>    * Enqueue an SRCU callback on the srcu_data structure associated with
>    * the current CPU and the specified srcu_struct structure, initiating
>    * grace-period processing if it is not already running.
> @@ -838,13 +874,6 @@ static void srcu_leak_callback(struct rcu_head *rhp)
>   static void __call_srcu(struct srcu_struct *ssp, struct rcu_head *rhp,
>   			rcu_callback_t func, bool do_norm)
>   {
> -	unsigned long flags;
> -	int idx;
> -	bool needexp = false;
> -	bool needgp = false;
> -	unsigned long s;
> -	struct srcu_data *sdp;
> -
>   	check_init_srcu_struct(ssp);
>   	if (debug_rcu_head_queue(rhp)) {
>   		/* Probable double call_srcu(), so leak the callback. */
> @@ -853,28 +882,7 @@ static void __call_srcu(struct srcu_struct *ssp, struct rcu_head *rhp,
>   		return;
>   	}
>   	rhp->func = func;
> -	idx = srcu_read_lock(ssp);
> -	sdp = raw_cpu_ptr(ssp->sda);
> -	spin_lock_irqsave_rcu_node(sdp, flags);
> -	rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp);
> -	rcu_segcblist_advance(&sdp->srcu_cblist,
> -			      rcu_seq_current(&ssp->srcu_gp_seq));
> -	s = rcu_seq_snap(&ssp->srcu_gp_seq);
> -	(void)rcu_segcblist_accelerate(&sdp->srcu_cblist, s);
> -	if (ULONG_CMP_LT(sdp->srcu_gp_seq_needed, s)) {
> -		sdp->srcu_gp_seq_needed = s;
> -		needgp = true;
> -	}
> -	if (!do_norm && ULONG_CMP_LT(sdp->srcu_gp_seq_needed_exp, s)) {
> -		sdp->srcu_gp_seq_needed_exp = s;
> -		needexp = true;
> -	}
> -	spin_unlock_irqrestore_rcu_node(sdp, flags);
> -	if (needgp)
> -		srcu_funnel_gp_start(ssp, sdp, s, do_norm);
> -	else if (needexp)
> -		srcu_funnel_exp_start(ssp, sdp->mynode, s);
> -	srcu_read_unlock(ssp, idx);
> +	srcu_gp_start_if_needed(ssp, rhp, do_norm);
>   }
>   
>   /**
> 

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member of the Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH RFC tip/core/rcu 4/5] srcu: Provide polling interfaces for Tiny SRCU grace periods

[Neeraj Upadhyay]

Hi Paul,

On 11/17/2020 6:10 AM, paulmck@kernel.org wrote:
> From: "Paul E. McKenney" <paulmck@kernel.org>
> 
> There is a need for a polling interface for SRCU grace
> periods, so this commit supplies get_state_synchronize_srcu(),
> start_poll_synchronize_srcu(), and poll_state_synchronize_srcu() for this
> purpose.  The first can be used if future grace periods are inevitable
> (perhaps due to a later call_srcu() invocation), the second if future
> grace periods might not otherwise happen, and the third to check if a
> grace period has elapsed since the corresponding call to either of the
> first two.
> 
> As with get_state_synchronize_rcu() and cond_synchronize_rcu(),
> the return value from either get_state_synchronize_srcu() or
> start_poll_synchronize_srcu() must be passed in to a later call to
> poll_state_synchronize_srcu().
> 
> Link: https://lore.kernel.org/rcu/20201112201547.GF3365678@moria.home.lan/
> Reported-by: Kent Overstreet <kent.overstreet@gmail.com>
> [ paulmck: Add EXPORT_SYMBOL_GPL() per kernel test robot feedback. ]
> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> ---
>   include/linux/rcupdate.h |  2 ++
>   include/linux/srcu.h     |  3 +++
>   include/linux/srcutiny.h |  1 +
>   kernel/rcu/srcutiny.c    | 52 ++++++++++++++++++++++++++++++++++++++++++++++--
>   4 files changed, 56 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index de08264..e09c0d8 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -33,6 +33,8 @@
>   #define ULONG_CMP_GE(a, b)	(ULONG_MAX / 2 >= (a) - (b))
>   #define ULONG_CMP_LT(a, b)	(ULONG_MAX / 2 < (a) - (b))
>   #define ulong2long(a)		(*(long *)(&(a)))
> +#define USHORT_CMP_GE(a, b)	(USHRT_MAX / 2 >= (unsigned short)((a) - (b)))
> +#define USHORT_CMP_LT(a, b)	(USHRT_MAX / 2 < (unsigned short)((a) - (b)))
>   
>   /* Exported common interfaces */
>   void call_rcu(struct rcu_head *head, rcu_callback_t func);
> diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> index e432cc9..a0895bb 100644
> --- a/include/linux/srcu.h
> +++ b/include/linux/srcu.h
> @@ -60,6 +60,9 @@ void cleanup_srcu_struct(struct srcu_struct *ssp);
>   int __srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp);
>   void __srcu_read_unlock(struct srcu_struct *ssp, int idx) __releases(ssp);
>   void synchronize_srcu(struct srcu_struct *ssp);
> +unsigned long get_state_synchronize_srcu(struct srcu_struct *ssp);
> +unsigned long start_poll_synchronize_srcu(struct srcu_struct *ssp);
> +bool poll_state_synchronize_srcu(struct srcu_struct *ssp, unsigned long cookie);
>   
>   #ifdef CONFIG_DEBUG_LOCK_ALLOC
>   
> diff --git a/include/linux/srcutiny.h b/include/linux/srcutiny.h
> index fed4a2d..e9bd6fb 100644
> --- a/include/linux/srcutiny.h
> +++ b/include/linux/srcutiny.h
> @@ -16,6 +16,7 @@
>   struct srcu_struct {
>   	short srcu_lock_nesting[2];	/* srcu_read_lock() nesting depth. */
>   	unsigned short srcu_idx;	/* Current reader array element in bit 0x2. */
> +	unsigned short srcu_idx_max;	/* Furthest future srcu_idx request. */
>   	u8 srcu_gp_running;		/* GP workqueue running? */
>   	u8 srcu_gp_waiting;		/* GP waiting for readers? */
>   	struct swait_queue_head srcu_wq;
> diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c
> index 3bac1db..b405811 100644
> --- a/kernel/rcu/srcutiny.c
> +++ b/kernel/rcu/srcutiny.c
> @@ -34,6 +34,7 @@ static int init_srcu_struct_fields(struct srcu_struct *ssp)
>   	ssp->srcu_gp_running = false;
>   	ssp->srcu_gp_waiting = false;
>   	ssp->srcu_idx = 0;
> +	ssp->srcu_idx_max = 0;
>   	INIT_WORK(&ssp->srcu_work, srcu_drive_gp);
>   	INIT_LIST_HEAD(&ssp->srcu_work.entry);
>   	return 0;
> @@ -114,7 +115,7 @@ void srcu_drive_gp(struct work_struct *wp)
>   	struct srcu_struct *ssp;
>   
>   	ssp = container_of(wp, struct srcu_struct, srcu_work);
> -	if (ssp->srcu_gp_running || !READ_ONCE(ssp->srcu_cb_head))
> +	if (ssp->srcu_gp_running || USHORT_CMP_GE(ssp->srcu_idx, READ_ONCE(ssp->srcu_idx_max)))
>   		return; /* Already running or nothing to do. */
>   
>   	/* Remove recently arrived callbacks and wait for readers. */
> @@ -147,14 +148,19 @@ void srcu_drive_gp(struct work_struct *wp)
>   	 * straighten that out.
>   	 */
>   	WRITE_ONCE(ssp->srcu_gp_running, false);
> -	if (READ_ONCE(ssp->srcu_cb_head))
> +	if (USHORT_CMP_GE(ssp->srcu_idx, READ_ONCE(ssp->srcu_idx_max)))

Should this be USHORT_CMP_LT ?

>   		schedule_work(&ssp->srcu_work);
>   }
>   EXPORT_SYMBOL_GPL(srcu_drive_gp);
>   
>   static void srcu_gp_start_if_needed(struct srcu_struct *ssp)
>   {
> +	unsigned short cookie;
> +
>   	if (!READ_ONCE(ssp->srcu_gp_running)) {
> +		cookie = get_state_synchronize_srcu(ssp);
> +		if (USHORT_CMP_LT(READ_ONCE(ssp->srcu_idx_max), cookie))
> +			WRITE_ONCE(ssp->srcu_idx_max, cookie);

I was thinking of a case which might break with this.

Consider a scenario, where GP is in progress and kworker is right
before below point, after executing callbacks:

void srcu_drive_gp(struct work_struct *wp) {
   <snip>

   while (lh) {
   <cb execution loop>
   }
   >>> CURRENT EXECUTION POINT

   WRITE_ONCE(ssp->srcu_gp_running, false);

   if (USHORT_CMP_LT(ssp->srcu_idx, READ_ONCE(ssp->srcu_idx_max)))
     schedule_work(&ssp->srcu_work);
}

Now, at this instance, srcu_gp_start_if_needed() runs and samples 
srcu_gp_running and returns, without updating srcu_idx_max

static void srcu_gp_start_if_needed(struct srcu_struct *ssp)
{
   if (!READ_ONCE(ssp->srcu_gp_running)) returns true
   <snip>
}

kworker running srcu_drive_gp() resumes and returns without queueing a 
new schedule_work(&ssp->srcu_work); for new GP?

Prior to this patch, call_srcu() enqueues a cb before entering 
srcu_gp_start_if_needed(), and srcu_drive_gp() observes this
queuing, and schedule a work for the new GP, for this scenario.


  	WRITE_ONCE(ssp->srcu_gp_running, false);
-	if (READ_ONCE(ssp->srcu_cb_head))
+	if (USHORT_CMP_GE(ssp->srcu_idx, READ_ONCE(ssp->srcu_idx_max)))
  		schedule_work(&ssp->srcu_work);

So, should the "cookie" calculation and "srcu_idx_max" setting be moved 
outside of ssp->srcu_gp_running check and maybe return the same cookie 
to caller and use that as the returned cookie from 
start_poll_synchronize_srcu() ?

srcu_gp_start_if_needed()
   cookie = get_state_synchronize_srcu(ssp);
   if (USHORT_CMP_LT(READ_ONCE(ssp->srcu_idx_max), cookie))
      WRITE_ONCE(ssp->srcu_idx_max, cookie);
   if (!READ_ONCE(ssp->srcu_gp_running)) {
   <snip>
}


Thanks
Neeraj

>   		if (likely(srcu_init_done))
>   			schedule_work(&ssp->srcu_work);
>   		else if (list_empty(&ssp->srcu_work.entry))
> @@ -196,6 +202,48 @@ void synchronize_srcu(struct srcu_struct *ssp)
>   }
>   EXPORT_SYMBOL_GPL(synchronize_srcu);
>   
> +/*
> + * get_state_synchronize_srcu - Provide an end-of-grace-period cookie
> + */
> +unsigned long get_state_synchronize_srcu(struct srcu_struct *ssp)
> +{
> +	unsigned long ret;
> +
> +	barrier();
> +	ret = (READ_ONCE(ssp->srcu_idx) + 3) & ~0x1;
> +	barrier();
> +	return ret & USHRT_MAX;
> +}
> +EXPORT_SYMBOL_GPL(get_state_synchronize_srcu);
> +
> +/*
> + * start_poll_synchronize_srcu - Provide cookie and start grace period
> + *
> + * The difference between this and get_state_synchronize_srcu() is that
> + * this function ensures that the poll_state_synchronize_srcu() will
> + * eventually return the value true.
> + */
> +unsigned long start_poll_synchronize_srcu(struct srcu_struct *ssp)
> +{
> +	unsigned long ret = get_state_synchronize_srcu(ssp);
> +
> +	srcu_gp_start_if_needed(ssp);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(start_poll_synchronize_srcu);
> +
> +/*
> + * poll_state_synchronize_srcu - Has cookie's grace period ended?
> + */
> +bool poll_state_synchronize_srcu(struct srcu_struct *ssp, unsigned long cookie)
> +{
> +	bool ret = USHORT_CMP_GE(READ_ONCE(ssp->srcu_idx), cookie);
> +
> +	barrier();
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(poll_state_synchronize_srcu);
> +
>   /* Lockdep diagnostics.  */
>   void __init rcu_scheduler_starting(void)
>   {
> 

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member of the Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH RFC tip/core/rcu 5/5] srcu: Provide polling interfaces for Tree SRCU grace periods

[Neeraj Upadhyay]

On 11/17/2020 6:10 AM, paulmck@kernel.org wrote:
> From: "Paul E. McKenney" <paulmck@kernel.org>
> 
> There is a need for a polling interface for SRCU grace
> periods, so this commit supplies get_state_synchronize_srcu(),
> start_poll_synchronize_srcu(), and poll_state_synchronize_srcu() for this
> purpose.  The first can be used if future grace periods are inevitable
> (perhaps due to a later call_srcu() invocation), the second if future
> grace periods might not otherwise happen, and the third to check if a
> grace period has elapsed since the corresponding call to either of the
> first two.
> 
> As with get_state_synchronize_rcu() and cond_synchronize_rcu(),
> the return value from either get_state_synchronize_srcu() or
> start_poll_synchronize_srcu() must be passed in to a later call to
> poll_state_synchronize_srcu().
> 
> Link: https://lore.kernel.org/rcu/20201112201547.GF3365678@moria.home.lan/
> Reported-by: Kent Overstreet <kent.overstreet@gmail.com>
> [ paulmck: Add EXPORT_SYMBOL_GPL() per kernel test robot feedback. ]
> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> ---
>   kernel/rcu/srcutree.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 60 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index d930ece..015d80e 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -810,7 +810,8 @@ static void srcu_leak_callback(struct rcu_head *rhp)
>   /*
>    * Start an SRCU grace period, and also queue the callback if non-NULL.
>    */
> -static void srcu_gp_start_if_needed(struct srcu_struct *ssp, struct rcu_head *rhp, bool do_norm)
> +static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
> +					     struct rcu_head *rhp, bool do_norm)
>   {
>   	unsigned long flags;
>   	int idx;
> @@ -822,7 +823,8 @@ static void srcu_gp_start_if_needed(struct srcu_struct *ssp, struct rcu_head *rh
>   	idx = srcu_read_lock(ssp);
>   	sdp = raw_cpu_ptr(ssp->sda);
>   	spin_lock_irqsave_rcu_node(sdp, flags);
> -	rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp);
> +	if (rhp)
> +		rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp);
>   	rcu_segcblist_advance(&sdp->srcu_cblist,
>   			      rcu_seq_current(&ssp->srcu_gp_seq));
>   	s = rcu_seq_snap(&ssp->srcu_gp_seq);
> @@ -841,6 +843,7 @@ static void srcu_gp_start_if_needed(struct srcu_struct *ssp, struct rcu_head *rh
>   	else if (needexp)
>   		srcu_funnel_exp_start(ssp, sdp->mynode, s);
>   	srcu_read_unlock(ssp, idx);
> +	return s;
>   }
>   
>   /*
> @@ -882,7 +885,7 @@ static void __call_srcu(struct srcu_struct *ssp, struct rcu_head *rhp,
>   		return;
>   	}
>   	rhp->func = func;
> -	srcu_gp_start_if_needed(ssp, rhp, do_norm);
> +	(void)srcu_gp_start_if_needed(ssp, rhp, do_norm);
>   }
>   
>   /**
> @@ -1011,6 +1014,60 @@ void synchronize_srcu(struct srcu_struct *ssp)
>   }
>   EXPORT_SYMBOL_GPL(synchronize_srcu);
>   
> +/**
> + * get_state_synchronize_srcu - Provide an end-of-grace-period cookie
> + * @ssp: srcu_struct to provide cookie for.
> + *
> + * This function returns a cookie that can be passed to
> + * poll_state_synchronize_srcu(), which will return true if a full grace
> + * period has elapsed in the meantime.  It is the caller's responsibility
> + * to make sure that grace period happens, for example, by invoking
> + * call_srcu() after return from get_state_synchronize_srcu().
> + */
> +unsigned long get_state_synchronize_srcu(struct srcu_struct *ssp)
> +{
> +	// Any prior manipulation of SRCU-protected data must happen
> +        // before the load from ->srcu_gp_seq.
> +	smp_mb();
> +	return rcu_seq_snap(&ssp->srcu_gp_seq);
> +}
> +EXPORT_SYMBOL_GPL(get_state_synchronize_srcu);
> +
> +/**
> + * start_poll_synchronize_srcu - Provide cookie and start grace period
> + * @ssp: srcu_struct to provide cookie for.
> + *
> + * This function returns a cookie that can be passed to
> + * poll_state_synchronize_srcu(), which will return true if a full grace
> + * period has elapsed in the meantime.  Unlike get_state_synchronize_srcu(),
> + * this function also ensures that any needed SRCU grace period will be
> + * started.  This convenience does come at a cost in terms of CPU overhead.
> + */
> +unsigned long start_poll_synchronize_srcu(struct srcu_struct *ssp)
> +{
> +	return srcu_gp_start_if_needed(ssp, NULL, true);
> +}
> +EXPORT_SYMBOL_GPL(start_poll_synchronize_srcu);
> +
> +/**
> + * poll_state_synchronize_srcu - Has cookie's grace period ended?
> + * @ssp: srcu_struct to provide cookie for.
> + * @cookie: Return value from get_state_synchronize_srcu() or start_poll_synchronize_srcu().
> + *
> + * This function takes the cookie that was returned from either
> + * get_state_synchronize_srcu() or start_poll_synchronize_srcu(), and
> + * returns @true if an SRCU grace period elapsed since the time that the
> + * cookie was created.
> + */
> +bool poll_state_synchronize_srcu(struct srcu_struct *ssp, unsigned long cookie)
> +{
> +	if (!rcu_seq_done(&ssp->srcu_gp_seq, cookie))
> +		return false;
> +	smp_mb(); // ^^^

Minor: Should this comment be more descriptive?



Thanks
Neeraj

> +	return true;
> +}
> +EXPORT_SYMBOL_GPL(poll_state_synchronize_srcu);
> +
>   /*
>    * Callback function for srcu_barrier() use.
>    */
> 

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member of the Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH RFC tip/core/rcu 4/5] srcu: Provide polling interfaces for Tiny SRCU grace periods

[Paul E. McKenney]

On Fri, Nov 20, 2020 at 05:28:32PM +0530, Neeraj Upadhyay wrote:
> Hi Paul,
> 
> On 11/17/2020 6:10 AM, paulmck@kernel.org wrote:
> > From: "Paul E. McKenney" <paulmck@kernel.org>
> > 
> > There is a need for a polling interface for SRCU grace
> > periods, so this commit supplies get_state_synchronize_srcu(),
> > start_poll_synchronize_srcu(), and poll_state_synchronize_srcu() for this
> > purpose.  The first can be used if future grace periods are inevitable
> > (perhaps due to a later call_srcu() invocation), the second if future
> > grace periods might not otherwise happen, and the third to check if a
> > grace period has elapsed since the corresponding call to either of the
> > first two.
> > 
> > As with get_state_synchronize_rcu() and cond_synchronize_rcu(),
> > the return value from either get_state_synchronize_srcu() or
> > start_poll_synchronize_srcu() must be passed in to a later call to
> > poll_state_synchronize_srcu().
> > 
> > Link: https://lore.kernel.org/rcu/20201112201547.GF3365678@moria.home.lan/
> > Reported-by: Kent Overstreet <kent.overstreet@gmail.com>
> > [ paulmck: Add EXPORT_SYMBOL_GPL() per kernel test robot feedback. ]
> > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > ---
> >   include/linux/rcupdate.h |  2 ++
> >   include/linux/srcu.h     |  3 +++
> >   include/linux/srcutiny.h |  1 +
> >   kernel/rcu/srcutiny.c    | 52 ++++++++++++++++++++++++++++++++++++++++++++++--
> >   4 files changed, 56 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > index de08264..e09c0d8 100644
> > --- a/include/linux/rcupdate.h
> > +++ b/include/linux/rcupdate.h
> > @@ -33,6 +33,8 @@
> >   #define ULONG_CMP_GE(a, b)	(ULONG_MAX / 2 >= (a) - (b))
> >   #define ULONG_CMP_LT(a, b)	(ULONG_MAX / 2 < (a) - (b))
> >   #define ulong2long(a)		(*(long *)(&(a)))
> > +#define USHORT_CMP_GE(a, b)	(USHRT_MAX / 2 >= (unsigned short)((a) - (b)))
> > +#define USHORT_CMP_LT(a, b)	(USHRT_MAX / 2 < (unsigned short)((a) - (b)))
> >   /* Exported common interfaces */
> >   void call_rcu(struct rcu_head *head, rcu_callback_t func);
> > diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> > index e432cc9..a0895bb 100644
> > --- a/include/linux/srcu.h
> > +++ b/include/linux/srcu.h
> > @@ -60,6 +60,9 @@ void cleanup_srcu_struct(struct srcu_struct *ssp);
> >   int __srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp);
> >   void __srcu_read_unlock(struct srcu_struct *ssp, int idx) __releases(ssp);
> >   void synchronize_srcu(struct srcu_struct *ssp);
> > +unsigned long get_state_synchronize_srcu(struct srcu_struct *ssp);
> > +unsigned long start_poll_synchronize_srcu(struct srcu_struct *ssp);
> > +bool poll_state_synchronize_srcu(struct srcu_struct *ssp, unsigned long cookie);
> >   #ifdef CONFIG_DEBUG_LOCK_ALLOC
> > diff --git a/include/linux/srcutiny.h b/include/linux/srcutiny.h
> > index fed4a2d..e9bd6fb 100644
> > --- a/include/linux/srcutiny.h
> > +++ b/include/linux/srcutiny.h
> > @@ -16,6 +16,7 @@
> >   struct srcu_struct {
> >   	short srcu_lock_nesting[2];	/* srcu_read_lock() nesting depth. */
> >   	unsigned short srcu_idx;	/* Current reader array element in bit 0x2. */
> > +	unsigned short srcu_idx_max;	/* Furthest future srcu_idx request. */
> >   	u8 srcu_gp_running;		/* GP workqueue running? */
> >   	u8 srcu_gp_waiting;		/* GP waiting for readers? */
> >   	struct swait_queue_head srcu_wq;
> > diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c
> > index 3bac1db..b405811 100644
> > --- a/kernel/rcu/srcutiny.c
> > +++ b/kernel/rcu/srcutiny.c
> > @@ -34,6 +34,7 @@ static int init_srcu_struct_fields(struct srcu_struct *ssp)
> >   	ssp->srcu_gp_running = false;
> >   	ssp->srcu_gp_waiting = false;
> >   	ssp->srcu_idx = 0;
> > +	ssp->srcu_idx_max = 0;
> >   	INIT_WORK(&ssp->srcu_work, srcu_drive_gp);
> >   	INIT_LIST_HEAD(&ssp->srcu_work.entry);
> >   	return 0;
> > @@ -114,7 +115,7 @@ void srcu_drive_gp(struct work_struct *wp)
> >   	struct srcu_struct *ssp;
> >   	ssp = container_of(wp, struct srcu_struct, srcu_work);
> > -	if (ssp->srcu_gp_running || !READ_ONCE(ssp->srcu_cb_head))
> > +	if (ssp->srcu_gp_running || USHORT_CMP_GE(ssp->srcu_idx, READ_ONCE(ssp->srcu_idx_max)))
> >   		return; /* Already running or nothing to do. */
> >   	/* Remove recently arrived callbacks and wait for readers. */
> > @@ -147,14 +148,19 @@ void srcu_drive_gp(struct work_struct *wp)
> >   	 * straighten that out.
> >   	 */
> >   	WRITE_ONCE(ssp->srcu_gp_running, false);
> > -	if (READ_ONCE(ssp->srcu_cb_head))
> > +	if (USHORT_CMP_GE(ssp->srcu_idx, READ_ONCE(ssp->srcu_idx_max)))
> 
> Should this be USHORT_CMP_LT ?

I believe that you are correct.  As is, it works but does needless
grace periods.

> >   		schedule_work(&ssp->srcu_work);
> >   }
> >   EXPORT_SYMBOL_GPL(srcu_drive_gp);
> >   static void srcu_gp_start_if_needed(struct srcu_struct *ssp)
> >   {
> > +	unsigned short cookie;
> > +
> >   	if (!READ_ONCE(ssp->srcu_gp_running)) {
> > +		cookie = get_state_synchronize_srcu(ssp);
> > +		if (USHORT_CMP_LT(READ_ONCE(ssp->srcu_idx_max), cookie))
> > +			WRITE_ONCE(ssp->srcu_idx_max, cookie);
> 
> I was thinking of a case which might break with this.
> 
> Consider a scenario, where GP is in progress and kworker is right
> before below point, after executing callbacks:
> 
> void srcu_drive_gp(struct work_struct *wp) {
>   <snip>

We updated ->srcu_idx up here, correct?  So it has bottom bit zero.

>   while (lh) {
>   <cb execution loop>
>   }
>   >>> CURRENT EXECUTION POINT

Keeping in mind that Tiny SRCU always runs !PREEMPT, this must be
due to an interrupt.

>   WRITE_ONCE(ssp->srcu_gp_running, false);
> 
>   if (USHORT_CMP_LT(ssp->srcu_idx, READ_ONCE(ssp->srcu_idx_max)))
>     schedule_work(&ssp->srcu_work);
> }
> 
> Now, at this instance, srcu_gp_start_if_needed() runs and samples
> srcu_gp_running and returns, without updating srcu_idx_max
> 
> static void srcu_gp_start_if_needed(struct srcu_struct *ssp)
> {
>   if (!READ_ONCE(ssp->srcu_gp_running)) returns true
>   <snip>
> }

This could happen in an interrupt handler, so with you thus far.

> kworker running srcu_drive_gp() resumes and returns without queueing a new
> schedule_work(&ssp->srcu_work); for new GP?
> 
> Prior to this patch, call_srcu() enqueues a cb before entering
> srcu_gp_start_if_needed(), and srcu_drive_gp() observes this
> queuing, and schedule a work for the new GP, for this scenario.
> 
> 
>  	WRITE_ONCE(ssp->srcu_gp_running, false);
> -	if (READ_ONCE(ssp->srcu_cb_head))
> +	if (USHORT_CMP_GE(ssp->srcu_idx, READ_ONCE(ssp->srcu_idx_max)))
>  		schedule_work(&ssp->srcu_work);
> 
> So, should the "cookie" calculation and "srcu_idx_max" setting be moved
> outside of ssp->srcu_gp_running check and maybe return the same cookie to
> caller and use that as the returned cookie from
> start_poll_synchronize_srcu() ?
> 
> srcu_gp_start_if_needed()
>   cookie = get_state_synchronize_srcu(ssp);
>   if (USHORT_CMP_LT(READ_ONCE(ssp->srcu_idx_max), cookie))
>      WRITE_ONCE(ssp->srcu_idx_max, cookie);
>   if (!READ_ONCE(ssp->srcu_gp_running)) {
>   <snip>
> }

I believe that you are quite correct, thank you!

But rcutorture does have a call_srcu() (really a ->call, but same if SRCU)
in a timer handler.  The race window is quite narrow, so testing it might
be a challenge...

This is what I end up with:

	static void srcu_gp_start_if_needed(struct srcu_struct *ssp)
	{
		unsigned short cookie;

		cookie = get_state_synchronize_srcu(ssp);
		if (USHORT_CMP_LT(READ_ONCE(ssp->srcu_idx_max), cookie))
			WRITE_ONCE(ssp->srcu_idx_max, cookie);
		if (!READ_ONCE(ssp->srcu_gp_running)) {
			if (likely(srcu_init_done))
				schedule_work(&ssp->srcu_work);
			else if (list_empty(&ssp->srcu_work.entry))
				list_add(&ssp->srcu_work.entry, &srcu_boot_list);
		}
	}

Does that look plausible?

							Thanx, Paul

Re: [PATCH RFC tip/core/rcu 5/5] srcu: Provide polling interfaces for Tree SRCU grace periods

[Paul E. McKenney]

On Fri, Nov 20, 2020 at 05:31:43PM +0530, Neeraj Upadhyay wrote:
> 
> 
> On 11/17/2020 6:10 AM, paulmck@kernel.org wrote:
> > From: "Paul E. McKenney" <paulmck@kernel.org>
> > 
> > There is a need for a polling interface for SRCU grace
> > periods, so this commit supplies get_state_synchronize_srcu(),
> > start_poll_synchronize_srcu(), and poll_state_synchronize_srcu() for this
> > purpose.  The first can be used if future grace periods are inevitable
> > (perhaps due to a later call_srcu() invocation), the second if future
> > grace periods might not otherwise happen, and the third to check if a
> > grace period has elapsed since the corresponding call to either of the
> > first two.
> > 
> > As with get_state_synchronize_rcu() and cond_synchronize_rcu(),
> > the return value from either get_state_synchronize_srcu() or
> > start_poll_synchronize_srcu() must be passed in to a later call to
> > poll_state_synchronize_srcu().
> > 
> > Link: https://lore.kernel.org/rcu/20201112201547.GF3365678@moria.home.lan/
> > Reported-by: Kent Overstreet <kent.overstreet@gmail.com>
> > [ paulmck: Add EXPORT_SYMBOL_GPL() per kernel test robot feedback. ]
> > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > ---
> >   kernel/rcu/srcutree.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++---
> >   1 file changed, 60 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > index d930ece..015d80e 100644
> > --- a/kernel/rcu/srcutree.c
> > +++ b/kernel/rcu/srcutree.c
> > @@ -810,7 +810,8 @@ static void srcu_leak_callback(struct rcu_head *rhp)
> >   /*
> >    * Start an SRCU grace period, and also queue the callback if non-NULL.
> >    */
> > -static void srcu_gp_start_if_needed(struct srcu_struct *ssp, struct rcu_head *rhp, bool do_norm)
> > +static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
> > +					     struct rcu_head *rhp, bool do_norm)
> >   {
> >   	unsigned long flags;
> >   	int idx;
> > @@ -822,7 +823,8 @@ static void srcu_gp_start_if_needed(struct srcu_struct *ssp, struct rcu_head *rh
> >   	idx = srcu_read_lock(ssp);
> >   	sdp = raw_cpu_ptr(ssp->sda);
> >   	spin_lock_irqsave_rcu_node(sdp, flags);
> > -	rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp);
> > +	if (rhp)
> > +		rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp);
> >   	rcu_segcblist_advance(&sdp->srcu_cblist,
> >   			      rcu_seq_current(&ssp->srcu_gp_seq));
> >   	s = rcu_seq_snap(&ssp->srcu_gp_seq);
> > @@ -841,6 +843,7 @@ static void srcu_gp_start_if_needed(struct srcu_struct *ssp, struct rcu_head *rh
> >   	else if (needexp)
> >   		srcu_funnel_exp_start(ssp, sdp->mynode, s);
> >   	srcu_read_unlock(ssp, idx);
> > +	return s;
> >   }
> >   /*
> > @@ -882,7 +885,7 @@ static void __call_srcu(struct srcu_struct *ssp, struct rcu_head *rhp,
> >   		return;
> >   	}
> >   	rhp->func = func;
> > -	srcu_gp_start_if_needed(ssp, rhp, do_norm);
> > +	(void)srcu_gp_start_if_needed(ssp, rhp, do_norm);
> >   }
> >   /**
> > @@ -1011,6 +1014,60 @@ void synchronize_srcu(struct srcu_struct *ssp)
> >   }
> >   EXPORT_SYMBOL_GPL(synchronize_srcu);
> > +/**
> > + * get_state_synchronize_srcu - Provide an end-of-grace-period cookie
> > + * @ssp: srcu_struct to provide cookie for.
> > + *
> > + * This function returns a cookie that can be passed to
> > + * poll_state_synchronize_srcu(), which will return true if a full grace
> > + * period has elapsed in the meantime.  It is the caller's responsibility
> > + * to make sure that grace period happens, for example, by invoking
> > + * call_srcu() after return from get_state_synchronize_srcu().
> > + */
> > +unsigned long get_state_synchronize_srcu(struct srcu_struct *ssp)
> > +{
> > +	// Any prior manipulation of SRCU-protected data must happen
> > +        // before the load from ->srcu_gp_seq.
> > +	smp_mb();
> > +	return rcu_seq_snap(&ssp->srcu_gp_seq);
> > +}
> > +EXPORT_SYMBOL_GPL(get_state_synchronize_srcu);
> > +
> > +/**
> > + * start_poll_synchronize_srcu - Provide cookie and start grace period
> > + * @ssp: srcu_struct to provide cookie for.
> > + *
> > + * This function returns a cookie that can be passed to
> > + * poll_state_synchronize_srcu(), which will return true if a full grace
> > + * period has elapsed in the meantime.  Unlike get_state_synchronize_srcu(),
> > + * this function also ensures that any needed SRCU grace period will be
> > + * started.  This convenience does come at a cost in terms of CPU overhead.
> > + */
> > +unsigned long start_poll_synchronize_srcu(struct srcu_struct *ssp)
> > +{
> > +	return srcu_gp_start_if_needed(ssp, NULL, true);
> > +}
> > +EXPORT_SYMBOL_GPL(start_poll_synchronize_srcu);
> > +
> > +/**
> > + * poll_state_synchronize_srcu - Has cookie's grace period ended?
> > + * @ssp: srcu_struct to provide cookie for.
> > + * @cookie: Return value from get_state_synchronize_srcu() or start_poll_synchronize_srcu().
> > + *
> > + * This function takes the cookie that was returned from either
> > + * get_state_synchronize_srcu() or start_poll_synchronize_srcu(), and
> > + * returns @true if an SRCU grace period elapsed since the time that the
> > + * cookie was created.
> > + */
> > +bool poll_state_synchronize_srcu(struct srcu_struct *ssp, unsigned long cookie)
> > +{
> > +	if (!rcu_seq_done(&ssp->srcu_gp_seq, cookie))
> > +		return false;
> > +	smp_mb(); // ^^^
> 
> Minor: Should this comment be more descriptive?

You have to read between the lines?  ;-)

How about like this?

							Thanx, Paul

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

bool poll_state_synchronize_srcu(struct srcu_struct *ssp, unsigned long cookie)
{
	if (!rcu_seq_done(&ssp->srcu_gp_seq, cookie))
		return false;
	// Ensure that the end of the SRCU grace period happens before
	// any subsequent code that the caller might execute.
	smp_mb(); // ^^^
	return true;
}

Re: [PATCH RFC tip/core/rcu 3/5] srcu: Provide internal interface to start a Tree SRCU grace period

[Paul E. McKenney]

On Fri, Nov 20, 2020 at 05:06:50PM +0530, Neeraj Upadhyay wrote:
> 
> 
> On 11/17/2020 6:10 AM, paulmck@kernel.org wrote:
> > From: "Paul E. McKenney" <paulmck@kernel.org>
> > 
> > There is a need for a polling interface for SRCU grace periods.
> > This polling needs to initiate an SRCU grace period without having
> > to queue (and manage) a callback.  This commit therefore splits the
> > Tree SRCU __call_srcu() function into callback-initialization and
> > queuing/start-grace-period portions, with the latter in a new function
> > named srcu_gp_start_if_needed().  This function may be passed a NULL
> > callback pointer, in which case it will refrain from queuing anything.
> > 
> > Why have the new function mess with queuing?  Locking considerations,
> > of course!
> > 
> > Link: https://lore.kernel.org/rcu/20201112201547.GF3365678@moria.home.lan/
> > Reported-by: Kent Overstreet <kent.overstreet@gmail.com>
> > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > ---
> 
> Reviewed-by: Neeraj Upadhyay <neeraju@codeaurora.org>

I applied both Reviewed-bys, thank you!

							Thanx, Paul

> Thanks
> Neeraj
> 
> >   kernel/rcu/srcutree.c | 66 +++++++++++++++++++++++++++++----------------------
> >   1 file changed, 37 insertions(+), 29 deletions(-)
> > 
> > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > index 79b7081..d930ece 100644
> > --- a/kernel/rcu/srcutree.c
> > +++ b/kernel/rcu/srcutree.c
> > @@ -808,6 +808,42 @@ static void srcu_leak_callback(struct rcu_head *rhp)
> >   }
> >   /*
> > + * Start an SRCU grace period, and also queue the callback if non-NULL.
> > + */
> > +static void srcu_gp_start_if_needed(struct srcu_struct *ssp, struct rcu_head *rhp, bool do_norm)
> > +{
> > +	unsigned long flags;
> > +	int idx;
> > +	bool needexp = false;
> > +	bool needgp = false;
> > +	unsigned long s;
> > +	struct srcu_data *sdp;
> > +
> > +	idx = srcu_read_lock(ssp);
> > +	sdp = raw_cpu_ptr(ssp->sda);
> > +	spin_lock_irqsave_rcu_node(sdp, flags);
> > +	rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp);
> > +	rcu_segcblist_advance(&sdp->srcu_cblist,
> > +			      rcu_seq_current(&ssp->srcu_gp_seq));
> > +	s = rcu_seq_snap(&ssp->srcu_gp_seq);
> > +	(void)rcu_segcblist_accelerate(&sdp->srcu_cblist, s);
> > +	if (ULONG_CMP_LT(sdp->srcu_gp_seq_needed, s)) {
> > +		sdp->srcu_gp_seq_needed = s;
> > +		needgp = true;
> > +	}
> > +	if (!do_norm && ULONG_CMP_LT(sdp->srcu_gp_seq_needed_exp, s)) {
> > +		sdp->srcu_gp_seq_needed_exp = s;
> > +		needexp = true;
> > +	}
> > +	spin_unlock_irqrestore_rcu_node(sdp, flags);
> > +	if (needgp)
> > +		srcu_funnel_gp_start(ssp, sdp, s, do_norm);
> > +	else if (needexp)
> > +		srcu_funnel_exp_start(ssp, sdp->mynode, s);
> > +	srcu_read_unlock(ssp, idx);
> > +}
> > +
> > +/*
> >    * Enqueue an SRCU callback on the srcu_data structure associated with
> >    * the current CPU and the specified srcu_struct structure, initiating
> >    * grace-period processing if it is not already running.
> > @@ -838,13 +874,6 @@ static void srcu_leak_callback(struct rcu_head *rhp)
> >   static void __call_srcu(struct srcu_struct *ssp, struct rcu_head *rhp,
> >   			rcu_callback_t func, bool do_norm)
> >   {
> > -	unsigned long flags;
> > -	int idx;
> > -	bool needexp = false;
> > -	bool needgp = false;
> > -	unsigned long s;
> > -	struct srcu_data *sdp;
> > -
> >   	check_init_srcu_struct(ssp);
> >   	if (debug_rcu_head_queue(rhp)) {
> >   		/* Probable double call_srcu(), so leak the callback. */
> > @@ -853,28 +882,7 @@ static void __call_srcu(struct srcu_struct *ssp, struct rcu_head *rhp,
> >   		return;
> >   	}
> >   	rhp->func = func;
> > -	idx = srcu_read_lock(ssp);
> > -	sdp = raw_cpu_ptr(ssp->sda);
> > -	spin_lock_irqsave_rcu_node(sdp, flags);
> > -	rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp);
> > -	rcu_segcblist_advance(&sdp->srcu_cblist,
> > -			      rcu_seq_current(&ssp->srcu_gp_seq));
> > -	s = rcu_seq_snap(&ssp->srcu_gp_seq);
> > -	(void)rcu_segcblist_accelerate(&sdp->srcu_cblist, s);
> > -	if (ULONG_CMP_LT(sdp->srcu_gp_seq_needed, s)) {
> > -		sdp->srcu_gp_seq_needed = s;
> > -		needgp = true;
> > -	}
> > -	if (!do_norm && ULONG_CMP_LT(sdp->srcu_gp_seq_needed_exp, s)) {
> > -		sdp->srcu_gp_seq_needed_exp = s;
> > -		needexp = true;
> > -	}
> > -	spin_unlock_irqrestore_rcu_node(sdp, flags);
> > -	if (needgp)
> > -		srcu_funnel_gp_start(ssp, sdp, s, do_norm);
> > -	else if (needexp)
> > -		srcu_funnel_exp_start(ssp, sdp->mynode, s);
> > -	srcu_read_unlock(ssp, idx);
> > +	srcu_gp_start_if_needed(ssp, rhp, do_norm);
> >   }
> >   /**
> > 
> 
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of
> the Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH tip/core/rcu 0/5] Provide SRCU polling grace-period interfaces

[Paul E. McKenney]

Hello!

This is V2 of the series provides a polling interface for SRCU grace periods.  The
API is still as follows:

unsigned long start_poll_synchronize_srcu(struct srcu_struct *ssp)

	Returns a "cookie" that can be thought of as a snapshot of
	the specified SRCU instance's grace-period sequence number.
	Also ensures that enough future grace periods happen to eventually
	make the grace-period sequence number reach the cookie.

bool poll_state_synchronize_srcu(struct srcu_struct *ssp, unsigned long cookie)

	Given a cookie from start_poll_synchronize_srcu(), returns true if
	at least one full SRCU grace period has elapsed in the meantime.
	Given finite SRCU readers in a well-behaved kernel, the following
	code will complete in finite time:

		cookie = start_poll_synchronize_srcu(&my_srcu);
		while (!poll_state_synchronize_srcu(&my_srcu, cookie))
			schedule_timeout_uninterruptible(1);

unsigned long get_state_synchronize_srcu(struct srcu_struct *ssp)

	Like start_poll_synchronize_srcu(), except that it does not start
	any grace periods.  This means that the following code is -not-
	guaranteed to complete:

		cookie = get_state_synchronize_srcu(&my_srcu);
		while (!poll_state_synchronize_srcu(&my_srcu, cookie))
			schedule_timeout_uninterruptible(1);

	Use this if you know that something else will be starting the
	needed SRCU grace periods.  This might also be useful if you
	had items that were likely to be reused before the SRCU grace
	period elapsed, so that you avoid burning CPU on SRCU grace
	periods that prove to be unnecessary.  Or if you don't want
	to have more than (say) 100 SRCU grace periods per seconds,
	in which case you might use a timer to start the grace periods.
	Or maybe you don't bother starting the SRCU grace period until
	some sort of emergency situation has arisen.  Or...

	OK, maybe no one needs it, but rcutorture does need it, so here
	it is anyway.

The patches in this version of the series are as follows:

1.	Make Tiny SRCU use multi-bit grace-period counter.

2.	Provide internal interface to start a Tiny SRCU grace period.

3.	Provide internal interface to start a Tree SRCU grace period.

4.	Provide polling interfaces for Tiny SRCU grace periods.

5.	Provide polling interfaces for Tree SRCU grace periods.

6.	Document polling interfaces for Tree SRCU grace periods.

Changes from v1:

o	Added EXPORT_SYMBOL_GPL() to allow rcutorture testing when
	rcutorture is built as a module.

o	Applied review feedback from Neeraj Upadhyay.

o	Updated RCU requirements documentation.

						Thanx, Paul

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

 Documentation/RCU/Design/Requirements/Requirements.rst |   18 ++
 include/linux/rcupdate.h                               |    2 
 include/linux/srcu.h                                   |    3 
 include/linux/srcutiny.h                               |    5 
 kernel/rcu/srcutiny.c                                  |   76 ++++++++-
 kernel/rcu/srcutree.c                                  |  133 ++++++++++++-----
 6 files changed, 191 insertions(+), 46 deletions(-)

Re: [PATCH tip/core/rcu 0/5] Provide SRCU polling grace-period interfaces

[Steven Rostedt]

On Fri, 20 Nov 2020 16:58:09 -0800
"Paul E. McKenney" <paulmck@kernel.org> wrote:

> 	OK, maybe no one needs it, but rcutorture does need it, so here
> 	it is anyway.

Ah, so there was a use case. I was scratching my head a bit before
reading this ;-)

-- Steve

Re: [PATCH tip/core/rcu 0/5] Provide SRCU polling grace-period interfaces

[Paul E. McKenney]

On Fri, Nov 20, 2020 at 08:05:53PM -0500, Steven Rostedt wrote:
> On Fri, 20 Nov 2020 16:58:09 -0800
> "Paul E. McKenney" <paulmck@kernel.org> wrote:
> 
> > 	OK, maybe no one needs it, but rcutorture does need it, so here
> > 	it is anyway.
> 
> Ah, so there was a use case. I was scratching my head a bit before
> reading this ;-)

Indeed, the overhead of start_poll_synchronize_srcu() is a killer for
some parts of rcutorture, plus those parts don't care if no grace period
gets started...

							Thanx, Paul

Re: [PATCH RFC tip/core/rcu 5/5] srcu: Provide polling interfaces for Tree SRCU grace periods

[Neeraj Upadhyay]

On 11/21/2020 5:46 AM, Paul E. McKenney wrote:
> On Fri, Nov 20, 2020 at 05:31:43PM +0530, Neeraj Upadhyay wrote:
>>
>>
>> On 11/17/2020 6:10 AM, paulmck@kernel.org wrote:
>>> From: "Paul E. McKenney" <paulmck@kernel.org>
>>>
>>> There is a need for a polling interface for SRCU grace
>>> periods, so this commit supplies get_state_synchronize_srcu(),
>>> start_poll_synchronize_srcu(), and poll_state_synchronize_srcu() for this
>>> purpose.  The first can be used if future grace periods are inevitable
>>> (perhaps due to a later call_srcu() invocation), the second if future
>>> grace periods might not otherwise happen, and the third to check if a
>>> grace period has elapsed since the corresponding call to either of the
>>> first two.
>>>
>>> As with get_state_synchronize_rcu() and cond_synchronize_rcu(),
>>> the return value from either get_state_synchronize_srcu() or
>>> start_poll_synchronize_srcu() must be passed in to a later call to
>>> poll_state_synchronize_srcu().
>>>
>>> Link: https://lore.kernel.org/rcu/20201112201547.GF3365678@moria.home.lan/
>>> Reported-by: Kent Overstreet <kent.overstreet@gmail.com>
>>> [ paulmck: Add EXPORT_SYMBOL_GPL() per kernel test robot feedback. ]
>>> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
>>> ---
>>>    kernel/rcu/srcutree.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++---
>>>    1 file changed, 60 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
>>> index d930ece..015d80e 100644
>>> --- a/kernel/rcu/srcutree.c
>>> +++ b/kernel/rcu/srcutree.c
>>> @@ -810,7 +810,8 @@ static void srcu_leak_callback(struct rcu_head *rhp)
>>>    /*
>>>     * Start an SRCU grace period, and also queue the callback if non-NULL.
>>>     */
>>> -static void srcu_gp_start_if_needed(struct srcu_struct *ssp, struct rcu_head *rhp, bool do_norm)
>>> +static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
>>> +					     struct rcu_head *rhp, bool do_norm)
>>>    {
>>>    	unsigned long flags;
>>>    	int idx;
>>> @@ -822,7 +823,8 @@ static void srcu_gp_start_if_needed(struct srcu_struct *ssp, struct rcu_head *rh
>>>    	idx = srcu_read_lock(ssp);
>>>    	sdp = raw_cpu_ptr(ssp->sda);
>>>    	spin_lock_irqsave_rcu_node(sdp, flags);
>>> -	rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp);
>>> +	if (rhp)
>>> +		rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp);
>>>    	rcu_segcblist_advance(&sdp->srcu_cblist,
>>>    			      rcu_seq_current(&ssp->srcu_gp_seq));
>>>    	s = rcu_seq_snap(&ssp->srcu_gp_seq);
>>> @@ -841,6 +843,7 @@ static void srcu_gp_start_if_needed(struct srcu_struct *ssp, struct rcu_head *rh
>>>    	else if (needexp)
>>>    		srcu_funnel_exp_start(ssp, sdp->mynode, s);
>>>    	srcu_read_unlock(ssp, idx);
>>> +	return s;
>>>    }
>>>    /*
>>> @@ -882,7 +885,7 @@ static void __call_srcu(struct srcu_struct *ssp, struct rcu_head *rhp,
>>>    		return;
>>>    	}
>>>    	rhp->func = func;
>>> -	srcu_gp_start_if_needed(ssp, rhp, do_norm);
>>> +	(void)srcu_gp_start_if_needed(ssp, rhp, do_norm);
>>>    }
>>>    /**
>>> @@ -1011,6 +1014,60 @@ void synchronize_srcu(struct srcu_struct *ssp)
>>>    }
>>>    EXPORT_SYMBOL_GPL(synchronize_srcu);
>>> +/**
>>> + * get_state_synchronize_srcu - Provide an end-of-grace-period cookie
>>> + * @ssp: srcu_struct to provide cookie for.
>>> + *
>>> + * This function returns a cookie that can be passed to
>>> + * poll_state_synchronize_srcu(), which will return true if a full grace
>>> + * period has elapsed in the meantime.  It is the caller's responsibility
>>> + * to make sure that grace period happens, for example, by invoking
>>> + * call_srcu() after return from get_state_synchronize_srcu().
>>> + */
>>> +unsigned long get_state_synchronize_srcu(struct srcu_struct *ssp)
>>> +{
>>> +	// Any prior manipulation of SRCU-protected data must happen
>>> +        // before the load from ->srcu_gp_seq.
>>> +	smp_mb();
>>> +	return rcu_seq_snap(&ssp->srcu_gp_seq);
>>> +}
>>> +EXPORT_SYMBOL_GPL(get_state_synchronize_srcu);
>>> +
>>> +/**
>>> + * start_poll_synchronize_srcu - Provide cookie and start grace period
>>> + * @ssp: srcu_struct to provide cookie for.
>>> + *
>>> + * This function returns a cookie that can be passed to
>>> + * poll_state_synchronize_srcu(), which will return true if a full grace
>>> + * period has elapsed in the meantime.  Unlike get_state_synchronize_srcu(),
>>> + * this function also ensures that any needed SRCU grace period will be
>>> + * started.  This convenience does come at a cost in terms of CPU overhead.
>>> + */
>>> +unsigned long start_poll_synchronize_srcu(struct srcu_struct *ssp)
>>> +{
>>> +	return srcu_gp_start_if_needed(ssp, NULL, true);
>>> +}
>>> +EXPORT_SYMBOL_GPL(start_poll_synchronize_srcu);
>>> +
>>> +/**
>>> + * poll_state_synchronize_srcu - Has cookie's grace period ended?
>>> + * @ssp: srcu_struct to provide cookie for.
>>> + * @cookie: Return value from get_state_synchronize_srcu() or start_poll_synchronize_srcu().
>>> + *
>>> + * This function takes the cookie that was returned from either
>>> + * get_state_synchronize_srcu() or start_poll_synchronize_srcu(), and
>>> + * returns @true if an SRCU grace period elapsed since the time that the
>>> + * cookie was created.
>>> + */
>>> +bool poll_state_synchronize_srcu(struct srcu_struct *ssp, unsigned long cookie)
>>> +{
>>> +	if (!rcu_seq_done(&ssp->srcu_gp_seq, cookie))
>>> +		return false;
>>> +	smp_mb(); // ^^^
>>
>> Minor: Should this comment be more descriptive?
> 
> You have to read between the lines?  ;-)

:)

> How about like this?
> 

Looks good to me!

Thanks
Neeraj

> 							Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
> bool poll_state_synchronize_srcu(struct srcu_struct *ssp, unsigned long cookie)
> {
> 	if (!rcu_seq_done(&ssp->srcu_gp_seq, cookie))
> 		return false;
> 	// Ensure that the end of the SRCU grace period happens before
> 	// any subsequent code that the caller might execute.
> 	smp_mb(); // ^^^
> 	return true;
> }
> 

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member of the Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH RFC tip/core/rcu 4/5] srcu: Provide polling interfaces for Tiny SRCU grace periods

[Neeraj Upadhyay]

On 11/21/2020 5:43 AM, Paul E. McKenney wrote:
> On Fri, Nov 20, 2020 at 05:28:32PM +0530, Neeraj Upadhyay wrote:
>> Hi Paul,
>>
>> On 11/17/2020 6:10 AM, paulmck@kernel.org wrote:
>>> From: "Paul E. McKenney" <paulmck@kernel.org>
>>>
>>> There is a need for a polling interface for SRCU grace
>>> periods, so this commit supplies get_state_synchronize_srcu(),
>>> start_poll_synchronize_srcu(), and poll_state_synchronize_srcu() for this
>>> purpose.  The first can be used if future grace periods are inevitable
>>> (perhaps due to a later call_srcu() invocation), the second if future
>>> grace periods might not otherwise happen, and the third to check if a
>>> grace period has elapsed since the corresponding call to either of the
>>> first two.
>>>
>>> As with get_state_synchronize_rcu() and cond_synchronize_rcu(),
>>> the return value from either get_state_synchronize_srcu() or
>>> start_poll_synchronize_srcu() must be passed in to a later call to
>>> poll_state_synchronize_srcu().
>>>
>>> Link: https://lore.kernel.org/rcu/20201112201547.GF3365678@moria.home.lan/
>>> Reported-by: Kent Overstreet <kent.overstreet@gmail.com>
>>> [ paulmck: Add EXPORT_SYMBOL_GPL() per kernel test robot feedback. ]
>>> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
>>> ---
>>>    include/linux/rcupdate.h |  2 ++
>>>    include/linux/srcu.h     |  3 +++
>>>    include/linux/srcutiny.h |  1 +
>>>    kernel/rcu/srcutiny.c    | 52 ++++++++++++++++++++++++++++++++++++++++++++++--
>>>    4 files changed, 56 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
>>> index de08264..e09c0d8 100644
>>> --- a/include/linux/rcupdate.h
>>> +++ b/include/linux/rcupdate.h
>>> @@ -33,6 +33,8 @@
>>>    #define ULONG_CMP_GE(a, b)	(ULONG_MAX / 2 >= (a) - (b))
>>>    #define ULONG_CMP_LT(a, b)	(ULONG_MAX / 2 < (a) - (b))
>>>    #define ulong2long(a)		(*(long *)(&(a)))
>>> +#define USHORT_CMP_GE(a, b)	(USHRT_MAX / 2 >= (unsigned short)((a) - (b)))
>>> +#define USHORT_CMP_LT(a, b)	(USHRT_MAX / 2 < (unsigned short)((a) - (b)))
>>>    /* Exported common interfaces */
>>>    void call_rcu(struct rcu_head *head, rcu_callback_t func);
>>> diff --git a/include/linux/srcu.h b/include/linux/srcu.h
>>> index e432cc9..a0895bb 100644
>>> --- a/include/linux/srcu.h
>>> +++ b/include/linux/srcu.h
>>> @@ -60,6 +60,9 @@ void cleanup_srcu_struct(struct srcu_struct *ssp);
>>>    int __srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp);
>>>    void __srcu_read_unlock(struct srcu_struct *ssp, int idx) __releases(ssp);
>>>    void synchronize_srcu(struct srcu_struct *ssp);
>>> +unsigned long get_state_synchronize_srcu(struct srcu_struct *ssp);
>>> +unsigned long start_poll_synchronize_srcu(struct srcu_struct *ssp);
>>> +bool poll_state_synchronize_srcu(struct srcu_struct *ssp, unsigned long cookie);
>>>    #ifdef CONFIG_DEBUG_LOCK_ALLOC
>>> diff --git a/include/linux/srcutiny.h b/include/linux/srcutiny.h
>>> index fed4a2d..e9bd6fb 100644
>>> --- a/include/linux/srcutiny.h
>>> +++ b/include/linux/srcutiny.h
>>> @@ -16,6 +16,7 @@
>>>    struct srcu_struct {
>>>    	short srcu_lock_nesting[2];	/* srcu_read_lock() nesting depth. */
>>>    	unsigned short srcu_idx;	/* Current reader array element in bit 0x2. */
>>> +	unsigned short srcu_idx_max;	/* Furthest future srcu_idx request. */
>>>    	u8 srcu_gp_running;		/* GP workqueue running? */
>>>    	u8 srcu_gp_waiting;		/* GP waiting for readers? */
>>>    	struct swait_queue_head srcu_wq;
>>> diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c
>>> index 3bac1db..b405811 100644
>>> --- a/kernel/rcu/srcutiny.c
>>> +++ b/kernel/rcu/srcutiny.c
>>> @@ -34,6 +34,7 @@ static int init_srcu_struct_fields(struct srcu_struct *ssp)
>>>    	ssp->srcu_gp_running = false;
>>>    	ssp->srcu_gp_waiting = false;
>>>    	ssp->srcu_idx = 0;
>>> +	ssp->srcu_idx_max = 0;
>>>    	INIT_WORK(&ssp->srcu_work, srcu_drive_gp);
>>>    	INIT_LIST_HEAD(&ssp->srcu_work.entry);
>>>    	return 0;
>>> @@ -114,7 +115,7 @@ void srcu_drive_gp(struct work_struct *wp)
>>>    	struct srcu_struct *ssp;
>>>    	ssp = container_of(wp, struct srcu_struct, srcu_work);
>>> -	if (ssp->srcu_gp_running || !READ_ONCE(ssp->srcu_cb_head))
>>> +	if (ssp->srcu_gp_running || USHORT_CMP_GE(ssp->srcu_idx, READ_ONCE(ssp->srcu_idx_max)))
>>>    		return; /* Already running or nothing to do. */
>>>    	/* Remove recently arrived callbacks and wait for readers. */
>>> @@ -147,14 +148,19 @@ void srcu_drive_gp(struct work_struct *wp)
>>>    	 * straighten that out.
>>>    	 */
>>>    	WRITE_ONCE(ssp->srcu_gp_running, false);
>>> -	if (READ_ONCE(ssp->srcu_cb_head))
>>> +	if (USHORT_CMP_GE(ssp->srcu_idx, READ_ONCE(ssp->srcu_idx_max)))
>>
>> Should this be USHORT_CMP_LT ?
> 
> I believe that you are correct.  As is, it works but does needless
> grace periods.
> 
>>>    		schedule_work(&ssp->srcu_work);
>>>    }
>>>    EXPORT_SYMBOL_GPL(srcu_drive_gp);
>>>    static void srcu_gp_start_if_needed(struct srcu_struct *ssp)
>>>    {
>>> +	unsigned short cookie;
>>> +
>>>    	if (!READ_ONCE(ssp->srcu_gp_running)) {
>>> +		cookie = get_state_synchronize_srcu(ssp);
>>> +		if (USHORT_CMP_LT(READ_ONCE(ssp->srcu_idx_max), cookie))
>>> +			WRITE_ONCE(ssp->srcu_idx_max, cookie);
>>
>> I was thinking of a case which might break with this.
>>
>> Consider a scenario, where GP is in progress and kworker is right
>> before below point, after executing callbacks:
>>
>> void srcu_drive_gp(struct work_struct *wp) {
>>    <snip>
> 
> We updated ->srcu_idx up here, correct?  So it has bottom bit zero.
> 
>>    while (lh) {
>>    <cb execution loop>
>>    }
>>    >>> CURRENT EXECUTION POINT
> 
> Keeping in mind that Tiny SRCU always runs !PREEMPT, this must be
> due to an interrupt.
> 
Looking more, issue can happen, even when kworker is waiting for GP
completion @

swait_event_exclusive(ssp->srcu_wq, 
!READ_ONCE(ssp->srcu_lock_nesting[idx]));

Other process can call call_srcu() and skip srcu_idx_max update, as
ssp->srcu_gp_running is true.


Thanks
Neeraj

>>    WRITE_ONCE(ssp->srcu_gp_running, false);
>>
>>    if (USHORT_CMP_LT(ssp->srcu_idx, READ_ONCE(ssp->srcu_idx_max)))
>>      schedule_work(&ssp->srcu_work);
>> }
>>
>> Now, at this instance, srcu_gp_start_if_needed() runs and samples
>> srcu_gp_running and returns, without updating srcu_idx_max
>>
>> static void srcu_gp_start_if_needed(struct srcu_struct *ssp)
>> {
>>    if (!READ_ONCE(ssp->srcu_gp_running)) returns true
>>    <snip>
>> }
> 
> This could happen in an interrupt handler, so with you thus far.
> 
>> kworker running srcu_drive_gp() resumes and returns without queueing a new
>> schedule_work(&ssp->srcu_work); for new GP?
>>
>> Prior to this patch, call_srcu() enqueues a cb before entering
>> srcu_gp_start_if_needed(), and srcu_drive_gp() observes this
>> queuing, and schedule a work for the new GP, for this scenario.
>>
>>
>>   	WRITE_ONCE(ssp->srcu_gp_running, false);
>> -	if (READ_ONCE(ssp->srcu_cb_head))
>> +	if (USHORT_CMP_GE(ssp->srcu_idx, READ_ONCE(ssp->srcu_idx_max)))
>>   		schedule_work(&ssp->srcu_work);
>>
>> So, should the "cookie" calculation and "srcu_idx_max" setting be moved
>> outside of ssp->srcu_gp_running check and maybe return the same cookie to
>> caller and use that as the returned cookie from
>> start_poll_synchronize_srcu() ?
>>
>> srcu_gp_start_if_needed()
>>    cookie = get_state_synchronize_srcu(ssp);
>>    if (USHORT_CMP_LT(READ_ONCE(ssp->srcu_idx_max), cookie))
>>       WRITE_ONCE(ssp->srcu_idx_max, cookie);
>>    if (!READ_ONCE(ssp->srcu_gp_running)) {
>>    <snip>
>> }
> 
> I believe that you are quite correct, thank you!
> 
> But rcutorture does have a call_srcu() (really a ->call, but same if SRCU)
> in a timer handler.  The race window is quite narrow, so testing it might
> be a challenge...
> 
> This is what I end up with:
> 
> 	static void srcu_gp_start_if_needed(struct srcu_struct *ssp)
> 	{
> 		unsigned short cookie;
> 
> 		cookie = get_state_synchronize_srcu(ssp);
> 		if (USHORT_CMP_LT(READ_ONCE(ssp->srcu_idx_max), cookie))
> 			WRITE_ONCE(ssp->srcu_idx_max, cookie);
> 		if (!READ_ONCE(ssp->srcu_gp_running)) {
> 			if (likely(srcu_init_done))
> 				schedule_work(&ssp->srcu_work);
> 			else if (list_empty(&ssp->srcu_work.entry))
> 				list_add(&ssp->srcu_work.entry, &srcu_boot_list);
> 		}
> 	}
> 
> Does that look plausible?

Looks good.

> 
> 							Thanx, Paul
> 

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member of the Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH RFC tip/core/rcu 4/5] srcu: Provide polling interfaces for Tiny SRCU grace periods

[Paul E. McKenney]

On Sun, Nov 22, 2020 at 07:57:26PM +0530, Neeraj Upadhyay wrote:
> On 11/21/2020 5:43 AM, Paul E. McKenney wrote:
> > On Fri, Nov 20, 2020 at 05:28:32PM +0530, Neeraj Upadhyay wrote:
> > > Hi Paul,
> > > 
> > > On 11/17/2020 6:10 AM, paulmck@kernel.org wrote:
> > > > From: "Paul E. McKenney" <paulmck@kernel.org>
> > > > 
> > > > There is a need for a polling interface for SRCU grace
> > > > periods, so this commit supplies get_state_synchronize_srcu(),
> > > > start_poll_synchronize_srcu(), and poll_state_synchronize_srcu() for this
> > > > purpose.  The first can be used if future grace periods are inevitable
> > > > (perhaps due to a later call_srcu() invocation), the second if future
> > > > grace periods might not otherwise happen, and the third to check if a
> > > > grace period has elapsed since the corresponding call to either of the
> > > > first two.
> > > > 
> > > > As with get_state_synchronize_rcu() and cond_synchronize_rcu(),
> > > > the return value from either get_state_synchronize_srcu() or
> > > > start_poll_synchronize_srcu() must be passed in to a later call to
> > > > poll_state_synchronize_srcu().
> > > > 
> > > > Link: https://lore.kernel.org/rcu/20201112201547.GF3365678@moria.home.lan/
> > > > Reported-by: Kent Overstreet <kent.overstreet@gmail.com>
> > > > [ paulmck: Add EXPORT_SYMBOL_GPL() per kernel test robot feedback. ]
> > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > > ---
> > > >    include/linux/rcupdate.h |  2 ++
> > > >    include/linux/srcu.h     |  3 +++
> > > >    include/linux/srcutiny.h |  1 +
> > > >    kernel/rcu/srcutiny.c    | 52 ++++++++++++++++++++++++++++++++++++++++++++++--
> > > >    4 files changed, 56 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > > > index de08264..e09c0d8 100644
> > > > --- a/include/linux/rcupdate.h
> > > > +++ b/include/linux/rcupdate.h
> > > > @@ -33,6 +33,8 @@
> > > >    #define ULONG_CMP_GE(a, b)	(ULONG_MAX / 2 >= (a) - (b))
> > > >    #define ULONG_CMP_LT(a, b)	(ULONG_MAX / 2 < (a) - (b))
> > > >    #define ulong2long(a)		(*(long *)(&(a)))
> > > > +#define USHORT_CMP_GE(a, b)	(USHRT_MAX / 2 >= (unsigned short)((a) - (b)))
> > > > +#define USHORT_CMP_LT(a, b)	(USHRT_MAX / 2 < (unsigned short)((a) - (b)))
> > > >    /* Exported common interfaces */
> > > >    void call_rcu(struct rcu_head *head, rcu_callback_t func);
> > > > diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> > > > index e432cc9..a0895bb 100644
> > > > --- a/include/linux/srcu.h
> > > > +++ b/include/linux/srcu.h
> > > > @@ -60,6 +60,9 @@ void cleanup_srcu_struct(struct srcu_struct *ssp);
> > > >    int __srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp);
> > > >    void __srcu_read_unlock(struct srcu_struct *ssp, int idx) __releases(ssp);
> > > >    void synchronize_srcu(struct srcu_struct *ssp);
> > > > +unsigned long get_state_synchronize_srcu(struct srcu_struct *ssp);
> > > > +unsigned long start_poll_synchronize_srcu(struct srcu_struct *ssp);
> > > > +bool poll_state_synchronize_srcu(struct srcu_struct *ssp, unsigned long cookie);
> > > >    #ifdef CONFIG_DEBUG_LOCK_ALLOC
> > > > diff --git a/include/linux/srcutiny.h b/include/linux/srcutiny.h
> > > > index fed4a2d..e9bd6fb 100644
> > > > --- a/include/linux/srcutiny.h
> > > > +++ b/include/linux/srcutiny.h
> > > > @@ -16,6 +16,7 @@
> > > >    struct srcu_struct {
> > > >    	short srcu_lock_nesting[2];	/* srcu_read_lock() nesting depth. */
> > > >    	unsigned short srcu_idx;	/* Current reader array element in bit 0x2. */
> > > > +	unsigned short srcu_idx_max;	/* Furthest future srcu_idx request. */
> > > >    	u8 srcu_gp_running;		/* GP workqueue running? */
> > > >    	u8 srcu_gp_waiting;		/* GP waiting for readers? */
> > > >    	struct swait_queue_head srcu_wq;
> > > > diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c
> > > > index 3bac1db..b405811 100644
> > > > --- a/kernel/rcu/srcutiny.c
> > > > +++ b/kernel/rcu/srcutiny.c
> > > > @@ -34,6 +34,7 @@ static int init_srcu_struct_fields(struct srcu_struct *ssp)
> > > >    	ssp->srcu_gp_running = false;
> > > >    	ssp->srcu_gp_waiting = false;
> > > >    	ssp->srcu_idx = 0;
> > > > +	ssp->srcu_idx_max = 0;
> > > >    	INIT_WORK(&ssp->srcu_work, srcu_drive_gp);
> > > >    	INIT_LIST_HEAD(&ssp->srcu_work.entry);
> > > >    	return 0;
> > > > @@ -114,7 +115,7 @@ void srcu_drive_gp(struct work_struct *wp)
> > > >    	struct srcu_struct *ssp;
> > > >    	ssp = container_of(wp, struct srcu_struct, srcu_work);
> > > > -	if (ssp->srcu_gp_running || !READ_ONCE(ssp->srcu_cb_head))
> > > > +	if (ssp->srcu_gp_running || USHORT_CMP_GE(ssp->srcu_idx, READ_ONCE(ssp->srcu_idx_max)))
> > > >    		return; /* Already running or nothing to do. */
> > > >    	/* Remove recently arrived callbacks and wait for readers. */
> > > > @@ -147,14 +148,19 @@ void srcu_drive_gp(struct work_struct *wp)
> > > >    	 * straighten that out.
> > > >    	 */
> > > >    	WRITE_ONCE(ssp->srcu_gp_running, false);
> > > > -	if (READ_ONCE(ssp->srcu_cb_head))
> > > > +	if (USHORT_CMP_GE(ssp->srcu_idx, READ_ONCE(ssp->srcu_idx_max)))
> > > 
> > > Should this be USHORT_CMP_LT ?
> > 
> > I believe that you are correct.  As is, it works but does needless
> > grace periods.
> > 
> > > >    		schedule_work(&ssp->srcu_work);
> > > >    }
> > > >    EXPORT_SYMBOL_GPL(srcu_drive_gp);
> > > >    static void srcu_gp_start_if_needed(struct srcu_struct *ssp)
> > > >    {
> > > > +	unsigned short cookie;
> > > > +
> > > >    	if (!READ_ONCE(ssp->srcu_gp_running)) {
> > > > +		cookie = get_state_synchronize_srcu(ssp);
> > > > +		if (USHORT_CMP_LT(READ_ONCE(ssp->srcu_idx_max), cookie))
> > > > +			WRITE_ONCE(ssp->srcu_idx_max, cookie);
> > > 
> > > I was thinking of a case which might break with this.
> > > 
> > > Consider a scenario, where GP is in progress and kworker is right
> > > before below point, after executing callbacks:
> > > 
> > > void srcu_drive_gp(struct work_struct *wp) {
> > >    <snip>
> > 
> > We updated ->srcu_idx up here, correct?  So it has bottom bit zero.
> > 
> > >    while (lh) {
> > >    <cb execution loop>
> > >    }
> > >    >>> CURRENT EXECUTION POINT
> > 
> > Keeping in mind that Tiny SRCU always runs !PREEMPT, this must be
> > due to an interrupt.
> > 
> Looking more, issue can happen, even when kworker is waiting for GP
> completion @
> 
> swait_event_exclusive(ssp->srcu_wq,
> !READ_ONCE(ssp->srcu_lock_nesting[idx]));
> 
> Other process can call call_srcu() and skip srcu_idx_max update, as
> ssp->srcu_gp_running is true.

Good point!  Does this mean that additional changes are required,
or does the fix below cover this situation as well?

							Thanx, Paul

> Thanks
> Neeraj
> 
> > >    WRITE_ONCE(ssp->srcu_gp_running, false);
> > > 
> > >    if (USHORT_CMP_LT(ssp->srcu_idx, READ_ONCE(ssp->srcu_idx_max)))
> > >      schedule_work(&ssp->srcu_work);
> > > }
> > > 
> > > Now, at this instance, srcu_gp_start_if_needed() runs and samples
> > > srcu_gp_running and returns, without updating srcu_idx_max
> > > 
> > > static void srcu_gp_start_if_needed(struct srcu_struct *ssp)
> > > {
> > >    if (!READ_ONCE(ssp->srcu_gp_running)) returns true
> > >    <snip>
> > > }
> > 
> > This could happen in an interrupt handler, so with you thus far.
> > 
> > > kworker running srcu_drive_gp() resumes and returns without queueing a new
> > > schedule_work(&ssp->srcu_work); for new GP?
> > > 
> > > Prior to this patch, call_srcu() enqueues a cb before entering
> > > srcu_gp_start_if_needed(), and srcu_drive_gp() observes this
> > > queuing, and schedule a work for the new GP, for this scenario.
> > > 
> > > 
> > >   	WRITE_ONCE(ssp->srcu_gp_running, false);
> > > -	if (READ_ONCE(ssp->srcu_cb_head))
> > > +	if (USHORT_CMP_GE(ssp->srcu_idx, READ_ONCE(ssp->srcu_idx_max)))
> > >   		schedule_work(&ssp->srcu_work);
> > > 
> > > So, should the "cookie" calculation and "srcu_idx_max" setting be moved
> > > outside of ssp->srcu_gp_running check and maybe return the same cookie to
> > > caller and use that as the returned cookie from
> > > start_poll_synchronize_srcu() ?
> > > 
> > > srcu_gp_start_if_needed()
> > >    cookie = get_state_synchronize_srcu(ssp);
> > >    if (USHORT_CMP_LT(READ_ONCE(ssp->srcu_idx_max), cookie))
> > >       WRITE_ONCE(ssp->srcu_idx_max, cookie);
> > >    if (!READ_ONCE(ssp->srcu_gp_running)) {
> > >    <snip>
> > > }
> > 
> > I believe that you are quite correct, thank you!
> > 
> > But rcutorture does have a call_srcu() (really a ->call, but same if SRCU)
> > in a timer handler.  The race window is quite narrow, so testing it might
> > be a challenge...
> > 
> > This is what I end up with:
> > 
> > 	static void srcu_gp_start_if_needed(struct srcu_struct *ssp)
> > 	{
> > 		unsigned short cookie;
> > 
> > 		cookie = get_state_synchronize_srcu(ssp);
> > 		if (USHORT_CMP_LT(READ_ONCE(ssp->srcu_idx_max), cookie))
> > 			WRITE_ONCE(ssp->srcu_idx_max, cookie);
> > 		if (!READ_ONCE(ssp->srcu_gp_running)) {
> > 			if (likely(srcu_init_done))
> > 				schedule_work(&ssp->srcu_work);
> > 			else if (list_empty(&ssp->srcu_work.entry))
> > 				list_add(&ssp->srcu_work.entry, &srcu_boot_list);
> > 		}
> > 	}
> > 
> > Does that look plausible?
> 
> Looks good.
> 
> > 
> > 							Thanx, Paul
> > 
> 
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of
> the Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH RFC tip/core/rcu 4/5] srcu: Provide polling interfaces for Tiny SRCU grace periods

[Neeraj Upadhyay]

On 11/22/2020 11:31 PM, Paul E. McKenney wrote:
> On Sun, Nov 22, 2020 at 07:57:26PM +0530, Neeraj Upadhyay wrote:
>> On 11/21/2020 5:43 AM, Paul E. McKenney wrote:
>>> On Fri, Nov 20, 2020 at 05:28:32PM +0530, Neeraj Upadhyay wrote:
>>>> Hi Paul,
>>>>
>>>> On 11/17/2020 6:10 AM, paulmck@kernel.org wrote:
>>>>> From: "Paul E. McKenney" <paulmck@kernel.org>
>>>>>
>>>>> There is a need for a polling interface for SRCU grace
>>>>> periods, so this commit supplies get_state_synchronize_srcu(),
>>>>> start_poll_synchronize_srcu(), and poll_state_synchronize_srcu() for this
>>>>> purpose.  The first can be used if future grace periods are inevitable
>>>>> (perhaps due to a later call_srcu() invocation), the second if future
>>>>> grace periods might not otherwise happen, and the third to check if a
>>>>> grace period has elapsed since the corresponding call to either of the
>>>>> first two.
>>>>>
>>>>> As with get_state_synchronize_rcu() and cond_synchronize_rcu(),
>>>>> the return value from either get_state_synchronize_srcu() or
>>>>> start_poll_synchronize_srcu() must be passed in to a later call to
>>>>> poll_state_synchronize_srcu().
>>>>>
>>>>> Link: https://lore.kernel.org/rcu/20201112201547.GF3365678@moria.home.lan/
>>>>> Reported-by: Kent Overstreet <kent.overstreet@gmail.com>
>>>>> [ paulmck: Add EXPORT_SYMBOL_GPL() per kernel test robot feedback. ]
>>>>> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
>>>>> ---
>>>>>     include/linux/rcupdate.h |  2 ++
>>>>>     include/linux/srcu.h     |  3 +++
>>>>>     include/linux/srcutiny.h |  1 +
>>>>>     kernel/rcu/srcutiny.c    | 52 ++++++++++++++++++++++++++++++++++++++++++++++--
>>>>>     4 files changed, 56 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
>>>>> index de08264..e09c0d8 100644
>>>>> --- a/include/linux/rcupdate.h
>>>>> +++ b/include/linux/rcupdate.h
>>>>> @@ -33,6 +33,8 @@
>>>>>     #define ULONG_CMP_GE(a, b)	(ULONG_MAX / 2 >= (a) - (b))
>>>>>     #define ULONG_CMP_LT(a, b)	(ULONG_MAX / 2 < (a) - (b))
>>>>>     #define ulong2long(a)		(*(long *)(&(a)))
>>>>> +#define USHORT_CMP_GE(a, b)	(USHRT_MAX / 2 >= (unsigned short)((a) - (b)))
>>>>> +#define USHORT_CMP_LT(a, b)	(USHRT_MAX / 2 < (unsigned short)((a) - (b)))
>>>>>     /* Exported common interfaces */
>>>>>     void call_rcu(struct rcu_head *head, rcu_callback_t func);
>>>>> diff --git a/include/linux/srcu.h b/include/linux/srcu.h
>>>>> index e432cc9..a0895bb 100644
>>>>> --- a/include/linux/srcu.h
>>>>> +++ b/include/linux/srcu.h
>>>>> @@ -60,6 +60,9 @@ void cleanup_srcu_struct(struct srcu_struct *ssp);
>>>>>     int __srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp);
>>>>>     void __srcu_read_unlock(struct srcu_struct *ssp, int idx) __releases(ssp);
>>>>>     void synchronize_srcu(struct srcu_struct *ssp);
>>>>> +unsigned long get_state_synchronize_srcu(struct srcu_struct *ssp);
>>>>> +unsigned long start_poll_synchronize_srcu(struct srcu_struct *ssp);
>>>>> +bool poll_state_synchronize_srcu(struct srcu_struct *ssp, unsigned long cookie);
>>>>>     #ifdef CONFIG_DEBUG_LOCK_ALLOC
>>>>> diff --git a/include/linux/srcutiny.h b/include/linux/srcutiny.h
>>>>> index fed4a2d..e9bd6fb 100644
>>>>> --- a/include/linux/srcutiny.h
>>>>> +++ b/include/linux/srcutiny.h
>>>>> @@ -16,6 +16,7 @@
>>>>>     struct srcu_struct {
>>>>>     	short srcu_lock_nesting[2];	/* srcu_read_lock() nesting depth. */
>>>>>     	unsigned short srcu_idx;	/* Current reader array element in bit 0x2. */
>>>>> +	unsigned short srcu_idx_max;	/* Furthest future srcu_idx request. */
>>>>>     	u8 srcu_gp_running;		/* GP workqueue running? */
>>>>>     	u8 srcu_gp_waiting;		/* GP waiting for readers? */
>>>>>     	struct swait_queue_head srcu_wq;
>>>>> diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c
>>>>> index 3bac1db..b405811 100644
>>>>> --- a/kernel/rcu/srcutiny.c
>>>>> +++ b/kernel/rcu/srcutiny.c
>>>>> @@ -34,6 +34,7 @@ static int init_srcu_struct_fields(struct srcu_struct *ssp)
>>>>>     	ssp->srcu_gp_running = false;
>>>>>     	ssp->srcu_gp_waiting = false;
>>>>>     	ssp->srcu_idx = 0;
>>>>> +	ssp->srcu_idx_max = 0;
>>>>>     	INIT_WORK(&ssp->srcu_work, srcu_drive_gp);
>>>>>     	INIT_LIST_HEAD(&ssp->srcu_work.entry);
>>>>>     	return 0;
>>>>> @@ -114,7 +115,7 @@ void srcu_drive_gp(struct work_struct *wp)
>>>>>     	struct srcu_struct *ssp;
>>>>>     	ssp = container_of(wp, struct srcu_struct, srcu_work);
>>>>> -	if (ssp->srcu_gp_running || !READ_ONCE(ssp->srcu_cb_head))
>>>>> +	if (ssp->srcu_gp_running || USHORT_CMP_GE(ssp->srcu_idx, READ_ONCE(ssp->srcu_idx_max)))
>>>>>     		return; /* Already running or nothing to do. */
>>>>>     	/* Remove recently arrived callbacks and wait for readers. */
>>>>> @@ -147,14 +148,19 @@ void srcu_drive_gp(struct work_struct *wp)
>>>>>     	 * straighten that out.
>>>>>     	 */
>>>>>     	WRITE_ONCE(ssp->srcu_gp_running, false);
>>>>> -	if (READ_ONCE(ssp->srcu_cb_head))
>>>>> +	if (USHORT_CMP_GE(ssp->srcu_idx, READ_ONCE(ssp->srcu_idx_max)))
>>>>
>>>> Should this be USHORT_CMP_LT ?
>>>
>>> I believe that you are correct.  As is, it works but does needless
>>> grace periods.
>>>
>>>>>     		schedule_work(&ssp->srcu_work);
>>>>>     }
>>>>>     EXPORT_SYMBOL_GPL(srcu_drive_gp);
>>>>>     static void srcu_gp_start_if_needed(struct srcu_struct *ssp)
>>>>>     {
>>>>> +	unsigned short cookie;
>>>>> +
>>>>>     	if (!READ_ONCE(ssp->srcu_gp_running)) {
>>>>> +		cookie = get_state_synchronize_srcu(ssp);
>>>>> +		if (USHORT_CMP_LT(READ_ONCE(ssp->srcu_idx_max), cookie))
>>>>> +			WRITE_ONCE(ssp->srcu_idx_max, cookie);
>>>>
>>>> I was thinking of a case which might break with this.
>>>>
>>>> Consider a scenario, where GP is in progress and kworker is right
>>>> before below point, after executing callbacks:
>>>>
>>>> void srcu_drive_gp(struct work_struct *wp) {
>>>>     <snip>
>>>
>>> We updated ->srcu_idx up here, correct?  So it has bottom bit zero.
>>>
>>>>     while (lh) {
>>>>     <cb execution loop>
>>>>     }
>>>>     >>> CURRENT EXECUTION POINT
>>>
>>> Keeping in mind that Tiny SRCU always runs !PREEMPT, this must be
>>> due to an interrupt.
>>>
>> Looking more, issue can happen, even when kworker is waiting for GP
>> completion @
>>
>> swait_event_exclusive(ssp->srcu_wq,
>> !READ_ONCE(ssp->srcu_lock_nesting[idx]));
>>
>> Other process can call call_srcu() and skip srcu_idx_max update, as
>> ssp->srcu_gp_running is true.
> 
> Good point!  Does this mean that additional changes are required,
> or does the fix below cover this situation as well?
> 
> 							Thanx, Paul
> 

I think the current fix covers this. Just wanted to higlight that
the window is not small and a rcutorture test case might be able to 
uncover the issue?


Thanks
Neeraj

>> Thanks
>> Neeraj
>>
>>>>     WRITE_ONCE(ssp->srcu_gp_running, false);
>>>>
>>>>     if (USHORT_CMP_LT(ssp->srcu_idx, READ_ONCE(ssp->srcu_idx_max)))
>>>>       schedule_work(&ssp->srcu_work);
>>>> }
>>>>
>>>> Now, at this instance, srcu_gp_start_if_needed() runs and samples
>>>> srcu_gp_running and returns, without updating srcu_idx_max
>>>>
>>>> static void srcu_gp_start_if_needed(struct srcu_struct *ssp)
>>>> {
>>>>     if (!READ_ONCE(ssp->srcu_gp_running)) returns true
>>>>     <snip>
>>>> }
>>>
>>> This could happen in an interrupt handler, so with you thus far.
>>>
>>>> kworker running srcu_drive_gp() resumes and returns without queueing a new
>>>> schedule_work(&ssp->srcu_work); for new GP?
>>>>
>>>> Prior to this patch, call_srcu() enqueues a cb before entering
>>>> srcu_gp_start_if_needed(), and srcu_drive_gp() observes this
>>>> queuing, and schedule a work for the new GP, for this scenario.
>>>>
>>>>
>>>>    	WRITE_ONCE(ssp->srcu_gp_running, false);
>>>> -	if (READ_ONCE(ssp->srcu_cb_head))
>>>> +	if (USHORT_CMP_GE(ssp->srcu_idx, READ_ONCE(ssp->srcu_idx_max)))
>>>>    		schedule_work(&ssp->srcu_work);
>>>>
>>>> So, should the "cookie" calculation and "srcu_idx_max" setting be moved
>>>> outside of ssp->srcu_gp_running check and maybe return the same cookie to
>>>> caller and use that as the returned cookie from
>>>> start_poll_synchronize_srcu() ?
>>>>
>>>> srcu_gp_start_if_needed()
>>>>     cookie = get_state_synchronize_srcu(ssp);
>>>>     if (USHORT_CMP_LT(READ_ONCE(ssp->srcu_idx_max), cookie))
>>>>        WRITE_ONCE(ssp->srcu_idx_max, cookie);
>>>>     if (!READ_ONCE(ssp->srcu_gp_running)) {
>>>>     <snip>
>>>> }
>>>
>>> I believe that you are quite correct, thank you!
>>>
>>> But rcutorture does have a call_srcu() (really a ->call, but same if SRCU)
>>> in a timer handler.  The race window is quite narrow, so testing it might
>>> be a challenge...
>>>
>>> This is what I end up with:
>>>
>>> 	static void srcu_gp_start_if_needed(struct srcu_struct *ssp)
>>> 	{
>>> 		unsigned short cookie;
>>>
>>> 		cookie = get_state_synchronize_srcu(ssp);
>>> 		if (USHORT_CMP_LT(READ_ONCE(ssp->srcu_idx_max), cookie))
>>> 			WRITE_ONCE(ssp->srcu_idx_max, cookie);
>>> 		if (!READ_ONCE(ssp->srcu_gp_running)) {
>>> 			if (likely(srcu_init_done))
>>> 				schedule_work(&ssp->srcu_work);
>>> 			else if (list_empty(&ssp->srcu_work.entry))
>>> 				list_add(&ssp->srcu_work.entry, &srcu_boot_list);
>>> 		}
>>> 	}
>>>
>>> Does that look plausible?
>>
>> Looks good.
>>
>>>
>>> 							Thanx, Paul
>>>
>>
>> -- 
>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of
>> the Code Aurora Forum, hosted by The Linux Foundation

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member of the Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH RFC tip/core/rcu 4/5] srcu: Provide polling interfaces for Tiny SRCU grace periods

[Paul E. McKenney]

On Mon, Nov 23, 2020 at 10:04:23AM +0530, Neeraj Upadhyay wrote:
> On 11/22/2020 11:31 PM, Paul E. McKenney wrote:
> > On Sun, Nov 22, 2020 at 07:57:26PM +0530, Neeraj Upadhyay wrote:
> > > On 11/21/2020 5:43 AM, Paul E. McKenney wrote:
> > > > On Fri, Nov 20, 2020 at 05:28:32PM +0530, Neeraj Upadhyay wrote:
> > > > > Hi Paul,
> > > > > 
> > > > > On 11/17/2020 6:10 AM, paulmck@kernel.org wrote:
> > > > > > From: "Paul E. McKenney" <paulmck@kernel.org>
> > > > > > 
> > > > > > There is a need for a polling interface for SRCU grace
> > > > > > periods, so this commit supplies get_state_synchronize_srcu(),
> > > > > > start_poll_synchronize_srcu(), and poll_state_synchronize_srcu() for this
> > > > > > purpose.  The first can be used if future grace periods are inevitable
> > > > > > (perhaps due to a later call_srcu() invocation), the second if future
> > > > > > grace periods might not otherwise happen, and the third to check if a
> > > > > > grace period has elapsed since the corresponding call to either of the
> > > > > > first two.
> > > > > > 
> > > > > > As with get_state_synchronize_rcu() and cond_synchronize_rcu(),
> > > > > > the return value from either get_state_synchronize_srcu() or
> > > > > > start_poll_synchronize_srcu() must be passed in to a later call to
> > > > > > poll_state_synchronize_srcu().
> > > > > > 
> > > > > > Link: https://lore.kernel.org/rcu/20201112201547.GF3365678@moria.home.lan/
> > > > > > Reported-by: Kent Overstreet <kent.overstreet@gmail.com>
> > > > > > [ paulmck: Add EXPORT_SYMBOL_GPL() per kernel test robot feedback. ]
> > > > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > > > > ---
> > > > > >     include/linux/rcupdate.h |  2 ++
> > > > > >     include/linux/srcu.h     |  3 +++
> > > > > >     include/linux/srcutiny.h |  1 +
> > > > > >     kernel/rcu/srcutiny.c    | 52 ++++++++++++++++++++++++++++++++++++++++++++++--
> > > > > >     4 files changed, 56 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > > > > > index de08264..e09c0d8 100644
> > > > > > --- a/include/linux/rcupdate.h
> > > > > > +++ b/include/linux/rcupdate.h
> > > > > > @@ -33,6 +33,8 @@
> > > > > >     #define ULONG_CMP_GE(a, b)	(ULONG_MAX / 2 >= (a) - (b))
> > > > > >     #define ULONG_CMP_LT(a, b)	(ULONG_MAX / 2 < (a) - (b))
> > > > > >     #define ulong2long(a)		(*(long *)(&(a)))
> > > > > > +#define USHORT_CMP_GE(a, b)	(USHRT_MAX / 2 >= (unsigned short)((a) - (b)))
> > > > > > +#define USHORT_CMP_LT(a, b)	(USHRT_MAX / 2 < (unsigned short)((a) - (b)))
> > > > > >     /* Exported common interfaces */
> > > > > >     void call_rcu(struct rcu_head *head, rcu_callback_t func);
> > > > > > diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> > > > > > index e432cc9..a0895bb 100644
> > > > > > --- a/include/linux/srcu.h
> > > > > > +++ b/include/linux/srcu.h
> > > > > > @@ -60,6 +60,9 @@ void cleanup_srcu_struct(struct srcu_struct *ssp);
> > > > > >     int __srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp);
> > > > > >     void __srcu_read_unlock(struct srcu_struct *ssp, int idx) __releases(ssp);
> > > > > >     void synchronize_srcu(struct srcu_struct *ssp);
> > > > > > +unsigned long get_state_synchronize_srcu(struct srcu_struct *ssp);
> > > > > > +unsigned long start_poll_synchronize_srcu(struct srcu_struct *ssp);
> > > > > > +bool poll_state_synchronize_srcu(struct srcu_struct *ssp, unsigned long cookie);
> > > > > >     #ifdef CONFIG_DEBUG_LOCK_ALLOC
> > > > > > diff --git a/include/linux/srcutiny.h b/include/linux/srcutiny.h
> > > > > > index fed4a2d..e9bd6fb 100644
> > > > > > --- a/include/linux/srcutiny.h
> > > > > > +++ b/include/linux/srcutiny.h
> > > > > > @@ -16,6 +16,7 @@
> > > > > >     struct srcu_struct {
> > > > > >     	short srcu_lock_nesting[2];	/* srcu_read_lock() nesting depth. */
> > > > > >     	unsigned short srcu_idx;	/* Current reader array element in bit 0x2. */
> > > > > > +	unsigned short srcu_idx_max;	/* Furthest future srcu_idx request. */
> > > > > >     	u8 srcu_gp_running;		/* GP workqueue running? */
> > > > > >     	u8 srcu_gp_waiting;		/* GP waiting for readers? */
> > > > > >     	struct swait_queue_head srcu_wq;
> > > > > > diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c
> > > > > > index 3bac1db..b405811 100644
> > > > > > --- a/kernel/rcu/srcutiny.c
> > > > > > +++ b/kernel/rcu/srcutiny.c
> > > > > > @@ -34,6 +34,7 @@ static int init_srcu_struct_fields(struct srcu_struct *ssp)
> > > > > >     	ssp->srcu_gp_running = false;
> > > > > >     	ssp->srcu_gp_waiting = false;
> > > > > >     	ssp->srcu_idx = 0;
> > > > > > +	ssp->srcu_idx_max = 0;
> > > > > >     	INIT_WORK(&ssp->srcu_work, srcu_drive_gp);
> > > > > >     	INIT_LIST_HEAD(&ssp->srcu_work.entry);
> > > > > >     	return 0;
> > > > > > @@ -114,7 +115,7 @@ void srcu_drive_gp(struct work_struct *wp)
> > > > > >     	struct srcu_struct *ssp;
> > > > > >     	ssp = container_of(wp, struct srcu_struct, srcu_work);
> > > > > > -	if (ssp->srcu_gp_running || !READ_ONCE(ssp->srcu_cb_head))
> > > > > > +	if (ssp->srcu_gp_running || USHORT_CMP_GE(ssp->srcu_idx, READ_ONCE(ssp->srcu_idx_max)))
> > > > > >     		return; /* Already running or nothing to do. */
> > > > > >     	/* Remove recently arrived callbacks and wait for readers. */
> > > > > > @@ -147,14 +148,19 @@ void srcu_drive_gp(struct work_struct *wp)
> > > > > >     	 * straighten that out.
> > > > > >     	 */
> > > > > >     	WRITE_ONCE(ssp->srcu_gp_running, false);
> > > > > > -	if (READ_ONCE(ssp->srcu_cb_head))
> > > > > > +	if (USHORT_CMP_GE(ssp->srcu_idx, READ_ONCE(ssp->srcu_idx_max)))
> > > > > 
> > > > > Should this be USHORT_CMP_LT ?
> > > > 
> > > > I believe that you are correct.  As is, it works but does needless
> > > > grace periods.
> > > > 
> > > > > >     		schedule_work(&ssp->srcu_work);
> > > > > >     }
> > > > > >     EXPORT_SYMBOL_GPL(srcu_drive_gp);
> > > > > >     static void srcu_gp_start_if_needed(struct srcu_struct *ssp)
> > > > > >     {
> > > > > > +	unsigned short cookie;
> > > > > > +
> > > > > >     	if (!READ_ONCE(ssp->srcu_gp_running)) {
> > > > > > +		cookie = get_state_synchronize_srcu(ssp);
> > > > > > +		if (USHORT_CMP_LT(READ_ONCE(ssp->srcu_idx_max), cookie))
> > > > > > +			WRITE_ONCE(ssp->srcu_idx_max, cookie);
> > > > > 
> > > > > I was thinking of a case which might break with this.
> > > > > 
> > > > > Consider a scenario, where GP is in progress and kworker is right
> > > > > before below point, after executing callbacks:
> > > > > 
> > > > > void srcu_drive_gp(struct work_struct *wp) {
> > > > >     <snip>
> > > > 
> > > > We updated ->srcu_idx up here, correct?  So it has bottom bit zero.
> > > > 
> > > > >     while (lh) {
> > > > >     <cb execution loop>
> > > > >     }
> > > > >     >>> CURRENT EXECUTION POINT
> > > > 
> > > > Keeping in mind that Tiny SRCU always runs !PREEMPT, this must be
> > > > due to an interrupt.
> > > > 
> > > Looking more, issue can happen, even when kworker is waiting for GP
> > > completion @
> > > 
> > > swait_event_exclusive(ssp->srcu_wq,
> > > !READ_ONCE(ssp->srcu_lock_nesting[idx]));
> > > 
> > > Other process can call call_srcu() and skip srcu_idx_max update, as
> > > ssp->srcu_gp_running is true.
> > 
> > Good point!  Does this mean that additional changes are required,
> > or does the fix below cover this situation as well?
> 
> I think the current fix covers this. Just wanted to higlight that
> the window is not small and a rcutorture test case might be able to uncover
> the issue?

Thus far no luck, though.  I am considering that this might be another
rcutorture bug.  :-/

							Thanx, Paul

> Thanks
> Neeraj
> 
> > > Thanks
> > > Neeraj
> > > 
> > > > >     WRITE_ONCE(ssp->srcu_gp_running, false);
> > > > > 
> > > > >     if (USHORT_CMP_LT(ssp->srcu_idx, READ_ONCE(ssp->srcu_idx_max)))
> > > > >       schedule_work(&ssp->srcu_work);
> > > > > }
> > > > > 
> > > > > Now, at this instance, srcu_gp_start_if_needed() runs and samples
> > > > > srcu_gp_running and returns, without updating srcu_idx_max
> > > > > 
> > > > > static void srcu_gp_start_if_needed(struct srcu_struct *ssp)
> > > > > {
> > > > >     if (!READ_ONCE(ssp->srcu_gp_running)) returns true
> > > > >     <snip>
> > > > > }
> > > > 
> > > > This could happen in an interrupt handler, so with you thus far.
> > > > 
> > > > > kworker running srcu_drive_gp() resumes and returns without queueing a new
> > > > > schedule_work(&ssp->srcu_work); for new GP?
> > > > > 
> > > > > Prior to this patch, call_srcu() enqueues a cb before entering
> > > > > srcu_gp_start_if_needed(), and srcu_drive_gp() observes this
> > > > > queuing, and schedule a work for the new GP, for this scenario.
> > > > > 
> > > > > 
> > > > >    	WRITE_ONCE(ssp->srcu_gp_running, false);
> > > > > -	if (READ_ONCE(ssp->srcu_cb_head))
> > > > > +	if (USHORT_CMP_GE(ssp->srcu_idx, READ_ONCE(ssp->srcu_idx_max)))
> > > > >    		schedule_work(&ssp->srcu_work);
> > > > > 
> > > > > So, should the "cookie" calculation and "srcu_idx_max" setting be moved
> > > > > outside of ssp->srcu_gp_running check and maybe return the same cookie to
> > > > > caller and use that as the returned cookie from
> > > > > start_poll_synchronize_srcu() ?
> > > > > 
> > > > > srcu_gp_start_if_needed()
> > > > >     cookie = get_state_synchronize_srcu(ssp);
> > > > >     if (USHORT_CMP_LT(READ_ONCE(ssp->srcu_idx_max), cookie))
> > > > >        WRITE_ONCE(ssp->srcu_idx_max, cookie);
> > > > >     if (!READ_ONCE(ssp->srcu_gp_running)) {
> > > > >     <snip>
> > > > > }
> > > > 
> > > > I believe that you are quite correct, thank you!
> > > > 
> > > > But rcutorture does have a call_srcu() (really a ->call, but same if SRCU)
> > > > in a timer handler.  The race window is quite narrow, so testing it might
> > > > be a challenge...
> > > > 
> > > > This is what I end up with:
> > > > 
> > > > 	static void srcu_gp_start_if_needed(struct srcu_struct *ssp)
> > > > 	{
> > > > 		unsigned short cookie;
> > > > 
> > > > 		cookie = get_state_synchronize_srcu(ssp);
> > > > 		if (USHORT_CMP_LT(READ_ONCE(ssp->srcu_idx_max), cookie))
> > > > 			WRITE_ONCE(ssp->srcu_idx_max, cookie);
> > > > 		if (!READ_ONCE(ssp->srcu_gp_running)) {
> > > > 			if (likely(srcu_init_done))
> > > > 				schedule_work(&ssp->srcu_work);
> > > > 			else if (list_empty(&ssp->srcu_work.entry))
> > > > 				list_add(&ssp->srcu_work.entry, &srcu_boot_list);
> > > > 		}
> > > > 	}
> > > > 
> > > > Does that look plausible?
> > > 
> > > Looks good.
> > > 
> > > > 
> > > > 							Thanx, Paul
> > > > 
> > > 
> > > -- 
> > > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of
> > > the Code Aurora Forum, hosted by The Linux Foundation
> 
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of
> the Code Aurora Forum, hosted by The Linux Foundation