locking/percpu-rwsem: Optimizations/tweaks

Currently the percpu-rwsem switches to (global) atomic ops while a writer is waiting; which could be quite a while and slows down releasing the readers.

This patch cures this problem by ordering the reader-state vs reader-count (see the comments in __percpu_down_read() and percpu_down_write()). This changes a global atomic op into a full memory barrier, which doesn't have the global cacheline contention.

Quoted 9 lines · Peter Zijlstra

Currently the percpu-rwsem switches to (global) atomic ops while a writer is waiting; which could be quite a while and slows down releasing the readers.

This patch cures this problem by ordering the reader-state vs reader-count (see the comments in __percpu_down_read() and percpu_down_write()). This changes a global atomic op into a full memory barrier, which doesn't have the global cacheline contention.

I've applied this patch + another change you sent on top of it.

Everything looks good to me except the __this_cpu_inc() in __percpu_down_read(),

kernel/locking/percpu-rwsem.c line 87

Quoted 3 lines

kernel/locking/percpu-rwsem.c +3 -0

+	__down_read(&sem->rw_sem);
+	__this_cpu_inc(*sem->read_count);
+	__up_read(&sem->rw_sem);

Preemption is already enabled, don't we need this_cpu_inc() ?

kernel/locking/percpu-rwsem.c line 203

Quoted 2 lines

kernel/locking/percpu-rwsem.c +1 -1

-EXPORT_SYMBOL_GPL(percpu_up_write);
+EXPORT_SYMBOL(percpu_up_write);

and this one ;) I do not really care, but it seems you did this change by accident.

Actually, I _think_ we can do some cleanups/improvements on top of this change, but we can do this later. In particular, _perhaps_ we can avoid the unconditional wakeup in __percpu_up_read(), but I am not sure and in any case this needs another change.

Reviewed-by Oleg Nesterov <[email protected]>

kernel/locking/percpu-rwsem.c +0 -0

@@ -19,6 +20,13 @@ int __percpu_init_rwsem(struct percpu_rw
 
 	/* ->rw_sem represents the whole percpu_rw_semaphore for lockdep */
 	rcu_sync_init(&sem->rss, RCU_SCHED_SYNC);

The current percpu-rwsem read side is entirely free of serializing instructions at the cost of having a synchronize_sched() in the write path.

The latency of the synchronize_sched() is too high for some users (cgroups), so provide a __percpu_init_rwsem(.bias) argument to forgot this synchronize_sched() at the cost of forcing all readers into the slow path, which has serializing instructions.

Quoted 9 lines · Peter Zijlstra

The current percpu-rwsem read side is entirely free of serializing instructions at the cost of having a synchronize_sched() in the write path.

The latency of the synchronize_sched() is too high for some users (cgroups), so provide a __percpu_init_rwsem(.bias) argument to forgot this synchronize_sched() at the cost of forcing all readers into the slow path, which has serializing instructions.

Oh well... I personally do not think this is what we want... Can't we just add the stupid rcu_sync_enter() into cgroup_init() at least for now?

Yes, this means the unnecessary .sync() at boot time, but it will go away after cleanups I am going to send.

Because, again, we will probably want to change this bias dynamically.

Oh well... I personally do not think this is what we want... Can't we just add the stupid rcu_sync_enter() into cgroup_init() at least for now? Yes, this means the unnecessary .sync() at boot time, but it will go away after cleanups I am going to send.

Quoted 5 lines · Oleg Nesterov

Oh well... I personally do not think this is what we want... Can't we just add the stupid rcu_sync_enter() into cgroup_init() at least for now? Yes, this means the unnecessary .sync() at boot time, but it will go away after cleanups I am going to send.

Something like so then?

---

include/linux/rcu_sync.h +1 -0

@@ -59,6 +59,7 @@ static inline bool rcu_sync_is_idle(stru
 }
 
 extern void rcu_sync_init(struct rcu_sync *, enum rcu_sync_type);
+extern void rcu_sync_sabotage(struct rcu_sync *);
 extern void rcu_sync_enter(struct rcu_sync *);
 extern void rcu_sync_exit(struct rcu_sync *);
 extern void rcu_sync_dtor(struct rcu_sync *);

kernel/cgroup.c +1 -0

+
 	get_user_ns(init_cgroup_ns.user_ns);
 
 	mutex_lock(&cgroup_mutex);

kernel/rcu/sync.c +2 -0

@@ -83,6 +83,21 @@ void rcu_sync_init(struct rcu_sync *rsp,
 }
 
 /**
+ * rcu_sync_sabotage() - Sabotage a fresh rcu_sync instance
+ * @rsp: Pointer to rcu_sync structure to be sabotaged

kernel/rcu/sync.c +3 -0

+}
+
+/**
  * rcu_sync_enter() - Force readers onto slowpath
  * @rsp: Pointer to rcu_sync structure to use for synchronization
  *

kernel/rcu/sync.c void rcu_sync_init(struct rcu_sync *rsp, · line 97

Quoted 13 lines · Peter Zijlstra

/**
+ * rcu_sync_sabotage() - Sabotage a fresh rcu_sync instance
+ * @rsp: Pointer to rcu_sync structure to be sabotaged
+ *
+ * Must be called after rcu_sync_init() and before first use.
+ *
+ * Ensures rcu_sync_is_idle() returns false and rcu_sync_{enter,exit}() pairs
+ * turn into NO-OPs.
+ */

kernel/rcu/sync.c +4 -0

+void rcu_sync_sabotage(struct rcu_sync *rsp)
+{
+	rsp->gp_count++;
+	rsp->gp_state = !GP_IDLE;

??? A very strange way to say GP_PENDING. A new GP_DISABLED, perhaps?

Quoted 16 lines · Paul E. McKenney

kernel/rcu/sync.c void rcu_sync_init(struct rcu_sync *rsp, · line 97

Quoted 13 lines · Peter Zijlstra

/**
+ * rcu_sync_sabotage() - Sabotage a fresh rcu_sync instance
+ * @rsp: Pointer to rcu_sync structure to be sabotaged
+ *
+ * Must be called after rcu_sync_init() and before first use.
+ *
+ * Ensures rcu_sync_is_idle() returns false and rcu_sync_{enter,exit}() pairs
+ * turn into NO-OPs.
+ */

kernel/rcu/sync.c +4 -0

+void rcu_sync_sabotage(struct rcu_sync *rsp)
+{
+	rsp->gp_count++;
+	rsp->gp_state = !GP_IDLE;

??? A very strange way to say GP_PENDING. A new GP_DISABLED, perhaps?

Right, so the important thing is that its not GP_IDLE, the rest doesn't really matter.

This forces rcu_sync_is_idle() to return false. The skewed gp_count ensures rcu_sync_{enter,exit}() pairs no-op.

Quoted 5 lines · Oleg Nesterov

Oh well... I personally do not think this is what we want... Can't we just add the stupid rcu_sync_enter() into cgroup_init() at least for now? Yes, this means the unnecessary .sync() at boot time, but it will go away after cleanups I am going to send.

Something like so then?

---

include/linux/rcu_sync.h +1 -0

@@ -59,6 +59,7 @@ static inline bool rcu_sync_is_idle(stru
 }
 
 extern void rcu_sync_init(struct rcu_sync *, enum rcu_sync_type);
+extern void rcu_sync_sabotage(struct rcu_sync *);
 extern void rcu_sync_enter(struct rcu_sync *);
 extern void rcu_sync_exit(struct rcu_sync *);
 extern void rcu_sync_dtor(struct rcu_sync *);

Quoted 9 lines · Peter Zijlstra

Quoted 5 lines · Oleg Nesterov

Oh well... I personally do not think this is what we want... Can't we just add the stupid rcu_sync_enter() into cgroup_init() at least for now? Yes, this means the unnecessary .sync() at boot time, but it will go away after cleanups I am going to send.

Something like so then?

OK, agreed,

Quoted 17 lines

---

include/linux/rcu_sync.h +1 -0

@@ -59,6 +59,7 @@ static inline bool rcu_sync_is_idle(stru
 }
 
 extern void rcu_sync_init(struct rcu_sync *, enum rcu_sync_type);
+extern void rcu_sync_sabotage(struct rcu_sync *);
 extern void rcu_sync_enter(struct rcu_sync *);
 extern void rcu_sync_exit(struct rcu_sync *);
 extern void rcu_sync_dtor(struct rcu_sync *);

kernel/cgroup.c +0 -0

@@ -5609,6 +5609,8 @@ int __init cgroup_init(void)
 	BUG_ON(cgroup_init_cftypes(NULL, cgroup_dfl_base_files));
 	BUG_ON(cgroup_init_cftypes(NULL, cgroup_legacy_base_files));
 

kernel/cgroup.c int __init cgroup_init(void) · line 5612

Quoted 1 line

kernel/cgroup.c +1 -0

+	rcu_sync_sabotage(&cgroup_threadgroup_rwsem.rss);

Heh ;) I too think it should be renamed. I'd suggest

	__rcu_sync_enter(rss);

although I do not really mind and agree with any name.

Hopefully I'll send some cleanups soon, rcu_sync_enter() will be re-implemented as

	rcu_sync_enter(rss)
	{
		if (__rcu_sync_enter(rss))
			__rcu_sync_wait(rss);
	}

kernel/cgroup.c int __init cgroup_init(void) · line 5616

Quoted 4 lines

kernel/cgroup.c +4 -0

+ * rcu_sync_sabotage() - Sabotage a fresh rcu_sync instance
+ * @rsp: Pointer to rcu_sync structure to be sabotaged
+ *
+ * Must be called after rcu_sync_init() and before first use.

OK. And after those cleanups __rcu_sync_enter() can be called at any time.

Oleg.

Quoted 6 lines · Oleg Nesterov

Heh ;) I too think it should be renamed. I'd suggest

	__rcu_sync_enter(rss);

although I do not really mind and agree with any name.

Hmmm... Why not just move the checks out into the caller? That would make the intent much more clear.

Quoted 5 lines · Paul E. McKenney

Quoted 3 lines · Oleg Nesterov

Heh ;) I too think it should be renamed. I'd suggest

Quoted 6 lines

Quoted 3 lines

	__rcu_sync_enter(rss);

although I do not really mind and agree with any name.

Hmmm... Why not just move the checks out into the caller? That would make the intent much more clear.

Hmm. which caller?

Quoted 8 lines · Oleg Nesterov

Quoted 7 lines · Paul E. McKenney

Quoted 5 lines · Oleg Nesterov

Heh ;) I too think it should be renamed. I'd suggest

	__rcu_sync_enter(rss);

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

