[PATCH 0/1] rcu/sync: simplify the state machine

[PATCH 0/1] rcu/sync: simplify the state machine

[Oleg Nesterov]

Let me finally try to close the gestalt ;)

This version doesn't add the new features yet, and it doesn't remove the
"must die" rcu_sync_enter_start(). But with this patch we are ready, just
I think that this should come as a separate change.

To simplify the review, see the the most important parts of the code with
the patch applied below.

Oleg.
-------------------------------------------------------------------------------

struct rcu_sync {
	int			gp_state;
	int			gp_count;
	wait_queue_head_t	gp_wait;

	struct rcu_head		cb_head;
};

enum { GP_IDLE = 0, GP_ENTER, GP_PASSED, GP_EXIT, GP_REPLAY };

#define	rss_lock	gp_wait.lock

static void rcu_sync_call(struct rcu_sync *rsp)
{
	call_rcu(&rsp->cb_head, rcu_sync_func);
}

static void rcu_sync_func(struct rcu_head *rcu)
{
	struct rcu_sync *rsp = container_of(rcu, struct rcu_sync, cb_head);
	unsigned long flags;

	WARN_ON_ONCE(rsp->gp_state == GP_IDLE);
	WARN_ON_ONCE(rsp->gp_state == GP_PASSED);

	spin_lock_irqsave(&rsp->rss_lock, flags);
	if (rsp->gp_count) {
		/*
		 * We're at least a GP after the GP_IDLE->GP_ENTER transition.
		 */
		rsp->gp_state = GP_PASSED;
		wake_up_locked(&rsp->gp_wait);
	} else if (rsp->gp_state == GP_REPLAY) {
		/*
		 * A new rcu_sync_exit() has happened; requeue the callback to
		 * catch a later GP.
		 */
		rsp->gp_state = GP_EXIT;
		rcu_sync_call(rsp);
	} else {
		/*
		 * We're at least a GP after the last rcu_sync_exit(); eveybody
		 * will now have observed the write side critical section.
		 * Let 'em rip!.
		 */
		rsp->gp_state = GP_IDLE;
	}
	spin_unlock_irqrestore(&rsp->rss_lock, flags);
}

void rcu_sync_enter(struct rcu_sync *rsp)
{
	int gp_state;

	spin_lock_irq(&rsp->rss_lock);
	gp_state = rsp->gp_state;
	if (gp_state == GP_IDLE) {
		rsp->gp_state = GP_ENTER;
		WARN_ON_ONCE(rsp->gp_count);
		/*
		 * Note that we could simply do rcu_sync_call(rsp) here and
		 * avoid the "if (gp_state == GP_IDLE)" block below.
		 *
		 * However, synchronize_rcu() can be faster if rcu_expedited
		 * or rcu_blocking_is_gp() is true.
		 *
		 * Another reason is that we can't wait for rcu callback if
		 * we are called at early boot time but this shouldn't happen.
		 */
	}
	rsp->gp_count++;
	spin_unlock_irq(&rsp->rss_lock);

	if (gp_state == GP_IDLE) {
		/*
		 * See the comment above, this simply does the "synchronous"
		 * call_rcu(rcu_sync_func) which does GP_ENTER -> GP_PASSED.
		 */
		synchronize_rcu();
		rcu_sync_func(&rsp->cb_head);
		/* Not really needed, wait_event() would see GP_PASSED. */
		return;
	}

	wait_event(rsp->gp_wait, rsp->gp_state >= GP_PASSED);
}

void rcu_sync_exit(struct rcu_sync *rsp)
{
	WARN_ON_ONCE(rsp->gp_state == GP_IDLE);
	WARN_ON_ONCE(rsp->gp_count == 0);

	spin_lock_irq(&rsp->rss_lock);
	if (!--rsp->gp_count) {
		if (rsp->gp_state == GP_PASSED) {
			rsp->gp_state = GP_EXIT;
			rcu_sync_call(rsp);
		} else if (rsp->gp_state == GP_EXIT) {
			rsp->gp_state = GP_REPLAY;
		}
	}
	spin_unlock_irq(&rsp->rss_lock);
}

Re: [PATCH 0/1] rcu/sync: simplify the state machine

[Oleg Nesterov]

With this patch rcu_sync has a single state variable and the transition rules
become really simple:

	GP_IDLE   - owned by the first rcu_sync_enter() which moves it to

	GP_ENTER  - owned by rcu-callback which moves it to

	GP_PASSED - owned by the last rcu_sync_exit() which moves it to

	GP_EXIT   - and this is the only "nontrivial" state.

		rcu-callback moves it back to GP_IDLE unless another enter()
		comes before a GP pass.

		If rcu-callback is invoked before the next rcu_sync_exit() it
		must see gp_count incremented by that enter() and set GP_PASSED.

		Otherwise, if the next rcu_sync_exit() wins the race, it will
		move it to

	GP_REPLAY - owned by rcu-callback which moves it to GP_EXIT

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 include/linux/rcu_sync.h |   2 -
 kernel/rcu/sync.c        | 165 +++++++++++++++++++++++++++--------------------
 2 files changed, 95 insertions(+), 72 deletions(-)

diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h
index e7ae221..3156a14 100644
--- a/include/linux/rcu_sync.h
+++ b/include/linux/rcu_sync.h
@@ -19,7 +19,6 @@ struct rcu_sync {
 	int			gp_count;
 	wait_queue_head_t	gp_wait;
 
-	int			cb_state;
 	struct rcu_head		cb_head;
 };
 
@@ -47,7 +46,6 @@ extern void rcu_sync_dtor(struct rcu_sync *);
 		.gp_state = 0,						\
 		.gp_count = 0,						\
 		.gp_wait = __WAIT_QUEUE_HEAD_INITIALIZER(name.gp_wait),	\
-		.cb_state = 0,						\
 	}
 
 #define	DEFINE_RCU_SYNC(name)	\
diff --git a/kernel/rcu/sync.c b/kernel/rcu/sync.c
index ee427e1..d9f80fc 100644
--- a/kernel/rcu/sync.c
+++ b/kernel/rcu/sync.c
@@ -10,15 +10,13 @@
 #include <linux/rcu_sync.h>
 #include <linux/sched.h>
 
-enum { GP_IDLE = 0, GP_PENDING, GP_PASSED };
-enum { CB_IDLE = 0, CB_PENDING, CB_REPLAY };
+enum { GP_IDLE = 0, GP_ENTER, GP_PASSED, GP_EXIT, GP_REPLAY };
 
 #define	rss_lock	gp_wait.lock
 
 /**
  * rcu_sync_init() - Initialize an rcu_sync structure
  * @rsp: Pointer to rcu_sync structure to be initialized
- * @type: Flavor of RCU with which to synchronize rcu_sync structure
  */
 void rcu_sync_init(struct rcu_sync *rsp)
 {
@@ -41,56 +39,26 @@ void rcu_sync_enter_start(struct rcu_sync *rsp)
 	rsp->gp_state = GP_PASSED;
 }
 