although I do not really mind and agree with any name.

Hmmm... Why not just move the checks out into the caller? That would make the intent much more clear.

Hmm. which caller?

The ones associated with a percpu_rwsem_bias of PERCPU_RWSEM_READER.

Quoted 14 lines · Paul E. McKenney

Quoted 13 lines · Oleg Nesterov

Quoted 11 lines · Paul E. McKenney

Quoted 6 lines · Oleg Nesterov

Heh ;) I too think it should be renamed. I'd suggest

	__rcu_sync_enter(rss);

although I do not really mind and agree with any name.

Hmmm... Why not just move the checks out into the caller? That would make the intent much more clear.

Quoted 3 lines

Quoted 1 line

Hmm. which caller?

The ones associated with a percpu_rwsem_bias of PERCPU_RWSEM_READER.

Ah. But if we add __rcu_sync_enter() instead of bias/PERCPU_RWSEM_READER.

IOW, please ignore 2/2 which adds PERCPU_RWSEM_READER, the new version just adds rcu_sync_sabotage() which should be renamed (and use GP_PASSED).

Quoted 3 lines · Oleg Nesterov

IOW, please ignore 2/2 which adds PERCPU_RWSEM_READER, the new version just adds rcu_sync_sabotage() which should be renamed (and use GP_PASSED).

OK, then move the checks out into the callers that would have used __rcu_sync_enter(). ;-)

And, there is another possible transition, GP_ENTER -> GP_IDLE, because not it is possible to call __rcu_sync_enter() and rcu_sync_exit() in any

Peter, Paul, could you review? Do you see any hole?

Why. Firstly, note that the state machine was greatly simplified, and rsp->cb_state has gone, we have a single "state" variable, gp_state. Note also the ->sync() op has gone (and actually ->wait() too, see the "TODO" comment).

GP_IDLE - owned by __rcu_sync_enter() which can only move this state 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 - owned by rcu-callback which moves it back to GP_IDLE.

Yes, this is a bit simplified, we also have GP_REPLAY, but hopefully clear.

And, there is another possible transition, GP_ENTER -> GP_IDLE, because not it is possible to call __rcu_sync_enter() and rcu_sync_exit() in any state (except obviously they should be balanced), and they do not block.

The only blocking call is __rcu_sync_wait() which actually waits for GP. Obviously should only be called if gp_count != 0, iow after __rcu_sync_enter.

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

static void rcu_sync_func(struct rcu_head *rcu);

static void rcu_sync_call(struct rcu_sync *rsp)

{
	// TODO: THIS IS SUBOPTIMAL. We want to call it directly
	// if rcu_blocking_is_gp() == T, but it has might_sleep().

	gp_ops[rsp->gp_type].call(&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;

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

	spin_lock_irqsave(&rsp->rss_lock, flags);
	if (rsp->gp_count) {
		/*
		 * We're at least a GP after the first __rcu_sync_enter().
		 */
		rsp->gp_state = GP_PASSED;

	} 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!.

		 *
		 * OR. ->gp_state can be still GP_ENTER if __rcu_sync_wait()
		 * wasn't called after __rcu_sync_enter(), abort.
		 */
		rsp->gp_state = GP_IDLE;
	}
	spin_unlock_irqrestore(&rsp->rss_lock, flags);
}

I think I ended up with this:

.----> GP_IDLE <--------------.

 |	  |                    |
 |	  | __rcu_sync_enter() | <GP> / rcu_sync_exit()
 |	  v                    |
 |	GP_ENTER --------------'
 |	  |
 |  <GP>  |
 |        v
 |	GP_PASSED <---------.
 |	  |                 |
 |	  | rcu_sync_exit() | <GP> / __rcu_sync_enter()
 |	  v                 |
 `----- GP_EXIT ------------'
	  ^
    <GP>  | __rcu_sync_enter() + rcu_sync_exit()
	  v
	GP_RETRY

Quoted 4 lines

static void rcu_sync_call(struct rcu_sync *rsp)

{
	// TODO: THIS IS SUBOPTIMAL. We want to call it directly
	// if rcu_blocking_is_gp() == T, but it has might_sleep().

Not sure I get that comment..

Quoted 6 lines

	spin_lock_irqsave(&rsp->rss_lock, flags);
	if (rsp->gp_count) {
		/*
		 * We're at least a GP after the first __rcu_sync_enter().
		 */
		rsp->gp_state = GP_PASSED;

So we can end up here in two ways afaict.

The simple way: someone called __rcu_sync_enter(), we go IDLE -> ENTER with a raised count. Once the GP passes, we get here, observe the raised count and advance to PASSED.

The more involved way: we were EXIT and someone calls __rcu_sync_enter() to raise the count again. The callback from rcu_sync_exit() was still pending and once we get here we observe the raised count and voila.

Now, since state != IDLE, I suppose this is valid, but it does hurt my brain.

So I think its solid, but you failed to mention one state transition, which seems ok in any case.

Quoted 3 lines · Paul E. McKenney

Quoted 1 line · Oleg Nesterov

Quoted 5 lines

Quoted 2 lines

IOW, please ignore 2/2 which adds PERCPU_RWSEM_READER, the new version just adds rcu_sync_sabotage() which should be renamed (and use GP_PASSED).

OK, then move the checks out into the callers that would have used __rcu_sync_enter(). ;-)

Cough, now I can't understand which check do you mean ;) OK, let me show the code, then we will hopefully understand each other.

----------------------------------------------------------------------
So, as you can see I have fooled you ;) I'll send the patch on top of Peter's changes, this is the (UNTESTED) code with the patch applied.

Peter, Paul, could you review? Do you see any hole?

Why. Firstly, note that the state machine was greatly simplified, and rsp->cb_state has gone, we have a single "state" variable, gp_state. Note also the ->sync() op has gone (and actually ->wait() too, see the "TODO" comment).

GP_IDLE - owned by __rcu_sync_enter() which can only move this state 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 - owned by rcu-callback which moves it back to GP_IDLE.

Yes, this is a bit simplified, we also have GP_REPLAY, but hopefully clear.

And, there is another possible transition, GP_ENTER -> GP_IDLE, because not it is possible to call __rcu_sync_enter() and rcu_sync_exit() in any state (except obviously they should be balanced), and they do not block.

The only blocking call is __rcu_sync_wait() which actually waits for GP. Obviously should only be called if gp_count != 0, iow after __rcu_sync_enter.

--------------------------------------------------------------------------
Now, cgroup_init() can simply call __rcu_sync_enter(&cgroup_threadgroup_rwsem) and switch this sem into the slow mode. Compared to "sabotage" from Peter this implies the unnecessary call_rcu_sched(), but I hope we can tolerate this.

And we can even add a runtime knob to switch between "fast" and "slow aka writer-biased" modes for cgroup_threadgroup_rwsem.

--------------------------------------------------------------------------
And I think __rcu_sync_enter() can have more users. Let's look at freeze_super(). It calls percpu_down_write() 3 times, and waits for 3 GP's sequentally.

Now we can add 3 __rcu_sync_enter's at the start and 3 rcu_sync_exit's at the end (actually we can do better, just to simplify). And again, note that rcu_sync_exit() will work correctly even if we (say) return -EBUSY, so rcu_sync_wait and/or percpu_down_write() was not called in between, and in this case we won't block waiting for GP.

What do you think?

Oleg.
---

// rcu_sync.h: ----------------------------------------------------------------

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

	struct rcu_head		cb_head;
	enum rcu_sync_type	gp_type;
};

// sync.c --------------------------------------------------------------------- #include <linux/rcu_sync.h>
#include <linux/sched.h>

#ifdef CONFIG_PROVE_RCU
#define __INIT_HELD(func) .held = func,
#else
#define __INIT_HELD(func)
#endif

static const struct {
	void (*call)(struct rcu_head *, void (*)(struct rcu_head *));
	void (*wait)(void);	// TODO: remove this, see the comment in dtor
#ifdef CONFIG_PROVE_RCU
	int  (*held)(void);
#endif
} gp_ops[] = {
	[RCU_SYNC] = {
		.call = call_rcu,
		.wait = rcu_barrier,
		__INIT_HELD(rcu_read_lock_held)
	},
	[RCU_SCHED_SYNC] = {
		.call = call_rcu_sched,
		.wait = rcu_barrier_sched,
		__INIT_HELD(rcu_read_lock_sched_held)
	},
	[RCU_BH_SYNC] = {
		.call = call_rcu_bh,
		.wait = rcu_barrier_bh,
		__INIT_HELD(rcu_read_lock_bh_held)
	},
};

#define rss_lock gp_wait.lock

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

static void rcu_sync_func(struct rcu_head *rcu);

static void rcu_sync_call(struct rcu_sync *rsp)

{
	// TODO: THIS IS SUBOPTIMAL. We want to call it directly
	// if rcu_blocking_is_gp() == T, but it has might_sleep().

	gp_ops[rsp->gp_type].call(&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;

	BUG_ON(rsp->gp_state == GP_IDLE);

	BUG_ON(rsp->gp_state == GP_PASSED);

	spin_lock_irqsave(&rsp->rss_lock, flags);
	if (rsp->gp_count) {
		/*
		 * We're at least a GP after the first __rcu_sync_enter().
		 */

		rsp->gp_state = GP_PASSED;
	} 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!.
		 *
		 * OR. ->gp_state can be still GP_ENTER if __rcu_sync_wait()
		 * wasn't called after __rcu_sync_enter(), abort.
		 */
		rsp->gp_state = GP_IDLE;
	}
	spin_unlock_irqrestore(&rsp->rss_lock, flags);
}

bool __rcu_sync_enter(struct rcu_sync *rsp)

{
	int gp_count, gp_state;

	spin_lock_irq(&rsp->rss_lock);
	gp_count = rsp->gp_count++;
	gp_state = rsp->gp_state;
	if (gp_state == GP_IDLE) {
		rsp->gp_state = GP_ENTER;
		rcu_sync_call(rsp);
	}
	spin_unlock_irq(&rsp->rss_lock);

	BUG_ON(gp_count != 0 && gp_state == GP_IDLE);
	BUG_ON(gp_count == 0 && gp_state == GP_PASSED);

	return gp_state < GP_PASSED;

}

void __rcu_sync_wait(struct rcu_sync *rsp)

{
	BUG_ON(rsp->gp_state == GP_IDLE);
	BUG_ON(rsp->gp_count == 0);

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

}

void rcu_sync_enter(struct rcu_sync *rsp)

{
	if (__rcu_sync_enter(rsp))
		__rcu_sync_wait(rsp);
}

void rcu_sync_exit(struct rcu_sync *rsp)

{
	BUG_ON(rsp->gp_state == GP_IDLE);

	BUG_ON(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);
}

void rcu_sync_dtor(struct rcu_sync *rsp)

{
	int gp_state;

	BUG_ON(rsp->gp_count);
	BUG_ON(rsp->gp_state == GP_PASSED);

	spin_lock_irq(&rsp->rss_lock);
	if (rsp->gp_state == GP_REPLAY)
		rsp->gp_state = GP_EXIT;

	gp_state = rsp->gp_state;
	spin_unlock_irq(&rsp->rss_lock);

	// TODO: add another wake_up_locked() into rcu_sync_func(),

	// use wait_event + spin_lock_wait, remove gp_ops->wait().
	if (gp_state != GP_IDLE) {
		gp_ops[rsp->gp_type].wait();
		BUG_ON(rsp->gp_state != GP_IDLE);
	}
}

Quoted 3 lines · Oleg Nesterov

And, there is another possible transition, GP_ENTER -> GP_IDLE, because not it is possible to call __rcu_sync_enter() and rcu_sync_exit() in any state (except obviously they should be balanced), and they do not block.

Hmmm... I am assuming that the only threads allowed to invoke rcu_sync_exit() are those that have executed an unpaired rcu_sync_enter(). With that assumption, rcu_sync_enter() is illegal if GP_IDLE or GP_ENTER, because the rcu_sync_enter() calls won't return until the state reaches GP_PASSED.

If you feel strongly about allowing rcu_sync_exit() in GP_ENTER state, could you please tell me your use case? Or am I confused?

Quoted 2 lines

The only blocking call is __rcu_sync_wait() which actually waits for GP. Obviously should only be called if gp_count != 0, iow after __rcu_sync_enter.

Well, rcu_sync_dtor() as well. But to your point, it is blocking in rcu_barrier(), not in wait_event().

Quoted 1 line

#define rss_lock gp_wait.lock

Interesting... Should there instead be a wait_queue_lock() and wait_queue_unlock()? If someone changes how wait-queue locking work, this one might be a bit tricky.

Quoted 4 lines

static void rcu_sync_call(struct rcu_sync *rsp)

{
	// TODO: THIS IS SUBOPTIMAL. We want to call it directly
	// if rcu_blocking_is_gp() == T, but it has might_sleep().

Careful! This optimization works only for RCU-sched and RCU-bh. With normal RCU, you can be tripped up by tasks preempted within RCU read-side critical sections should CONFIG_PREEMPT=y.

Quoted 5 lines

static void rcu_sync_func(struct rcu_head *rcu)

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

Quoted 1 line

	BUG_ON(rsp->gp_state == GP_IDLE);

If you really want rcu_sync_exit() to be invoked before the corresponding rcu_sync_enter() returns, and if you want that to set the state to GP_IDLE, then the above BUG_ON() can fire. This is why I was assuming that rcu_sync_enter() must return before the corresponding rcu_sync_exit() can start.

Quoted 1 line

	BUG_ON(rsp->gp_state == GP_PASSED);

READ_ONCE() would feel better in both of the above BUG_ON()s, given that we are not yet holding the lock.

Quoted 5 lines

	spin_lock_irqsave(&rsp->rss_lock, flags);
	if (rsp->gp_count) {
		/*
		 * We're at least a GP after the first __rcu_sync_enter().
		 */

Don't we need a wakeup here?

Quoted 13 lines

bool __rcu_sync_enter(struct rcu_sync *rsp)

{
	int gp_count, gp_state;

	spin_lock_irq(&rsp->rss_lock);
	gp_count = rsp->gp_count++;
	gp_state = rsp->gp_state;
	if (gp_state == GP_IDLE) {
		rsp->gp_state = GP_ENTER;
		rcu_sync_call(rsp);
	}
	spin_unlock_irq(&rsp->rss_lock);

Quoted 2 lines

	BUG_ON(gp_count != 0 && gp_state == GP_IDLE);
	BUG_ON(gp_count == 0 && gp_state == GP_PASSED);

Isn't gp_count==0 illegal here regardless of gp_state value?

Quoted 1 line

	return gp_state < GP_PASSED;

And the above statement is another reason why I believe that it should be illegal to invoke rcu_sync_exit() until after the matching rcu_sync_enter() has returned. If we were preempted just before the above "return" statement, we might be in GP_IDLE state upon return,

Quoted 6 lines

void __rcu_sync_wait(struct rcu_sync *rsp)

{
	BUG_ON(rsp->gp_state == GP_IDLE);
	BUG_ON(rsp->gp_count == 0);

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

I would feel better if all the references to ->gp_state and ->gp_count outside of the lock be READ_ONCE(). Compilers can be tricky beasts.

rcu_sync_enter() checks to see if the rcu_sync structure is in writer mode, and, if not, initiates a transition to writer mode and waits for that transition to complete. Either way, a writer reference is acquired.

Quoted 5 lines

void rcu_sync_enter(struct rcu_sync *rsp)

{
	if (__rcu_sync_enter(rsp))
		__rcu_sync_wait(rsp);
}

Here is what I believe rcu_sync_exit() does:

rcu_sync_exit() removes an updater reference, and if there are no more, starts the transition to reader mode. Note that this must handle transitions back to writer mode that occur before the transition to reader mode has fully completed.

Quoted 3 lines

void rcu_sync_exit(struct rcu_sync *rsp)

{
	BUG_ON(rsp->gp_state == GP_IDLE);

	BUG_ON(READ_ONCE(rsp->gp_state) == GP_ENTER);

If you do allow rcu_sync_exit() while GP_ENTER, then all the readers can do rcu_sync_exit() in that state, leaving the state at GP_PASSED instead of the desired GP_IDLE.

And here is what I believe rcu_sync_dtor() does:

rcu_sync_dtor() cleans up at end, waiting for a full transition back into reader mode. Anything invoking rcu_sync_dtor() must have ensured that there will be no more calls to rcu_sync_enter() and rcu_sync_exit().

Quoted 7 lines

void rcu_sync_dtor(struct rcu_sync *rsp)

{
	int gp_state;

	BUG_ON(rsp->gp_count);
	BUG_ON(rsp->gp_state == GP_PASSED);

Quoted 3 lines

	spin_lock_irq(&rsp->rss_lock);
	if (rsp->gp_state == GP_REPLAY)
		rsp->gp_state = GP_EXIT;

OK, this ensures that the .wait() below will wait for the callback, but it might result in some RCU read-side critical sections still being in flight after rcu_sync_dtor() completes.

In other words, any transition
started by an rcu_sync_exit() will normally wait until all pre-existing RCU readers complete -- unless that rcu_sync_exit() set the state to GP_REPLAY and was quickly followed by rcu_sync_dtor().

We need to add the first wakeup before we can add another one. ;-)

But I don't see why we need a second wakeup, given that rcu_barrier() wakes itself up.

Quoted 6 lines

	// use wait_event + spin_lock_wait, remove gp_ops->wait().
	if (gp_state != GP_IDLE) {
		gp_ops[rsp->gp_type].wait();
		BUG_ON(rsp->gp_state != GP_IDLE);
	}
}

And please see below for the state table and assumptions.

Quoted 7 lines · Paul E. McKenney

Quoted 3 lines · Oleg Nesterov

And, there is another possible transition, GP_ENTER -> GP_IDLE, because not it is possible to call __rcu_sync_enter() and rcu_sync_exit() in any state (except obviously they should be balanced), and they do not block.

Quoted 5 lines

Hmmm... I am assuming that the only threads allowed to invoke rcu_sync_exit() are those that have executed an unpaired rcu_sync_enter(). With that assumption, rcu_sync_enter() is illegal if GP_IDLE or GP_ENTER, because the rcu_sync_enter() calls won't return until the state reaches GP_PASSED.

Not sure I understand... Obviously rcu_sync_exit() can only be used if it pairs with the preceeding __rcu_sync_enter() or rcu_sync_enter(), if nothing else rsp->gp_count-- must not underflow.

rcu_sync_enter() or __rcu_sync_enter() is legal in any state, the latter won't block.

Quoted 2 lines

If you feel strongly about allowing rcu_sync_exit() in GP_ENTER state, could you please tell me your use case? Or am I confused?

I'll write another email, let me reply to the code review first. And thanks for your review!

Quoted 5 lines

Quoted 2 lines

The only blocking call is __rcu_sync_wait() which actually waits for GP. Obviously should only be called if gp_count != 0, iow after __rcu_sync_enter.

Well, rcu_sync_dtor() as well. But to your point, it is blocking in rcu_barrier(), not in wait_event().

Yes, yes, sure. I mean't the only blocking call in enter/exit paths.

Quoted 5 lines

Quoted 1 line

#define rss_lock gp_wait.lock

Interesting... Should there instead be a wait_queue_lock() and wait_queue_unlock()? If someone changes how wait-queue locking work, this one might be a bit tricky.

Well, the current version does the same... I do not mind to add wait_queue_lock(), but personally I do not think this would be better. Either way the code should not use gp_wait.lock directly, to make it clear that we abuse the spinlock we already have in rcu_sync->gp_wait.

To me

	spin_lock_irq(&rsp->rss_lock);
	spin_unlock_irq(&rsp->rss_lock);

looks a bit better than

	wait_queue_lock();
	wait_queue_unlock();

because we need to protect the state, not the wait-queue. And if someone changes how wait-queue locking work, we can just actually add "spinlock_t rss_lock" into rcu_sync and remove that "#define".

Quoted 8 lines

Quoted 4 lines

static void rcu_sync_call(struct rcu_sync *rsp)

{
	// TODO: THIS IS SUBOPTIMAL. We want to call it directly
	// if rcu_blocking_is_gp() == T, but it has might_sleep().

Careful! This optimization works only for RCU-sched and RCU-bh. With normal RCU, you can be tripped up by tasks preempted within RCU read-side critical sections should CONFIG_PREEMPT=y.

Yes, thanks, I understand ;) another reason why I do not want to add this optimization into the initial version.

Quoted 5 lines

Quoted 5 lines

static void rcu_sync_func(struct rcu_head *rcu)

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

Quoted 7 lines

Quoted 1 line

	BUG_ON(rsp->gp_state == GP_IDLE);

If you really want rcu_sync_exit() to be invoked before the corresponding rcu_sync_enter() returns, and if you want that to set the state to GP_IDLE, then the above BUG_ON() can fire. This is why I was assuming that rcu_sync_enter() must return before the corresponding rcu_sync_exit() can start.

Again, can't understand. Let me repeat, of course rcu_sync_exit() can only be called after __rcu_sync_enter() (or rcu_sync_enter() which does __rcu_sync_enter + wait), they obviously should be balanced.

But, it should be fine to call rcu_sync_exit() right after __rcu_sync_enter() returns, without waiting for GP_PASSED, iow without __rcu_sync_wait().

But probably I misunderstood you... Either way, rcu_sync_func() can never hit gp_state == GP_IDLE. Because this state can only be set by rcu_sync_func(), nobody else can set this state. Ignoring the initial state of course.

And every caller of rcu_sync_call() (which does call_rcu(rcu_sync_func)) updates the state first:

	rsp->gp_state = GP_ENTER;	// __rcu_sync_enter()
	rcu_sync_call(rsp);

	rsp->gp_state = GP_EXIT;	// rcu_sync_exit() or REPLAY
	rcu_sync_call(rsp);

so I simply can't understand your concern.

Quoted 4 lines

Quoted 1 line

	BUG_ON(rsp->gp_state == GP_PASSED);

READ_ONCE() would feel better in both of the above BUG_ON()s, given that we are not yet holding the lock.

The lockless checks are fine, both states can only be set by the callback. As for READ_ONCE(), please see below.

Quoted 7 lines

Quoted 5 lines

	spin_lock_irqsave(&rsp->rss_lock, flags);
	if (rsp->gp_count) {
		/*
		 * We're at least a GP after the first __rcu_sync_enter().
		 */

Don't we need a wakeup here?

Heh ;) let me show you what I have in kernel/rcu/sync.c~

	if (rsp->gp_count) {
		/*
		 * COMMENT.
		 */
		rsp->gp_state = GP_PASSED;
		wake_up_locked(&rsp->gp_wait);
	} else if (rsp->gp_state == GP_REPLAY) {

apparently I removed this wake_up_locked() by accident when I edited the "COMMENT" placeholder ;)

Thanks!

Quoted 13 lines

Quoted 13 lines

bool __rcu_sync_enter(struct rcu_sync *rsp)

{
	int gp_count, gp_state;

	spin_lock_irq(&rsp->rss_lock);
	gp_count = rsp->gp_count++;
	gp_state = rsp->gp_state;
	if (gp_state == GP_IDLE) {
		rsp->gp_state = GP_ENTER;
		rcu_sync_call(rsp);
	}
	spin_unlock_irq(&rsp->rss_lock);

Quoted 4 lines

Quoted 2 lines

	BUG_ON(gp_count != 0 && gp_state == GP_IDLE);
	BUG_ON(gp_count == 0 && gp_state == GP_PASSED);

Isn't gp_count==0 illegal here regardless of gp_state value?

No. gp_count is the old value of rsp->gp_count, before it was incremented by us.

Quoted 6 lines

Quoted 1 line

	return gp_state < GP_PASSED;

And the above statement is another reason why I believe that it should be illegal to invoke rcu_sync_exit() until after the matching rcu_sync_enter() has returned. If we were preempted just before the above "return" statement, we might be in GP_IDLE state upon return,

How? Note the GP_IDLE -> GP_ENTER transition above, it can't be GP_IDLE. And since we incremented rsp->gp_count, GP_IDLE is not possibly until rcu_sync_exit() which pairs with this "enter" decrements the counter and fires the RCU callback which will set GP_IDLE after GP.

Quoted 9 lines

Quoted 6 lines

void __rcu_sync_wait(struct rcu_sync *rsp)

{
	BUG_ON(rsp->gp_state == GP_IDLE);
	BUG_ON(rsp->gp_count == 0);

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

I would feel better if all the references to ->gp_state and ->gp_count outside of the lock be READ_ONCE(). Compilers can be tricky beasts.

Oh. I won't argue too much, but I do not agree.

Because I started to hate these _ONCE() helpers a long ago. Why? Because (imo) we have too many XXX_ONCE() added "just in case, because compiler can be buggy".

Now, when I look some particular READ_ONCE() I can almost never understand the reason: do we really need it for correctness? or this is another "it does not hurt" case?

So why do we need READ_ONCE() here? How this wait_event() differs from any other wait_event() which does a plain LOAD? Should we "fix" them all?

Quoted 9 lines

rcu_sync_enter() checks to see if the rcu_sync structure is in writer mode, and, if not, initiates a transition to writer mode and waits for that transition to complete. Either way, a writer reference is acquired.

Quoted 5 lines

void rcu_sync_enter(struct rcu_sync *rsp)

{
	if (__rcu_sync_enter(rsp))
		__rcu_sync_wait(rsp);
}

Yes. And of course it could call __rcu_sync_wait() unconditionally:

	__rcu_sync_enter(rsp);
	__rcu_sync_wait(rsp);

This "if" is more the documentation than optimization.

Quoted 7 lines

Here is what I believe rcu_sync_exit() does:

rcu_sync_exit() removes an updater reference, and if there are no more, starts the transition to reader mode. Note that this must handle transitions back to writer mode that occur before the transition to reader mode has fully completed.

Quoted 7 lines

Quoted 3 lines

void rcu_sync_exit(struct rcu_sync *rsp)

{
	BUG_ON(rsp->gp_state == GP_IDLE);

	BUG_ON(READ_ONCE(rsp->gp_state) == GP_ENTER);

If you do allow rcu_sync_exit() while GP_ENTER, then all the readers

Quoted 2 lines

can do rcu_sync_exit() in that state, leaving the state at GP_PASSED instead of the desired GP_IDLE.

Can't understand this too...

Firstly, readers do not do rcu_sync_enter/exit. They only use rcu_sync_is_idle() which checks gp_state == GP_IDLE.

As for enter() path, we only need to ensure that after __rcu_sync_wait() returns, all CPU's must see gp_state != GP_IDLE. Yes, yes, this is not really true, we also need the memory barriers implied by RCU, lets ignore this to simplify the discussion.

Now lets suppose that rcu_sync_exit() is called while GP_ENTER. This is only possible if __rcu_sync_wait() was not called by us. IOW, this thread does

	__rcu_sync_enter();
	rcu_sync_exit();

by any reason.

Now. If gp_count is not zero after we decrement it, rcu_sync_exit() does nothing.

If it is zero, we could set GP_IDLE right now. We do not care about the readers in this case, exactly because __rcu_sync_wait() was not called and thus we can't even know if other CPU's had any chance to observe the gp_state != GP_IDLE state set by __rcu_sync_enter().

But. We can't actually set GP_IDLE, because GP_ENTER means that the RCU callback is already pending and this can break the next enter(). IOW, this would break the "GP_IDLE is owned by __rcu_sync_enter" contract.

And we do not need to do this! We rely on the already pending rcu_sync_func() which will set GP_IDLE _if_ the counter is still zero.

Now suppose that another writer (__rcu_sync_enter) comes and increments gp_count right before rcu_sync_func() is called by RCU. In this case rcu_sync_func() will notice rsp->gp_count != 0 and correctly set GP_PASSED.

Note that in this case that new writer won't wait for a full GP, quite possibly __rcu_sync_wait() won't even block. And this is correct, we know that we have at least one full GP after the previous GP_IDLE -> GP_ENTER transition.

Quoted 5 lines

And here is what I believe rcu_sync_dtor() does:

rcu_sync_dtor() cleans up at end, waiting for a full transition back into reader mode. Anything invoking rcu_sync_dtor() must have ensured that there will be no more calls to rcu_sync_enter() and rcu_sync_exit().

Sure, we are going to destroy/free this object.

Quoted 11 lines

Quoted 10 lines

void rcu_sync_dtor(struct rcu_sync *rsp)

{
	int gp_state;

	BUG_ON(rsp->gp_count);
	BUG_ON(rsp->gp_state == GP_PASSED);

	spin_lock_irq(&rsp->rss_lock);
	if (rsp->gp_state == GP_REPLAY)
		rsp->gp_state = GP_EXIT;

Quoted 3 lines

OK, this ensures that the .wait() below will wait for the callback, but it might result in some RCU read-side critical sections still being in flight after rcu_sync_dtor() completes.

Hmm. Obviously, the caller should prevent this somehow or it is simply buggy. Or I misunderstood.

Quoted 4 lines

In other words, any transition
started by an rcu_sync_exit() will normally wait until all pre-existing RCU readers complete -- unless that rcu_sync_exit() set the state to GP_REPLAY and was quickly followed by rcu_sync_dtor().

Again, we are going to (say) free this memory. The caller must ensure that rcu_sync_is_idle(rsp) or anything else which can touch this object is not possible.

The only reason for rcu_sync_dtor() is that we can't free/destroy this memory until we ensure that the pending rcu_sync_func() finishes. And of course we need to ensure it won't re-arm itself, that is why we do GP_REPLAY -> GP_EXIT transition.

And in any case, this patch doesn't change rcu_sync_dtor() logic, it is the same even if ->cb_state goes away.

Quoted 1 line

We need to add the first wakeup before we can add another one. ;-)

Yes ;) see above.

Quoted 9 lines

But I don't see why we need a second wakeup, given that rcu_barrier() wakes itself up.

Quoted 6 lines

	// use wait_event + spin_lock_wait, remove gp_ops->wait().
	if (gp_state != GP_IDLE) {
		gp_ops[rsp->gp_type].wait();
		BUG_ON(rsp->gp_state != GP_IDLE);
	}
}

Because we can remove .wait() op and simplify the code a bit more. This also can be more efficient, altough this doesn't really matter. The main reason is that this would be more clear: we only need to sync with one particular callback, we do not need to wait for all outstanding RCU callbacks to complete. Anyway, this is TODO and needs another patch, we will discuss this later.

Quoted 1 line

And please see below for the state table and assumptions.

I'll try to read it carefully later.

Thanks Paul!

Oleg.

Quoted 2 lines · Oleg Nesterov

rcu_sync_enter() or __rcu_sync_enter() is legal in any state, the latter won't block.

Actually, I had no idea that __rcu_sync_enter() was intended for anything other than internal use.

Other than that, agreed, with the exception that it is illegal after rcu_sync_dtor() has been called.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

static void rcu_sync_call(struct rcu_sync *rsp)

{
	// TODO: THIS IS SUBOPTIMAL. We want to call it directly
	// if rcu_blocking_is_gp() == T, but it has might_sleep().

Quoted 6 lines

Quoted 3 lines

Careful! This optimization works only for RCU-sched and RCU-bh. With normal RCU, you can be tripped up by tasks preempted within RCU read-side critical sections should CONFIG_PREEMPT=y.

Yes, thanks, I understand ;) another reason why I do not want to add this optimization into the initial version.

So I should take this as a request to export rcu_blocking_is_gp()?

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

void rcu_sync_dtor(struct rcu_sync *rsp)

{
	int gp_state;

	BUG_ON(rsp->gp_count);
	BUG_ON(rsp->gp_state == GP_PASSED);

	spin_lock_irq(&rsp->rss_lock);
	if (rsp->gp_state == GP_REPLAY)
		rsp->gp_state = GP_EXIT;

Quoted 6 lines

Quoted 3 lines

OK, this ensures that the .wait() below will wait for the callback, but it might result in some RCU read-side critical sections still being in flight after rcu_sync_dtor() completes.

Hmm. Obviously, the caller should prevent this somehow or it is simply buggy. Or I misunderstood.

Hard to say without knowing what the permitted use cases are...

Me, I would make rcu_sync_dtor() wait the extra grace period in this case. It should be a low-probability race, and it reduces the _dtor-time state space.

What it looks like you are saying is that the caller must not only ensure that there will never again be a __rcu_sync_enter(), rcu_sync_enter(), or rcu_sync_exit() (or, I suppose, rcu_sync_dtor()) for this rcu_sync structure,

but must also ensure that any relevant RCU read-side critical sections have completed.

Quoted 3 lines · Oleg Nesterov

And, there is another possible transition, GP_ENTER -> GP_IDLE, because not it is possible to call __rcu_sync_enter() and rcu_sync_exit() in any state (except obviously they should be balanced), and they do not block.

If you feel strongly about allowing rcu_sync_exit() in GP_ENTER state, could you please tell me your use case? Or am I confused?

And I think __rcu_sync_enter() can have more users. Let's look at freeze_super(). It calls percpu_down_write() 3 times, and waits for 3 GP's sequentally.

Quoted 6 lines

Now we can add 3 __rcu_sync_enter's at the start and 3 rcu_sync_exit's at the end (actually we can do better, just to simplify). And again, note that rcu_sync_exit() will work correctly even if we (say) return -EBUSY, so rcu_sync_wait and/or percpu_down_write() was not called in between, and in this case we won't block waiting for GP.

I am not going to claim to understand freeze_super(), but it does seem to have a fair amount of waiting.

But yes, you could put rcu_sync_enter()

and rcu_sync_exit() before and
after a series of write-side enter/exit pairs in order to force things to stay in writer mode, if that is what you are suggesting.

Paul, I had to switch to internal bugzillas after the first email, and now I feel I can't read... I'll try to answer one question right now, tomorrow I'll reread your email, probably I need to answer something else...

Quoted 6 lines · Oleg Nesterov

Quoted 4 lines

And, there is another possible transition, GP_ENTER -> GP_IDLE, because not it is possible to call __rcu_sync_enter() and rcu_sync_exit() in any state (except obviously they should be balanced), and they do not block.

...

Quoted 2 lines

If you feel strongly about allowing rcu_sync_exit() in GP_ENTER state, could you please tell me your use case? Or am I confused?

See below,

Quoted 11 lines

And I think __rcu_sync_enter() can have more users. Let's look at freeze_super(). It calls percpu_down_write() 3 times, and waits for 3 GP's sequentally.

Quoted 6 lines

Now we can add 3 __rcu_sync_enter's at the start and 3 rcu_sync_exit's at the end (actually we can do better, just to simplify). And again, note that rcu_sync_exit() will work correctly even if we (say) return -EBUSY, so rcu_sync_wait and/or percpu_down_write() was not called in between, and in this case we won't block waiting for GP.

Quoted 4 lines

I am not going to claim to understand freeze_super(), but it does seem to have a fair amount of waiting.

But yes, you could put rcu_sync_enter()

                         ^^^^^^^^^^^^^^

__rcu_sync_enter, see below,

Quoted 3 lines

and rcu_sync_exit() before and
after a series of write-side enter/exit pairs in order to force things to stay in writer mode, if that is what you are suggesting.

No, no, this is not what I am trying to suggest.

The problem is that freeze_super() takes 3 semaphores for writing in row, this means that it needs to wait for 3 GP's sequentally, and it does this with sb->s_umount held. This is just ugly.

OK, lets suppose it simply does

	freeze_super(sb)
	{
		down_write(&sb->s_umount);
		if (NEED_TO_FREEZE) {
			percpu_down_write(SEM1);
			percpu_down_write(SEM2);
			percpu_down_write(SEM3);
		}
		up_write(&sb->s_umount);
	}

and every percpu_down_write() waits for GP.

Now, suppose we add the additional enter/exit's:

	freeze_super(sb)
	{
		// this doesn't block
		__rcu_sync_enter(SEM3);
		__rcu_sync_enter(SEM2);
		__rcu_sync_enter(SEM1);

		down_write(&sb->s_umount);
		if (NEED_TO_FREEZE) {
			percpu_down_write(SEM1);

			percpu_down_write(SEM2);
			percpu_down_write(SEM3);
		}
		up_write(&sb->s_umount);

		rcu_sync_exit(SEM1);

		rcu_sync_exit(SEM2);
		rcu_sync_exit(SEM3);

		
	}

Again, actually we can do better, just to simplify.

Now. the fisrt percpu_down_write(SEM1) can block waiting for GP or not, this depends on how many time it spends in down_write().

But the 2nd and the 3rd percpu_down_write() most likely won't block, so in the likely case freeze_super() will need a single GP pass.

And note that NEED_TO_FREEZE can be false, in this case rcu_sync_exit() will be called in GP_ENTER state.

To some degree, this is like get_state_synchronize_rcu/cond_synchronize_rcu. But obviously percpu_down_write() can not use these helpers, and in this particular case __rcu_sync_enter() is better because it forces the start of GP pass.

-----------------------------------------------------------------------
As for cgroups, we want to switch cgroup_threadgroup_rwsem into the slow mode, at least for now.

We could add the additional hooks/hacks into rcu/sync.c but why? We can do this without any changes outside of cgroup.c right now, just add rcu_sync_enter() into cgroup_init().

But we do not want to add a pointless synchronize_sched() at boot time, __rcu_sync_enter() looks much better.

------------------------------------------------------------------------
And even __cgroup_procs_write() could use __rcu_sync_enter(). But lets ignore this for now.

Quoted 9 lines · Oleg Nesterov

Now, suppose we add the additional enter/exit's:

	freeze_super(sb)
	{
		// this doesn't block
		__rcu_sync_enter(SEM3);
		__rcu_sync_enter(SEM2);
		__rcu_sync_enter(SEM1);

Quoted 3 lines

		down_write(&sb->s_umount);
		if (NEED_TO_FREEZE) {
			percpu_down_write(SEM1);

The above waits for the grace period initiated by __rcu_sync_enter(), correct? Presumably "yes", because it will invoke rcu_sync_enter(), which will see the state as GP_ENTER, and will thus wait.

But your point is that if !NEED_TO_FREEZE, we will get here without waiting for a grace period.

But why aren't the __rcu_sync_enter() and rcu_sync_exit() calls inside the "if" statement?

That aside, would it make sense to name __rcu_sync_enter() something like rcu_sync_begin_to_enter(), rcu_sync_pre_enter() or some such? Something to make it clear that it just starts the job and that something else is needed to finish it.

And here is an updated state table. I do not yet separately call out __rcu_sync_enter(), though without it the rcu_sync_exit() transition out of state B cannot happen.

Quoted 12 lines · Paul E. McKenney

Quoted 9 lines · Oleg Nesterov

Now, suppose we add the additional enter/exit's:

	freeze_super(sb)
	{
		// this doesn't block
		__rcu_sync_enter(SEM3);
		__rcu_sync_enter(SEM2);
		__rcu_sync_enter(SEM1);

Quoted 7 lines

Quoted 3 lines

		down_write(&sb->s_umount);
		if (NEED_TO_FREEZE) {
			percpu_down_write(SEM1);

The above waits for the grace period initiated by __rcu_sync_enter(), correct? Presumably "yes", because it will invoke rcu_sync_enter(), which will see the state as GP_ENTER, and will thus wait.

But if down_write() blocks and/or NEED_TO_FREEZE takes some time it could already see the GP_PASSED state, or at least it can sleep less.

Quoted 3 lines

But your point is that if !NEED_TO_FREEZE, we will get here without waiting for a grace period.

Quoted 2 lines

But why aren't the __rcu_sync_enter() and rcu_sync_exit() calls inside the "if" statement?

Yes, if we do __rcu_sync_enter() inside "if", then rcu_sync_exit() can't hit GP_ENTER.

But why we should disallow this use-case? It does not complicate the code at all.

And see above, we want to initiate the GP "asap", so that we will sleep less later. Although yes, freeze_super() is not the best example. And __cgroup_procs_write() too, but note that cgroup_kn_lock_live() is rather heavy, takes the global locks, and can fail. So (ignoring the fact we are going to switch cgroup_threadgroup_rwsem into the slow mode for now) __rcu_sync_enter() at the start could help to lessen the time percpu_down_write(cgroup_threadgroup_rwsem) sleeps with the cgroup_mutex held.

Quoted 4 lines

That aside, would it make sense to name __rcu_sync_enter() something like rcu_sync_begin_to_enter(), rcu_sync_pre_enter() or some such? Something to make it clear that it just starts the job and that something else is needed to finish it.

Sure. Agreed, will rename.

Quoted 3 lines

And here is an updated state table. I do not yet separately call out __rcu_sync_enter(), though without it the rcu_sync_exit() transition out of state B cannot happen.

Thanks! I'll try to double-check it.

Quoted 33 lines · Oleg Nesterov

Quoted 12 lines · Paul E. McKenney

Quoted 9 lines · Oleg Nesterov

Now, suppose we add the additional enter/exit's:

	freeze_super(sb)
	{
		// this doesn't block
		__rcu_sync_enter(SEM3);
		__rcu_sync_enter(SEM2);
		__rcu_sync_enter(SEM1);

Quoted 7 lines

Quoted 3 lines

		down_write(&sb->s_umount);
		if (NEED_TO_FREEZE) {
			percpu_down_write(SEM1);

The above waits for the grace period initiated by __rcu_sync_enter(), correct? Presumably "yes", because it will invoke rcu_sync_enter(), which will see the state as GP_ENTER, and will thus wait.

But if down_write() blocks and/or NEED_TO_FREEZE takes some time it could already see the GP_PASSED state, or at least it can sleep less.

Quoted 5 lines

But your point is that if !NEED_TO_FREEZE, we will get here without waiting for a grace period.

But why aren't the __rcu_sync_enter() and rcu_sync_exit() calls inside the "if" statement?

Yes, if we do __rcu_sync_enter() inside "if", then rcu_sync_exit() can't hit GP_ENTER.

Quoted 2 lines

But why we should disallow this use-case? It does not complicate the code at all.

I do agree that it doesn't complicate the current implementation. But it relies on a global lock, so I am not at all confident that this implementation is the final word.

And speaking of global locks, failing to discourage the pattern above means that the code is unnecessarily acquiring three global locks, which doesn't seem like a good thing to me.

I agree that there are use cases for beginning-of-time __rcu_sync_enter() or whatever we end up naming it.

Paul, sorry for delay.

Quoted 35 lines · Paul E. McKenney

Quoted 33 lines · Oleg Nesterov

Quoted 12 lines · Paul E. McKenney

Quoted 9 lines · Oleg Nesterov

Now, suppose we add the additional enter/exit's:

        freeze_super(sb)
        {
                // this doesn't block
                __rcu_sync_enter(SEM3);
                __rcu_sync_enter(SEM2);
                __rcu_sync_enter(SEM1);

Quoted 7 lines

Quoted 3 lines

                down_write(&sb->s_umount);
                if (NEED_TO_FREEZE) {
                        percpu_down_write(SEM1);

The above waits for the grace period initiated by __rcu_sync_enter(), correct? Presumably "yes", because it will invoke rcu_sync_enter(), which will see the state as GP_ENTER, and will thus wait.

But if down_write() blocks and/or NEED_TO_FREEZE takes some time it could already see the GP_PASSED state, or at least it can sleep less.

Quoted 5 lines

But your point is that if !NEED_TO_FREEZE, we will get here without waiting for a grace period.

But why aren't the __rcu_sync_enter() and rcu_sync_exit() calls inside the "if" statement?

Yes, if we do __rcu_sync_enter() inside "if", then rcu_sync_exit() can't hit GP_ENTER.

Quoted 6 lines

Quoted 2 lines

But why we should disallow this use-case? It does not complicate the code at all.

I do agree that it doesn't complicate the current implementation. But it relies on a global lock, so I am not at all confident that this implementation is the final word.

Hmm. which global lock? Or did you mean freeze_super(), not rcu_sync?

Quoted 3 lines

And speaking of global locks, failing to discourage the pattern above means that the code is unnecessarily acquiring three global locks, which doesn't seem like a good thing to me.

Well, I do not agree, but this wasn't written by me. Just in case, all these locks above are not really global, they are per-sb, but this is minor.

And the patches which changed sb->s_writers to use percpu_rw_semaphore/rcu_sync didn't change this logic.

Except the old implementation was buggy, and the readers were slower than now.

Quoted 2 lines

I agree that there are use cases for beginning-of-time __rcu_sync_enter() or whatever we end up naming it.

OK, at least iiuc you agree that cgroup_init() can use __rcu_sync_enter(). As for other potential use-cases, we will disccuss this later. I will have to CC you anyway ;)

So I'll send v2 with renames after I test it. Thanks again.

Can you also make clear which patches of PeterZ's I should be adding as well for testing? I've lost the plot as to what goes with what..

Quoted 3 lines · John Stultz

Can you also make clear which patches of PeterZ's I should be adding as well for testing? I've lost the plot as to what goes with what..

Well, my understanding is that Peter is going to send v2 with some minor changes, I am going to send this patch on top of his series.

Paul, sorry for delay.

Quoted 35 lines · Paul E. McKenney

Quoted 33 lines · Oleg Nesterov

Quoted 12 lines · Paul E. McKenney

Quoted 9 lines · Oleg Nesterov

Now, suppose we add the additional enter/exit's:

	freeze_super(sb)
	{
		// this doesn't block
		__rcu_sync_enter(SEM3);
		__rcu_sync_enter(SEM2);
		__rcu_sync_enter(SEM1);

Quoted 7 lines

Quoted 3 lines

		down_write(&sb->s_umount);
		if (NEED_TO_FREEZE) {
			percpu_down_write(SEM1);

The above waits for the grace period initiated by __rcu_sync_enter(), correct? Presumably "yes", because it will invoke rcu_sync_enter(), which will see the state as GP_ENTER, and will thus wait.

But if down_write() blocks and/or NEED_TO_FREEZE takes some time it could already see the GP_PASSED state, or at least it can sleep less.

Quoted 5 lines

But your point is that if !NEED_TO_FREEZE, we will get here without waiting for a grace period.

But why aren't the __rcu_sync_enter() and rcu_sync_exit() calls inside the "if" statement?

Yes, if we do __rcu_sync_enter() inside "if", then rcu_sync_exit() can't hit GP_ENTER.

Quoted 6 lines

Quoted 2 lines

But why we should disallow this use-case? It does not complicate the code at all.

I do agree that it doesn't complicate the current implementation. But it relies on a global lock, so I am not at all confident that this implementation is the final word.

Hmm. which global lock? Or did you mean freeze_super(), not rcu_sync?

Quoted 3 lines

And speaking of global locks, failing to discourage the pattern above means that the code is unnecessarily acquiring three global locks, which doesn't seem like a good thing to me.

Well, I do not agree, but this wasn't written by me. Just in case, all these locks above are not really global, they are per-sb, but this is minor.

And the patches which changed sb->s_writers to use percpu_rw_semaphore/rcu_sync didn't change this logic.

Except the old implementation was buggy, and the readers were slower than now.

Quoted 2 lines

I agree that there are use cases for beginning-of-time __rcu_sync_enter() or whatever we end up naming it.

OK, at least iiuc you agree that cgroup_init() can use __rcu_sync_enter(). As for other potential use-cases, we will disccuss this later. I will have to CC you anyway ;)

So I'll send v2 with renames after I test it. Thanks again.