-/**
- * rcu_sync_enter() - Force readers onto slowpath
- * @rsp: Pointer to rcu_sync structure to use for synchronization
- *
- * This function is used by updaters who need readers to make use of
- * a slowpath during the update.  After this function returns, all
- * subsequent calls to rcu_sync_is_idle() will return false, which
- * tells readers to stay off their fastpaths.  A later call to
- * rcu_sync_exit() re-enables reader slowpaths.
- *
- * When called in isolation, rcu_sync_enter() must wait for a grace
- * period, however, closely spaced calls to rcu_sync_enter() can
- * optimize away the grace-period wait via a state machine implemented
- * by rcu_sync_enter(), rcu_sync_exit(), and rcu_sync_func().
- */
-void rcu_sync_enter(struct rcu_sync *rsp)
-{
-	bool need_wait, need_sync;
 
-	spin_lock_irq(&rsp->rss_lock);
-	need_wait = rsp->gp_count++;
-	need_sync = rsp->gp_state == GP_IDLE;
-	if (need_sync)
-		rsp->gp_state = GP_PENDING;
-	spin_unlock_irq(&rsp->rss_lock);
+static void rcu_sync_func(struct rcu_head *rcu);
 
-	WARN_ON_ONCE(need_wait && need_sync);
-	if (need_sync) {
-		synchronize_rcu();
-		rsp->gp_state = GP_PASSED;
-		wake_up_all(&rsp->gp_wait);
-	} else if (need_wait) {
-		wait_event(rsp->gp_wait, rsp->gp_state == GP_PASSED);
-	} else {
-		/*
-		 * Possible when there's a pending CB from a rcu_sync_exit().
-		 * Nobody has yet been allowed the 'fast' path and thus we can
-		 * avoid doing any sync(). The callback will get 'dropped'.
-		 */
-		WARN_ON_ONCE(rsp->gp_state != GP_PASSED);
-	}
+static void rcu_sync_call(struct rcu_sync *rsp)
+{
+	call_rcu(&rsp->cb_head, rcu_sync_func);
 }
 
 /**
  * rcu_sync_func() - Callback function managing reader access to fastpath
  * @rhp: Pointer to rcu_head in rcu_sync structure to use for synchronization
  *
- * This function is passed to one of the call_rcu() functions by
+ * This function is passed to call_rcu() function by rcu_sync_enter() and
  * rcu_sync_exit(), so that it is invoked after a grace period following the
- * that invocation of rcu_sync_exit().  It takes action based on events that
+ * that invocation of enter/exit.
+ *
+ * If it is called by rcu_sync_enter() it signals that all the readers were
+ * switched onto slow path.
+ *
+ * If it is called by rcu_sync_exit() it takes action based on events that
  * have taken place in the meantime, so that closely spaced rcu_sync_enter()
  * and rcu_sync_exit() pairs need not wait for a grace period.
  *
@@ -102,40 +70,93 @@ void rcu_sync_enter(struct rcu_sync *rsp)
  * rcu_sync_exit().  Otherwise, set all state back to idle so that readers
  * can again use their fastpaths.
  */
-static void rcu_sync_func(struct rcu_head *rhp)
+static void rcu_sync_func(struct rcu_head *rcu)
 {
-	struct rcu_sync *rsp = container_of(rhp, struct rcu_sync, cb_head);
+	struct rcu_sync *rsp = container_of(rcu, struct rcu_sync, cb_head);
 	unsigned long flags;
 
-	WARN_ON_ONCE(rsp->gp_state != GP_PASSED);
-	WARN_ON_ONCE(rsp->cb_state == CB_IDLE);
+	WARN_ON_ONCE(rsp->gp_state == GP_IDLE);
+	WARN_ON_ONCE(rsp->gp_state == GP_PASSED);
 
 	spin_lock_irqsave(&rsp->rss_lock, flags);
 	if (rsp->gp_count) {
 		/*
-		 * A new rcu_sync_begin() has happened; drop the callback.
+		 * We're at least a GP after the GP_IDLE->GP_ENTER transition.
 		 */
-		rsp->cb_state = CB_IDLE;
-	} else if (rsp->cb_state == CB_REPLAY) {
+		rsp->gp_state = GP_PASSED;
+		wake_up_locked(&rsp->gp_wait);
+	} else if (rsp->gp_state == GP_REPLAY) {
 		/*
-		 * A new rcu_sync_exit() has happened; requeue the callback
-		 * to catch a later GP.
+		 * A new rcu_sync_exit() has happened; requeue the callback to
+		 * catch a later GP.
 		 */
-		rsp->cb_state = CB_PENDING;
-		call_rcu(&rsp->cb_head, rcu_sync_func);
+		rsp->gp_state = GP_EXIT;
+		rcu_sync_call(rsp);
 	} else {
 		/*
-		 * We're at least a GP after rcu_sync_exit(); eveybody will now
-		 * have observed the write side critical section. Let 'em rip!.
+		 * We're at least a GP after the last rcu_sync_exit(); eveybody
+		 * will now have observed the write side critical section.
+		 * Let 'em rip!.
 		 */
-		rsp->cb_state = CB_IDLE;
 		rsp->gp_state = GP_IDLE;
 	}
 	spin_unlock_irqrestore(&rsp->rss_lock, flags);
 }
 
 /**
- * rcu_sync_exit() - Allow readers back onto fast patch after grace period
+ * rcu_sync_enter() - Force readers onto slowpath
+ * @rsp: Pointer to rcu_sync structure to use for synchronization
+ *
+ * This function is used by updaters who need readers to make use of
+ * a slowpath during the update.  After this function returns, all
+ * subsequent calls to rcu_sync_is_idle() will return false, which
+ * tells readers to stay off their fastpaths.  A later call to
+ * rcu_sync_exit() re-enables reader slowpaths.
+ *
+ * When called in isolation, rcu_sync_enter() must wait for a grace
+ * period, however, closely spaced calls to rcu_sync_enter() can
+ * optimize away the grace-period wait via a state machine implemented
+ * by rcu_sync_enter(), rcu_sync_exit(), and rcu_sync_func().
+ */
+void rcu_sync_enter(struct rcu_sync *rsp)
+{
+	int gp_state;
+
+	spin_lock_irq(&rsp->rss_lock);
+	gp_state = rsp->gp_state;
+	if (gp_state == GP_IDLE) {
+		rsp->gp_state = GP_ENTER;
+		WARN_ON_ONCE(rsp->gp_count);
+		/*
+		 * Note that we could simply do rcu_sync_call(rsp) here and
+		 * avoid the "if (gp_state == GP_IDLE)" block below.
+		 *
+		 * However, synchronize_rcu() can be faster if rcu_expedited
+		 * or rcu_blocking_is_gp() is true.
+		 *
+		 * Another reason is that we can't wait for rcu callback if
+		 * we are called at early boot time but this shouldn't happen.
+		 */
+	}
+	rsp->gp_count++;
+	spin_unlock_irq(&rsp->rss_lock);
+
+	if (gp_state == GP_IDLE) {
+		/*
+		 * See the comment above, this simply does the "synchronous"
+		 * call_rcu(rcu_sync_func) which does GP_ENTER -> GP_PASSED.
+		 */
+		synchronize_rcu();
+		rcu_sync_func(&rsp->cb_head);
+		/* Not really needed, wait_event() would see GP_PASSED. */
+		return;
+	}
+
+	wait_event(rsp->gp_wait, rsp->gp_state >= GP_PASSED);
+}
+
+/**
+ * rcu_sync_exit() - Allow readers back onto fast path after grace period
  * @rsp: Pointer to rcu_sync structure to use for synchronization
  *
  * This function is used by updaters who have completed, and can therefore
@@ -146,13 +167,16 @@ static void rcu_sync_func(struct rcu_head *rhp)
  */
 void rcu_sync_exit(struct rcu_sync *rsp)
 {
+	WARN_ON_ONCE(rsp->gp_state == GP_IDLE);
+	WARN_ON_ONCE(rsp->gp_count == 0);
+
 	spin_lock_irq(&rsp->rss_lock);
 	if (!--rsp->gp_count) {
-		if (rsp->cb_state == CB_IDLE) {
-			rsp->cb_state = CB_PENDING;
-			call_rcu(&rsp->cb_head, rcu_sync_func);
-		} else if (rsp->cb_state == CB_PENDING) {
-			rsp->cb_state = CB_REPLAY;
+		if (rsp->gp_state == GP_PASSED) {
+			rsp->gp_state = GP_EXIT;
+			rcu_sync_call(rsp);
+		} else if (rsp->gp_state == GP_EXIT) {
+			rsp->gp_state = GP_REPLAY;
 		}
 	}
 	spin_unlock_irq(&rsp->rss_lock);
@@ -164,18 +188,19 @@ void rcu_sync_exit(struct rcu_sync *rsp)
  */
 void rcu_sync_dtor(struct rcu_sync *rsp)
 {
-	int cb_state;
+	int gp_state;
 
 	WARN_ON_ONCE(rsp->gp_count);
+	WARN_ON_ONCE(rsp->gp_state == GP_PASSED);
 
 	spin_lock_irq(&rsp->rss_lock);
-	if (rsp->cb_state == CB_REPLAY)
-		rsp->cb_state = CB_PENDING;
-	cb_state = rsp->cb_state;
+	if (rsp->gp_state == GP_REPLAY)
+		rsp->gp_state = GP_EXIT;
+	gp_state = rsp->gp_state;
 	spin_unlock_irq(&rsp->rss_lock);
 
-	if (cb_state != CB_IDLE) {
+	if (gp_state != GP_IDLE) {
 		rcu_barrier();
-		WARN_ON_ONCE(rsp->cb_state != CB_IDLE);
+		WARN_ON_ONCE(rsp->gp_state != GP_IDLE);
 	}
 }
-- 
2.5.0

[PATCH 1/1] rcu/sync: simplify the state machine

[Oleg Nesterov]

With this patch rcu_sync has a single state variable and the transition rules
become really simple:

	GP_IDLE   - owned by the first rcu_sync_enter() which moves it to

	GP_ENTER  - owned by rcu-callback which moves it to

	GP_PASSED - owned by the last rcu_sync_exit() which moves it to

	GP_EXIT   - and this is the only "nontrivial" state.

		rcu-callback moves it back to GP_IDLE unless another enter()
		comes before a GP pass.

		If rcu-callback is invoked before the next rcu_sync_exit() it
		must see gp_count incremented by that enter() and set GP_PASSED.

		Otherwise, if the next rcu_sync_exit() wins the race, it will
		move it to

	GP_REPLAY - owned by rcu-callback which moves it to GP_EXIT

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 include/linux/rcu_sync.h |   2 -
 kernel/rcu/sync.c        | 165 +++++++++++++++++++++++++++--------------------
 2 files changed, 95 insertions(+), 72 deletions(-)

diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h
index e7ae221..3156a14 100644
--- a/include/linux/rcu_sync.h
+++ b/include/linux/rcu_sync.h
@@ -19,7 +19,6 @@ struct rcu_sync {
 	int			gp_count;
 	wait_queue_head_t	gp_wait;
 
-	int			cb_state;
 	struct rcu_head		cb_head;
 };
 
@@ -47,7 +46,6 @@ extern void rcu_sync_dtor(struct rcu_sync *);
 		.gp_state = 0,						\
 		.gp_count = 0,						\
 		.gp_wait = __WAIT_QUEUE_HEAD_INITIALIZER(name.gp_wait),	\
-		.cb_state = 0,						\
 	}
 
 #define	DEFINE_RCU_SYNC(name)	\
diff --git a/kernel/rcu/sync.c b/kernel/rcu/sync.c
index ee427e1..d9f80fc 100644
--- a/kernel/rcu/sync.c
+++ b/kernel/rcu/sync.c
@@ -10,15 +10,13 @@
 #include <linux/rcu_sync.h>
 #include <linux/sched.h>
 
-enum { GP_IDLE = 0, GP_PENDING, GP_PASSED };
-enum { CB_IDLE = 0, CB_PENDING, CB_REPLAY };
+enum { GP_IDLE = 0, GP_ENTER, GP_PASSED, GP_EXIT, GP_REPLAY };
 
 #define	rss_lock	gp_wait.lock
 
 /**
  * rcu_sync_init() - Initialize an rcu_sync structure
  * @rsp: Pointer to rcu_sync structure to be initialized
- * @type: Flavor of RCU with which to synchronize rcu_sync structure
  */
 void rcu_sync_init(struct rcu_sync *rsp)
 {
@@ -41,56 +39,26 @@ void rcu_sync_enter_start(struct rcu_sync *rsp)
 	rsp->gp_state = GP_PASSED;
 }
 
-/**
- * rcu_sync_enter() - Force readers onto slowpath
- * @rsp: Pointer to rcu_sync structure to use for synchronization
- *
- * This function is used by updaters who need readers to make use of
- * a slowpath during the update.  After this function returns, all
- * subsequent calls to rcu_sync_is_idle() will return false, which
- * tells readers to stay off their fastpaths.  A later call to
- * rcu_sync_exit() re-enables reader slowpaths.
- *
- * When called in isolation, rcu_sync_enter() must wait for a grace
- * period, however, closely spaced calls to rcu_sync_enter() can
- * optimize away the grace-period wait via a state machine implemented
- * by rcu_sync_enter(), rcu_sync_exit(), and rcu_sync_func().
- */
-void rcu_sync_enter(struct rcu_sync *rsp)
-{
-	bool need_wait, need_sync;
 
-	spin_lock_irq(&rsp->rss_lock);
-	need_wait = rsp->gp_count++;
-	need_sync = rsp->gp_state == GP_IDLE;
-	if (need_sync)
-		rsp->gp_state = GP_PENDING;
-	spin_unlock_irq(&rsp->rss_lock);
+static void rcu_sync_func(struct rcu_head *rcu);
 
-	WARN_ON_ONCE(need_wait && need_sync);
-	if (need_sync) {
-		synchronize_rcu();
-		rsp->gp_state = GP_PASSED;
-		wake_up_all(&rsp->gp_wait);
-	} else if (need_wait) {
-		wait_event(rsp->gp_wait, rsp->gp_state == GP_PASSED);
-	} else {
-		/*
-		 * Possible when there's a pending CB from a rcu_sync_exit().
-		 * Nobody has yet been allowed the 'fast' path and thus we can
-		 * avoid doing any sync(). The callback will get 'dropped'.
-		 */
-		WARN_ON_ONCE(rsp->gp_state != GP_PASSED);
-	}
+static void rcu_sync_call(struct rcu_sync *rsp)
+{
+	call_rcu(&rsp->cb_head, rcu_sync_func);
 }
 
 /**
  * rcu_sync_func() - Callback function managing reader access to fastpath
  * @rhp: Pointer to rcu_head in rcu_sync structure to use for synchronization
  *
- * This function is passed to one of the call_rcu() functions by
+ * This function is passed to call_rcu() function by rcu_sync_enter() and
  * rcu_sync_exit(), so that it is invoked after a grace period following the
- * that invocation of rcu_sync_exit().  It takes action based on events that
+ * that invocation of enter/exit.
+ *
+ * If it is called by rcu_sync_enter() it signals that all the readers were
+ * switched onto slow path.
+ *
+ * If it is called by rcu_sync_exit() it takes action based on events that
  * have taken place in the meantime, so that closely spaced rcu_sync_enter()
  * and rcu_sync_exit() pairs need not wait for a grace period.
  *
@@ -102,40 +70,93 @@ void rcu_sync_enter(struct rcu_sync *rsp)
  * rcu_sync_exit().  Otherwise, set all state back to idle so that readers
  * can again use their fastpaths.
  */
-static void rcu_sync_func(struct rcu_head *rhp)
+static void rcu_sync_func(struct rcu_head *rcu)
 {
-	struct rcu_sync *rsp = container_of(rhp, struct rcu_sync, cb_head);
+	struct rcu_sync *rsp = container_of(rcu, struct rcu_sync, cb_head);
 	unsigned long flags;
 
-	WARN_ON_ONCE(rsp->gp_state != GP_PASSED);
-	WARN_ON_ONCE(rsp->cb_state == CB_IDLE);
+	WARN_ON_ONCE(rsp->gp_state == GP_IDLE);
+	WARN_ON_ONCE(rsp->gp_state == GP_PASSED);
 
 	spin_lock_irqsave(&rsp->rss_lock, flags);
 	if (rsp->gp_count) {
 		/*
-		 * A new rcu_sync_begin() has happened; drop the callback.
+		 * We're at least a GP after the GP_IDLE->GP_ENTER transition.
 		 */
-		rsp->cb_state = CB_IDLE;
-	} else if (rsp->cb_state == CB_REPLAY) {
+		rsp->gp_state = GP_PASSED;
+		wake_up_locked(&rsp->gp_wait);
+	} else if (rsp->gp_state == GP_REPLAY) {
 		/*
-		 * A new rcu_sync_exit() has happened; requeue the callback
-		 * to catch a later GP.
+		 * A new rcu_sync_exit() has happened; requeue the callback to
+		 * catch a later GP.
 		 */
-		rsp->cb_state = CB_PENDING;
-		call_rcu(&rsp->cb_head, rcu_sync_func);
+		rsp->gp_state = GP_EXIT;
+		rcu_sync_call(rsp);
 	} else {
 		/*
-		 * We're at least a GP after rcu_sync_exit(); eveybody will now
-		 * have observed the write side critical section. Let 'em rip!.
+		 * We're at least a GP after the last rcu_sync_exit(); eveybody
+		 * will now have observed the write side critical section.
+		 * Let 'em rip!.
 		 */
-		rsp->cb_state = CB_IDLE;
 		rsp->gp_state = GP_IDLE;
 	}
 	spin_unlock_irqrestore(&rsp->rss_lock, flags);
 }
 
 /**
- * rcu_sync_exit() - Allow readers back onto fast patch after grace period
+ * rcu_sync_enter() - Force readers onto slowpath
+ * @rsp: Pointer to rcu_sync structure to use for synchronization
+ *
+ * This function is used by updaters who need readers to make use of
+ * a slowpath during the update.  After this function returns, all
+ * subsequent calls to rcu_sync_is_idle() will return false, which
+ * tells readers to stay off their fastpaths.  A later call to
+ * rcu_sync_exit() re-enables reader slowpaths.
+ *
+ * When called in isolation, rcu_sync_enter() must wait for a grace
+ * period, however, closely spaced calls to rcu_sync_enter() can
+ * optimize away the grace-period wait via a state machine implemented
+ * by rcu_sync_enter(), rcu_sync_exit(), and rcu_sync_func().
+ */
+void rcu_sync_enter(struct rcu_sync *rsp)
+{
+	int gp_state;
+
+	spin_lock_irq(&rsp->rss_lock);
+	gp_state = rsp->gp_state;
+	if (gp_state == GP_IDLE) {
+		rsp->gp_state = GP_ENTER;
+		WARN_ON_ONCE(rsp->gp_count);
+		/*
+		 * Note that we could simply do rcu_sync_call(rsp) here and
+		 * avoid the "if (gp_state == GP_IDLE)" block below.
+		 *
+		 * However, synchronize_rcu() can be faster if rcu_expedited
+		 * or rcu_blocking_is_gp() is true.
+		 *
+		 * Another reason is that we can't wait for rcu callback if
+		 * we are called at early boot time but this shouldn't happen.
+		 */
+	}
+	rsp->gp_count++;
+	spin_unlock_irq(&rsp->rss_lock);
+
+	if (gp_state == GP_IDLE) {
+		/*
+		 * See the comment above, this simply does the "synchronous"
+		 * call_rcu(rcu_sync_func) which does GP_ENTER -> GP_PASSED.
+		 */
+		synchronize_rcu();
+		rcu_sync_func(&rsp->cb_head);
+		/* Not really needed, wait_event() would see GP_PASSED. */
+		return;
+	}
+
+	wait_event(rsp->gp_wait, rsp->gp_state >= GP_PASSED);
+}
+
+/**
+ * rcu_sync_exit() - Allow readers back onto fast path after grace period
  * @rsp: Pointer to rcu_sync structure to use for synchronization
  *
  * This function is used by updaters who have completed, and can therefore
@@ -146,13 +167,16 @@ static void rcu_sync_func(struct rcu_head *rhp)
  */
 void rcu_sync_exit(struct rcu_sync *rsp)
 {
+	WARN_ON_ONCE(rsp->gp_state == GP_IDLE);
+	WARN_ON_ONCE(rsp->gp_count == 0);
+
 	spin_lock_irq(&rsp->rss_lock);
 	if (!--rsp->gp_count) {
-		if (rsp->cb_state == CB_IDLE) {
-			rsp->cb_state = CB_PENDING;
-			call_rcu(&rsp->cb_head, rcu_sync_func);
-		} else if (rsp->cb_state == CB_PENDING) {
-			rsp->cb_state = CB_REPLAY;
+		if (rsp->gp_state == GP_PASSED) {
+			rsp->gp_state = GP_EXIT;
+			rcu_sync_call(rsp);
+		} else if (rsp->gp_state == GP_EXIT) {
+			rsp->gp_state = GP_REPLAY;
 		}
 	}
 	spin_unlock_irq(&rsp->rss_lock);
@@ -164,18 +188,19 @@ void rcu_sync_exit(struct rcu_sync *rsp)
  */
 void rcu_sync_dtor(struct rcu_sync *rsp)
 {
-	int cb_state;
+	int gp_state;
 
 	WARN_ON_ONCE(rsp->gp_count);
+	WARN_ON_ONCE(rsp->gp_state == GP_PASSED);
 
 	spin_lock_irq(&rsp->rss_lock);
-	if (rsp->cb_state == CB_REPLAY)
-		rsp->cb_state = CB_PENDING;
-	cb_state = rsp->cb_state;
+	if (rsp->gp_state == GP_REPLAY)
+		rsp->gp_state = GP_EXIT;
+	gp_state = rsp->gp_state;
 	spin_unlock_irq(&rsp->rss_lock);
 
-	if (cb_state != CB_IDLE) {
+	if (gp_state != GP_IDLE) {
 		rcu_barrier();
-		WARN_ON_ONCE(rsp->cb_state != CB_IDLE);
+		WARN_ON_ONCE(rsp->gp_state != GP_IDLE);
 	}
 }
-- 
2.5.0

Re: [PATCH 1/1] rcu/sync: simplify the state machine

[Paul E. McKenney]

On Thu, Apr 25, 2019 at 06:50:55PM +0200, Oleg Nesterov wrote:
> With this patch rcu_sync has a single state variable and the transition rules
> become really simple:
> 
> 	GP_IDLE   - owned by the first rcu_sync_enter() which moves it to
> 
> 	GP_ENTER  - owned by rcu-callback which moves it to
> 
> 	GP_PASSED - owned by the last rcu_sync_exit() which moves it to
> 
> 	GP_EXIT   - and this is the only "nontrivial" state.
> 
> 		rcu-callback moves it back to GP_IDLE unless another enter()
> 		comes before a GP pass.
> 
> 		If rcu-callback is invoked before the next rcu_sync_exit() it
> 		must see gp_count incremented by that enter() and set GP_PASSED.
> 
> 		Otherwise, if the next rcu_sync_exit() wins the race, it will
> 		move it to
> 
> 	GP_REPLAY - owned by rcu-callback which moves it to GP_EXIT
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Queued and passing initial tests, thank you!  It may be a day or two
before it shows up on -rcu, but it will get there!

							Thanx, Paul

> ---
>  include/linux/rcu_sync.h |   2 -
>  kernel/rcu/sync.c        | 165 +++++++++++++++++++++++++++--------------------
>  2 files changed, 95 insertions(+), 72 deletions(-)
> 
> diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h
> index e7ae221..3156a14 100644
> --- a/include/linux/rcu_sync.h
> +++ b/include/linux/rcu_sync.h
> @@ -19,7 +19,6 @@ struct rcu_sync {
>  	int			gp_count;
>  	wait_queue_head_t	gp_wait;
>  
> -	int			cb_state;
>  	struct rcu_head		cb_head;
>  };
>  
> @@ -47,7 +46,6 @@ extern void rcu_sync_dtor(struct rcu_sync *);
>  		.gp_state = 0,						\
>  		.gp_count = 0,						\
>  		.gp_wait = __WAIT_QUEUE_HEAD_INITIALIZER(name.gp_wait),	\
> -		.cb_state = 0,						\
>  	}
>  
>  #define	DEFINE_RCU_SYNC(name)	\
> diff --git a/kernel/rcu/sync.c b/kernel/rcu/sync.c
> index ee427e1..d9f80fc 100644
> --- a/kernel/rcu/sync.c
> +++ b/kernel/rcu/sync.c
> @@ -10,15 +10,13 @@
>  #include <linux/rcu_sync.h>
>  #include <linux/sched.h>
>  
> -enum { GP_IDLE = 0, GP_PENDING, GP_PASSED };
> -enum { CB_IDLE = 0, CB_PENDING, CB_REPLAY };
> +enum { GP_IDLE = 0, GP_ENTER, GP_PASSED, GP_EXIT, GP_REPLAY };
>  
>  #define	rss_lock	gp_wait.lock
>  
>  /**
>   * rcu_sync_init() - Initialize an rcu_sync structure
>   * @rsp: Pointer to rcu_sync structure to be initialized
> - * @type: Flavor of RCU with which to synchronize rcu_sync structure
>   */
>  void rcu_sync_init(struct rcu_sync *rsp)
>  {
> @@ -41,56 +39,26 @@ void rcu_sync_enter_start(struct rcu_sync *rsp)
>  	rsp->gp_state = GP_PASSED;
>  }
>  
> -/**
> - * rcu_sync_enter() - Force readers onto slowpath
> - * @rsp: Pointer to rcu_sync structure to use for synchronization
> - *
> - * This function is used by updaters who need readers to make use of
> - * a slowpath during the update.  After this function returns, all
> - * subsequent calls to rcu_sync_is_idle() will return false, which
> - * tells readers to stay off their fastpaths.  A later call to
> - * rcu_sync_exit() re-enables reader slowpaths.
> - *
> - * When called in isolation, rcu_sync_enter() must wait for a grace
> - * period, however, closely spaced calls to rcu_sync_enter() can
> - * optimize away the grace-period wait via a state machine implemented
> - * by rcu_sync_enter(), rcu_sync_exit(), and rcu_sync_func().
> - */
> -void rcu_sync_enter(struct rcu_sync *rsp)
> -{
> -	bool need_wait, need_sync;
>  
> -	spin_lock_irq(&rsp->rss_lock);
> -	need_wait = rsp->gp_count++;
> -	need_sync = rsp->gp_state == GP_IDLE;
> -	if (need_sync)
> -		rsp->gp_state = GP_PENDING;
> -	spin_unlock_irq(&rsp->rss_lock);
> +static void rcu_sync_func(struct rcu_head *rcu);
>  
> -	WARN_ON_ONCE(need_wait && need_sync);
> -	if (need_sync) {
> -		synchronize_rcu();
> -		rsp->gp_state = GP_PASSED;
> -		wake_up_all(&rsp->gp_wait);
> -	} else if (need_wait) {
> -		wait_event(rsp->gp_wait, rsp->gp_state == GP_PASSED);
> -	} else {
> -		/*
> -		 * Possible when there's a pending CB from a rcu_sync_exit().
> -		 * Nobody has yet been allowed the 'fast' path and thus we can
> -		 * avoid doing any sync(). The callback will get 'dropped'.
> -		 */
> -		WARN_ON_ONCE(rsp->gp_state != GP_PASSED);
> -	}
> +static void rcu_sync_call(struct rcu_sync *rsp)
> +{
> +	call_rcu(&rsp->cb_head, rcu_sync_func);
>  }
>  
>  /**
>   * rcu_sync_func() - Callback function managing reader access to fastpath
>   * @rhp: Pointer to rcu_head in rcu_sync structure to use for synchronization
>   *
> - * This function is passed to one of the call_rcu() functions by
> + * This function is passed to call_rcu() function by rcu_sync_enter() and
>   * rcu_sync_exit(), so that it is invoked after a grace period following the
> - * that invocation of rcu_sync_exit().  It takes action based on events that
> + * that invocation of enter/exit.
> + *
> + * If it is called by rcu_sync_enter() it signals that all the readers were
> + * switched onto slow path.
> + *
> + * If it is called by rcu_sync_exit() it takes action based on events that
>   * have taken place in the meantime, so that closely spaced rcu_sync_enter()
>   * and rcu_sync_exit() pairs need not wait for a grace period.
>   *
> @@ -102,40 +70,93 @@ void rcu_sync_enter(struct rcu_sync *rsp)
>   * rcu_sync_exit().  Otherwise, set all state back to idle so that readers
>   * can again use their fastpaths.
>   */
> -static void rcu_sync_func(struct rcu_head *rhp)
> +static void rcu_sync_func(struct rcu_head *rcu)
>  {
> -	struct rcu_sync *rsp = container_of(rhp, struct rcu_sync, cb_head);
> +	struct rcu_sync *rsp = container_of(rcu, struct rcu_sync, cb_head);
>  	unsigned long flags;
>  
> -	WARN_ON_ONCE(rsp->gp_state != GP_PASSED);
> -	WARN_ON_ONCE(rsp->cb_state == CB_IDLE);
> +	WARN_ON_ONCE(rsp->gp_state == GP_IDLE);
> +	WARN_ON_ONCE(rsp->gp_state == GP_PASSED);
>  
>  	spin_lock_irqsave(&rsp->rss_lock, flags);
>  	if (rsp->gp_count) {
>  		/*
> -		 * A new rcu_sync_begin() has happened; drop the callback.
> +		 * We're at least a GP after the GP_IDLE->GP_ENTER transition.
>  		 */
> -		rsp->cb_state = CB_IDLE;
> -	} else if (rsp->cb_state == CB_REPLAY) {
> +		rsp->gp_state = GP_PASSED;
> +		wake_up_locked(&rsp->gp_wait);
> +	} else if (rsp->gp_state == GP_REPLAY) {
>  		/*
> -		 * A new rcu_sync_exit() has happened; requeue the callback
> -		 * to catch a later GP.
> +		 * A new rcu_sync_exit() has happened; requeue the callback to
> +		 * catch a later GP.
>  		 */
> -		rsp->cb_state = CB_PENDING;
> -		call_rcu(&rsp->cb_head, rcu_sync_func);
> +		rsp->gp_state = GP_EXIT;
> +		rcu_sync_call(rsp);
>  	} else {
>  		/*
> -		 * We're at least a GP after rcu_sync_exit(); eveybody will now
> -		 * have observed the write side critical section. Let 'em rip!.
> +		 * We're at least a GP after the last rcu_sync_exit(); eveybody
> +		 * will now have observed the write side critical section.
> +		 * Let 'em rip!.
>  		 */
> -		rsp->cb_state = CB_IDLE;
>  		rsp->gp_state = GP_IDLE;
>  	}
>  	spin_unlock_irqrestore(&rsp->rss_lock, flags);
>  }
>  
>  /**
> - * rcu_sync_exit() - Allow readers back onto fast patch after grace period
> + * rcu_sync_enter() - Force readers onto slowpath
> + * @rsp: Pointer to rcu_sync structure to use for synchronization
> + *
> + * This function is used by updaters who need readers to make use of
> + * a slowpath during the update.  After this function returns, all
> + * subsequent calls to rcu_sync_is_idle() will return false, which
> + * tells readers to stay off their fastpaths.  A later call to
> + * rcu_sync_exit() re-enables reader slowpaths.
> + *
> + * When called in isolation, rcu_sync_enter() must wait for a grace
> + * period, however, closely spaced calls to rcu_sync_enter() can
> + * optimize away the grace-period wait via a state machine implemented
> + * by rcu_sync_enter(), rcu_sync_exit(), and rcu_sync_func().
> + */
> +void rcu_sync_enter(struct rcu_sync *rsp)
> +{
> +	int gp_state;
> +
> +	spin_lock_irq(&rsp->rss_lock);
> +	gp_state = rsp->gp_state;
> +	if (gp_state == GP_IDLE) {
> +		rsp->gp_state = GP_ENTER;
> +		WARN_ON_ONCE(rsp->gp_count);
> +		/*
> +		 * Note that we could simply do rcu_sync_call(rsp) here and
> +		 * avoid the "if (gp_state == GP_IDLE)" block below.
> +		 *
> +		 * However, synchronize_rcu() can be faster if rcu_expedited
> +		 * or rcu_blocking_is_gp() is true.
> +		 *
> +		 * Another reason is that we can't wait for rcu callback if
> +		 * we are called at early boot time but this shouldn't happen.
> +		 */
> +	}
> +	rsp->gp_count++;
> +	spin_unlock_irq(&rsp->rss_lock);
> +
> +	if (gp_state == GP_IDLE) {
> +		/*
> +		 * See the comment above, this simply does the "synchronous"
> +		 * call_rcu(rcu_sync_func) which does GP_ENTER -> GP_PASSED.
> +		 */
> +		synchronize_rcu();
> +		rcu_sync_func(&rsp->cb_head);
> +		/* Not really needed, wait_event() would see GP_PASSED. */
> +		return;
> +	}
> +
> +	wait_event(rsp->gp_wait, rsp->gp_state >= GP_PASSED);
> +}
> +
> +/**
> + * rcu_sync_exit() - Allow readers back onto fast path after grace period
>   * @rsp: Pointer to rcu_sync structure to use for synchronization
>   *
>   * This function is used by updaters who have completed, and can therefore
> @@ -146,13 +167,16 @@ static void rcu_sync_func(struct rcu_head *rhp)
>   */
>  void rcu_sync_exit(struct rcu_sync *rsp)
>  {
> +	WARN_ON_ONCE(rsp->gp_state == GP_IDLE);
> +	WARN_ON_ONCE(rsp->gp_count == 0);
> +
>  	spin_lock_irq(&rsp->rss_lock);
>  	if (!--rsp->gp_count) {
> -		if (rsp->cb_state == CB_IDLE) {
> -			rsp->cb_state = CB_PENDING;
> -			call_rcu(&rsp->cb_head, rcu_sync_func);
> -		} else if (rsp->cb_state == CB_PENDING) {
> -			rsp->cb_state = CB_REPLAY;
> +		if (rsp->gp_state == GP_PASSED) {
> +			rsp->gp_state = GP_EXIT;
> +			rcu_sync_call(rsp);
> +		} else if (rsp->gp_state == GP_EXIT) {
> +			rsp->gp_state = GP_REPLAY;
>  		}
>  	}
>  	spin_unlock_irq(&rsp->rss_lock);
> @@ -164,18 +188,19 @@ void rcu_sync_exit(struct rcu_sync *rsp)
>   */
>  void rcu_sync_dtor(struct rcu_sync *rsp)
>  {
> -	int cb_state;
> +	int gp_state;
>  
>  	WARN_ON_ONCE(rsp->gp_count);
> +	WARN_ON_ONCE(rsp->gp_state == GP_PASSED);
>  
>  	spin_lock_irq(&rsp->rss_lock);
> -	if (rsp->cb_state == CB_REPLAY)
> -		rsp->cb_state = CB_PENDING;
> -	cb_state = rsp->cb_state;
> +	if (rsp->gp_state == GP_REPLAY)
> +		rsp->gp_state = GP_EXIT;
> +	gp_state = rsp->gp_state;
>  	spin_unlock_irq(&rsp->rss_lock);
>  
> -	if (cb_state != CB_IDLE) {
> +	if (gp_state != GP_IDLE) {
>  		rcu_barrier();
> -		WARN_ON_ONCE(rsp->cb_state != CB_IDLE);
> +		WARN_ON_ONCE(rsp->gp_state != GP_IDLE);
>  	}
>  }
> -- 
> 2.5.0
> 
> 

Re: [PATCH 1/1] rcu/sync: simplify the state machine

[Paul E. McKenney]

On Sat, Apr 27, 2019 at 02:02:30PM -0700, Paul E. McKenney wrote:
> On Thu, Apr 25, 2019 at 06:50:55PM +0200, Oleg Nesterov wrote:
> > With this patch rcu_sync has a single state variable and the transition rules
> > become really simple:
> > 
> > 	GP_IDLE   - owned by the first rcu_sync_enter() which moves it to
> > 
> > 	GP_ENTER  - owned by rcu-callback which moves it to
> > 
> > 	GP_PASSED - owned by the last rcu_sync_exit() which moves it to
> > 
> > 	GP_EXIT   - and this is the only "nontrivial" state.
> > 
> > 		rcu-callback moves it back to GP_IDLE unless another enter()
> > 		comes before a GP pass.
> > 
> > 		If rcu-callback is invoked before the next rcu_sync_exit() it
> > 		must see gp_count incremented by that enter() and set GP_PASSED.
> > 
> > 		Otherwise, if the next rcu_sync_exit() wins the race, it will
> > 		move it to
> > 
> > 	GP_REPLAY - owned by rcu-callback which moves it to GP_EXIT
> > 
> > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> 
> Queued and passing initial tests, thank you!  It may be a day or two
> before it shows up on -rcu, but it will get there!

And it still looks good after review, so I have pushed it.  I did add
READ_ONCE() and WRITE_ONCE() to unprotected uses of ->gp_state, but
please let me know if I messed anything up.

							Thanx, Paul

> > ---
> >  include/linux/rcu_sync.h |   2 -
> >  kernel/rcu/sync.c        | 165 +++++++++++++++++++++++++++--------------------
> >  2 files changed, 95 insertions(+), 72 deletions(-)
> > 
> > diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h
> > index e7ae221..3156a14 100644
> > --- a/include/linux/rcu_sync.h
> > +++ b/include/linux/rcu_sync.h
> > @@ -19,7 +19,6 @@ struct rcu_sync {
> >  	int			gp_count;
> >  	wait_queue_head_t	gp_wait;
> >  
> > -	int			cb_state;
> >  	struct rcu_head		cb_head;
> >  };
> >  
> > @@ -47,7 +46,6 @@ extern void rcu_sync_dtor(struct rcu_sync *);
> >  		.gp_state = 0,						\
> >  		.gp_count = 0,						\
> >  		.gp_wait = __WAIT_QUEUE_HEAD_INITIALIZER(name.gp_wait),	\
> > -		.cb_state = 0,						\
> >  	}
> >  
> >  #define	DEFINE_RCU_SYNC(name)	\
> > diff --git a/kernel/rcu/sync.c b/kernel/rcu/sync.c
> > index ee427e1..d9f80fc 100644
> > --- a/kernel/rcu/sync.c
> > +++ b/kernel/rcu/sync.c
> > @@ -10,15 +10,13 @@
> >  #include <linux/rcu_sync.h>
> >  #include <linux/sched.h>
> >  
> > -enum { GP_IDLE = 0, GP_PENDING, GP_PASSED };
> > -enum { CB_IDLE = 0, CB_PENDING, CB_REPLAY };
> > +enum { GP_IDLE = 0, GP_ENTER, GP_PASSED, GP_EXIT, GP_REPLAY };
> >  
> >  #define	rss_lock	gp_wait.lock
> >  
> >  /**
> >   * rcu_sync_init() - Initialize an rcu_sync structure
> >   * @rsp: Pointer to rcu_sync structure to be initialized
> > - * @type: Flavor of RCU with which to synchronize rcu_sync structure
> >   */
> >  void rcu_sync_init(struct rcu_sync *rsp)
> >  {
> > @@ -41,56 +39,26 @@ void rcu_sync_enter_start(struct rcu_sync *rsp)
> >  	rsp->gp_state = GP_PASSED;
> >  }
> >  
> > -/**
> > - * rcu_sync_enter() - Force readers onto slowpath
> > - * @rsp: Pointer to rcu_sync structure to use for synchronization
> > - *
> > - * This function is used by updaters who need readers to make use of
> > - * a slowpath during the update.  After this function returns, all
> > - * subsequent calls to rcu_sync_is_idle() will return false, which
> > - * tells readers to stay off their fastpaths.  A later call to
> > - * rcu_sync_exit() re-enables reader slowpaths.
> > - *
> > - * When called in isolation, rcu_sync_enter() must wait for a grace
> > - * period, however, closely spaced calls to rcu_sync_enter() can
> > - * optimize away the grace-period wait via a state machine implemented
> > - * by rcu_sync_enter(), rcu_sync_exit(), and rcu_sync_func().
> > - */
> > -void rcu_sync_enter(struct rcu_sync *rsp)
> > -{
> > -	bool need_wait, need_sync;
> >  
> > -	spin_lock_irq(&rsp->rss_lock);
> > -	need_wait = rsp->gp_count++;
> > -	need_sync = rsp->gp_state == GP_IDLE;
> > -	if (need_sync)
> > -		rsp->gp_state = GP_PENDING;
> > -	spin_unlock_irq(&rsp->rss_lock);
> > +static void rcu_sync_func(struct rcu_head *rcu);
> >  
> > -	WARN_ON_ONCE(need_wait && need_sync);
> > -	if (need_sync) {
> > -		synchronize_rcu();
> > -		rsp->gp_state = GP_PASSED;
> > -		wake_up_all(&rsp->gp_wait);
> > -	} else if (need_wait) {
> > -		wait_event(rsp->gp_wait, rsp->gp_state == GP_PASSED);
> > -	} else {
> > -		/*
> > -		 * Possible when there's a pending CB from a rcu_sync_exit().
> > -		 * Nobody has yet been allowed the 'fast' path and thus we can
> > -		 * avoid doing any sync(). The callback will get 'dropped'.
> > -		 */
> > -		WARN_ON_ONCE(rsp->gp_state != GP_PASSED);
> > -	}
> > +static void rcu_sync_call(struct rcu_sync *rsp)
> > +{
> > +	call_rcu(&rsp->cb_head, rcu_sync_func);
> >  }
> >  
> >  /**
> >   * rcu_sync_func() - Callback function managing reader access to fastpath
> >   * @rhp: Pointer to rcu_head in rcu_sync structure to use for synchronization
> >   *
> > - * This function is passed to one of the call_rcu() functions by
> > + * This function is passed to call_rcu() function by rcu_sync_enter() and
> >   * rcu_sync_exit(), so that it is invoked after a grace period following the
> > - * that invocation of rcu_sync_exit().  It takes action based on events that
> > + * that invocation of enter/exit.
> > + *
> > + * If it is called by rcu_sync_enter() it signals that all the readers were
> > + * switched onto slow path.
> > + *
> > + * If it is called by rcu_sync_exit() it takes action based on events that
> >   * have taken place in the meantime, so that closely spaced rcu_sync_enter()
> >   * and rcu_sync_exit() pairs need not wait for a grace period.
> >   *
> > @@ -102,40 +70,93 @@ void rcu_sync_enter(struct rcu_sync *rsp)
> >   * rcu_sync_exit().  Otherwise, set all state back to idle so that readers
> >   * can again use their fastpaths.
> >   */
> > -static void rcu_sync_func(struct rcu_head *rhp)
> > +static void rcu_sync_func(struct rcu_head *rcu)
> >  {
> > -	struct rcu_sync *rsp = container_of(rhp, struct rcu_sync, cb_head);
> > +	struct rcu_sync *rsp = container_of(rcu, struct rcu_sync, cb_head);
> >  	unsigned long flags;
> >  
> > -	WARN_ON_ONCE(rsp->gp_state != GP_PASSED);
> > -	WARN_ON_ONCE(rsp->cb_state == CB_IDLE);
> > +	WARN_ON_ONCE(rsp->gp_state == GP_IDLE);
> > +	WARN_ON_ONCE(rsp->gp_state == GP_PASSED);
> >  
> >  	spin_lock_irqsave(&rsp->rss_lock, flags);
> >  	if (rsp->gp_count) {
> >  		/*
> > -		 * A new rcu_sync_begin() has happened; drop the callback.
> > +		 * We're at least a GP after the GP_IDLE->GP_ENTER transition.
> >  		 */
> > -		rsp->cb_state = CB_IDLE;
> > -	} else if (rsp->cb_state == CB_REPLAY) {
> > +		rsp->gp_state = GP_PASSED;
> > +		wake_up_locked(&rsp->gp_wait);
> > +	} else if (rsp->gp_state == GP_REPLAY) {
> >  		/*
> > -		 * A new rcu_sync_exit() has happened; requeue the callback
> > -		 * to catch a later GP.
> > +		 * A new rcu_sync_exit() has happened; requeue the callback to
> > +		 * catch a later GP.
> >  		 */
> > -		rsp->cb_state = CB_PENDING;
> > -		call_rcu(&rsp->cb_head, rcu_sync_func);
> > +		rsp->gp_state = GP_EXIT;
> > +		rcu_sync_call(rsp);
> >  	} else {
> >  		/*
> > -		 * We're at least a GP after rcu_sync_exit(); eveybody will now
> > -		 * have observed the write side critical section. Let 'em rip!.
> > +		 * We're at least a GP after the last rcu_sync_exit(); eveybody
> > +		 * will now have observed the write side critical section.
> > +		 * Let 'em rip!.
> >  		 */
> > -		rsp->cb_state = CB_IDLE;
> >  		rsp->gp_state = GP_IDLE;
> >  	}
> >  	spin_unlock_irqrestore(&rsp->rss_lock, flags);
> >  }
> >  
> >  /**
> > - * rcu_sync_exit() - Allow readers back onto fast patch after grace period
> > + * rcu_sync_enter() - Force readers onto slowpath
> > + * @rsp: Pointer to rcu_sync structure to use for synchronization
> > + *
> > + * This function is used by updaters who need readers to make use of
> > + * a slowpath during the update.  After this function returns, all
> > + * subsequent calls to rcu_sync_is_idle() will return false, which
> > + * tells readers to stay off their fastpaths.  A later call to
> > + * rcu_sync_exit() re-enables reader slowpaths.
> > + *
> > + * When called in isolation, rcu_sync_enter() must wait for a grace
> > + * period, however, closely spaced calls to rcu_sync_enter() can
> > + * optimize away the grace-period wait via a state machine implemented
> > + * by rcu_sync_enter(), rcu_sync_exit(), and rcu_sync_func().
> > + */
> > +void rcu_sync_enter(struct rcu_sync *rsp)
> > +{
> > +	int gp_state;
> > +
> > +	spin_lock_irq(&rsp->rss_lock);
> > +	gp_state = rsp->gp_state;
> > +	if (gp_state == GP_IDLE) {
> > +		rsp->gp_state = GP_ENTER;
> > +		WARN_ON_ONCE(rsp->gp_count);
> > +		/*
> > +		 * Note that we could simply do rcu_sync_call(rsp) here and
> > +		 * avoid the "if (gp_state == GP_IDLE)" block below.
> > +		 *
> > +		 * However, synchronize_rcu() can be faster if rcu_expedited
> > +		 * or rcu_blocking_is_gp() is true.
> > +		 *
> > +		 * Another reason is that we can't wait for rcu callback if
> > +		 * we are called at early boot time but this shouldn't happen.
> > +		 */
> > +	}
> > +	rsp->gp_count++;
> > +	spin_unlock_irq(&rsp->rss_lock);
> > +
> > +	if (gp_state == GP_IDLE) {
> > +		/*
> > +		 * See the comment above, this simply does the "synchronous"
> > +		 * call_rcu(rcu_sync_func) which does GP_ENTER -> GP_PASSED.
> > +		 */
> > +		synchronize_rcu();
> > +		rcu_sync_func(&rsp->cb_head);
> > +		/* Not really needed, wait_event() would see GP_PASSED. */
> > +		return;
> > +	}
> > +
> > +	wait_event(rsp->gp_wait, rsp->gp_state >= GP_PASSED);
> > +}
> > +
> > +/**
> > + * rcu_sync_exit() - Allow readers back onto fast path after grace period
> >   * @rsp: Pointer to rcu_sync structure to use for synchronization
> >   *
> >   * This function is used by updaters who have completed, and can therefore
> > @@ -146,13 +167,16 @@ static void rcu_sync_func(struct rcu_head *rhp)
> >   */
> >  void rcu_sync_exit(struct rcu_sync *rsp)
> >  {
> > +	WARN_ON_ONCE(rsp->gp_state == GP_IDLE);
> > +	WARN_ON_ONCE(rsp->gp_count == 0);
> > +
> >  	spin_lock_irq(&rsp->rss_lock);
> >  	if (!--rsp->gp_count) {
> > -		if (rsp->cb_state == CB_IDLE) {
> > -			rsp->cb_state = CB_PENDING;
> > -			call_rcu(&rsp->cb_head, rcu_sync_func);
> > -		} else if (rsp->cb_state == CB_PENDING) {
> > -			rsp->cb_state = CB_REPLAY;
> > +		if (rsp->gp_state == GP_PASSED) {
> > +			rsp->gp_state = GP_EXIT;
> > +			rcu_sync_call(rsp);
> > +		} else if (rsp->gp_state == GP_EXIT) {
> > +			rsp->gp_state = GP_REPLAY;
> >  		}
> >  	}
> >  	spin_unlock_irq(&rsp->rss_lock);
> > @@ -164,18 +188,19 @@ void rcu_sync_exit(struct rcu_sync *rsp)
> >   */
> >  void rcu_sync_dtor(struct rcu_sync *rsp)
> >  {
> > -	int cb_state;
> > +	int gp_state;
> >  
> >  	WARN_ON_ONCE(rsp->gp_count);
> > +	WARN_ON_ONCE(rsp->gp_state == GP_PASSED);
> >  
> >  	spin_lock_irq(&rsp->rss_lock);
> > -	if (rsp->cb_state == CB_REPLAY)
> > -		rsp->cb_state = CB_PENDING;
> > -	cb_state = rsp->cb_state;
> > +	if (rsp->gp_state == GP_REPLAY)
> > +		rsp->gp_state = GP_EXIT;
> > +	gp_state = rsp->gp_state;
> >  	spin_unlock_irq(&rsp->rss_lock);
> >  
> > -	if (cb_state != CB_IDLE) {
> > +	if (gp_state != GP_IDLE) {
> >  		rcu_barrier();
> > -		WARN_ON_ONCE(rsp->cb_state != CB_IDLE);
> > +		WARN_ON_ONCE(rsp->gp_state != GP_IDLE);
> >  	}
> >  }
> > -- 
> > 2.5.0
> > 
> > 

Re: [PATCH 1/1] rcu/sync: simplify the state machine

[Oleg Nesterov]

On 04/28, Paul E. McKenney wrote:
>
> And it still looks good after review, so I have pushed it.

Thanks!

> I did add
> READ_ONCE() and WRITE_ONCE() to unprotected uses of ->gp_state, but
> please let me know if I messed anything up.

Well, at least WRITE_ONCE()'s look certainly unneeded to me, gp_state
is protected by rss_lock.

WARN_ON_ONCE(gp_state) can read gp_state lockless, but even in this case
I do not understand what READ_ONCE() tries to prevent...

Nevermind, this won't hurt and as I already said I don't understand the
_ONCE() magic anyway ;)

Thanks,

Oleg.

Re: [PATCH 1/1] rcu/sync: simplify the state machine

[Paul E. McKenney]

On Mon, Apr 29, 2019 at 06:06:04PM +0200, Oleg Nesterov wrote:
> On 04/28, Paul E. McKenney wrote:
> >
> > And it still looks good after review, so I have pushed it.
> 
> Thanks!
> 
> > I did add
> > READ_ONCE() and WRITE_ONCE() to unprotected uses of ->gp_state, but
> > please let me know if I messed anything up.
> 
> Well, at least WRITE_ONCE()'s look certainly unneeded to me, gp_state
> is protected by rss_lock.
> 
> WARN_ON_ONCE(gp_state) can read gp_state lockless, but even in this case
> I do not understand what READ_ONCE() tries to prevent...
> 
> Nevermind, this won't hurt and as I already said I don't understand the
> _ONCE() magic anyway ;)

If I understand correctly, rcu_sync_is_idle() can be inline and returns
->gp_state.  Without the READ_ONCE(), the compiler might fuse reads from
consecutive calls to rcu_sync_is_idle() or (under register pressure)
re-read from it, getting inconsistent results.  For example, this:

	tmp = rcu_sync_is_idle(rsp);
	do_something(tmp);
	do_something_else(tmp);

Might become this:

	do_something(rcu_sync_is_idle(rsp));
	do_something_else(rcu_sync_is_idle(rsp));

This might actually be harmless given current calls, but it would be at
best an accident waiting to happen.

Or am I missing something here?

							Thanx, Paul

Re: [PATCH 1/1] rcu/sync: simplify the state machine

[Oleg Nesterov]

On 04/29, Paul E. McKenney wrote:
>
> On Mon, Apr 29, 2019 at 06:06:04PM +0200, Oleg Nesterov wrote:
> >
> > Well, at least WRITE_ONCE()'s look certainly unneeded to me, gp_state
> > is protected by rss_lock.
> >
> > WARN_ON_ONCE(gp_state) can read gp_state lockless, but even in this case
> > I do not understand what READ_ONCE() tries to prevent...
> >
> > Nevermind, this won't hurt and as I already said I don't understand the
> > _ONCE() magic anyway ;)
>
> If I understand correctly, rcu_sync_is_idle() can be inline and returns
> ->gp_state.

Ah, sorry! I didn't mean rcu_sync_is_idle(). To be honeest, I didn't even
notice this change, but it looks obviously fine to me, with or without this
patch.

And yes,

> Without the READ_ONCE(), the compiler might fuse reads from
> consecutive calls to rcu_sync_is_idle() or (under register pressure)
> re-read from it, getting inconsistent results.  For example, this:
>
> 	tmp = rcu_sync_is_idle(rsp);
> 	do_something(tmp);
> 	do_something_else(tmp);
>
> Might become this:
>
> 	do_something(rcu_sync_is_idle(rsp));
> 	do_something_else(rcu_sync_is_idle(rsp));


this is very clear. Even for me ;)

Thanks,

Oleg.