Eliminate BUG_ON() for sync.c

RFC PATCH In review Paul E. McKenney 2 participants 11 messages Oct 22, 2018 – Nov 1, 2018

Subsystems (2)

RCU Sleepable read-copy update (SRCU)

lore

Share & tools

lore

Paul E. McKenney

author

2018-10-22 14:52 UTC #0 lore raw

RFC PATCH 1/1 Eliminate BUG_ON() for sync.c

The sync.c file has a number of calls to BUG_ON(), which panics the kernel, which is not a good strategy for devices (like embedded) that don't have a way to capture console output. This commit therefore changes these BUG_ON() calls to WARN_ON_ONCE(), but does so quite naively.

Improved changes welcome. ;-)

Reported-by Linus Torvalds <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Peter Zijlstra <[email protected]>

kernel/rcu/sync.c +7 -7

index 3f943efcf61c..28ac78ba0656 100644
@@ -125,12 +125,12 @@ void rcu_sync_enter(struct rcu_sync *rsp)
 		rsp->gp_state = GP_PENDING;
 	spin_unlock_irq(&rsp->rss_lock);
 
-	BUG_ON(need_wait && need_sync);
-
 	if (need_sync) {
 		gp_ops[rsp->gp_type].sync();
 		rsp->gp_state = GP_PASSED;
 		wake_up_all(&rsp->gp_wait);
+		if (WARN_ON_ONCE(need_wait))
+			wait_event(rsp->gp_wait, rsp->gp_state == GP_PASSED);
 	} else if (need_wait) {
 		wait_event(rsp->gp_wait, rsp->gp_state == GP_PASSED);
 	} else {

@@ -139,7 +139,7 @@ void rcu_sync_enter(struct rcu_sync *rsp)
 		 * Nobody has yet been allowed the 'fast' path and thus we can
 		 * avoid doing any sync(). The callback will get 'dropped'.
 		 */
-		BUG_ON(rsp->gp_state != GP_PASSED);
+		WARN_ON_ONCE(rsp->gp_state != GP_PASSED);
 	}
 }

@@ -166,8 +166,8 @@ static void rcu_sync_func(struct rcu_head *rhp)
 	struct rcu_sync *rsp = container_of(rhp, struct rcu_sync, cb_head);
 	unsigned long flags;
 
-	BUG_ON(rsp->gp_state != GP_PASSED);
-	BUG_ON(rsp->cb_state == CB_IDLE);
+	WARN_ON_ONCE(rsp->gp_state != GP_PASSED);
+	WARN_ON_ONCE(rsp->cb_state == CB_IDLE);
 
 	spin_lock_irqsave(&rsp->rss_lock, flags);
 	if (rsp->gp_count) {

@@ -225,7 +225,7 @@ void rcu_sync_dtor(struct rcu_sync *rsp)
 {
 	int cb_state;
 
-	BUG_ON(rsp->gp_count);
+	WARN_ON_ONCE(rsp->gp_count);
 
 	spin_lock_irq(&rsp->rss_lock);
 	if (rsp->cb_state == CB_REPLAY)

@@ -235,6 +235,6 @@ void rcu_sync_dtor(struct rcu_sync *rsp)
 
 	if (cb_state != CB_IDLE) {
 		gp_ops[rsp->gp_type].wait();
-		BUG_ON(rsp->cb_state != CB_IDLE);
+		WARN_ON_ONCE(rsp->cb_state != CB_IDLE);
 	}
 }

Oleg Nesterov

2018-10-22 15:24 UTC #1 lore raw

Quoted 3 lines · Paul E. McKenney

The sync.c file has a number of calls to BUG_ON(), which panics the kernel, which is not a good

Agreed.

I added these BUG_ON's for documentation when I was prototyping this code, perhaps we can simply remove them.

kernel/rcu/sync.c void rcu_sync_enter(struct rcu_sync *rsp) · line 133

Quoted 12 lines

kernel/rcu/sync.c +2 -2

@@ -125,12 +125,12 @@ void rcu_sync_enter(struct rcu_sync *rsp)
 		rsp->gp_state = GP_PENDING;
 	spin_unlock_irq(&rsp->rss_lock);
 
-	BUG_ON(need_wait && need_sync);
-
 	if (need_sync) {
 		gp_ops[rsp->gp_type].sync();
 		rsp->gp_state = GP_PASSED;
 		wake_up_all(&rsp->gp_wait);
+		if (WARN_ON_ONCE(need_wait))
+			wait_event(rsp->gp_wait, rsp->gp_state == GP_PASSED);

This wait_event(gp_state == GP_PASSED) is pointless, note that this branch does gp_state = GP_PASSED 2 lines above.

And if we add WARN_ON_ONCE(need_wait), then we should probably also add WARN_ON_ONCE(need_sync) into the next "if (need_wait)" branch just for symmetry.

So I'd suggest to either turn that BUG_ON(need_wait && need_sync) above into WARN_ON_ONCE(wait && sync) or simply remove it.

Again, the only purpose of this BUG_ON() is to explain to the reader that it is not (must not be) possible that, say, gp_state == GP_IDLE while gp_count != 0.

----------------------------------------------------------------------------
Damn.

This suddenly reminds me that I rewrote this code completely, and you even reviewed the new implementation and (iirc) acked it!

However, I failed to force myself to rewrite the comments, and that is why I didn't send the "official" patch :/

May be some time...

Oleg.

Paul E. McKenney

author

2018-10-22 15:56 UTC #2 lore raw

Quoted 9 lines · Oleg Nesterov

Quoted 3 lines · Paul E. McKenney
The sync.c file has a number of calls to BUG_ON(), which panics the kernel, which is not a good
Agreed.
I added these BUG_ON's for documentation when I was prototyping this code, perhaps we can simply remove them.

I do like this approach. ;-)

Quoted 4 lines

kernel/rcu/sync.c +0 -0

@@ -125,12 +125,12 @@ void rcu_sync_enter(struct rcu_sync *rsp)
 		rsp->gp_state = GP_PENDING;
 	spin_unlock_irq(&rsp->rss_lock);

Quoted 11 lines

kernel/rcu/sync.c void rcu_sync_enter(struct rcu_sync *rsp) · line 133
Quoted 8 lines
kernel/rcu/sync.c +2 -2
-	BUG_ON(need_wait && need_sync);
-
 	if (need_sync) {
 		gp_ops[rsp->gp_type].sync();
 		rsp->gp_state = GP_PASSED;
 		wake_up_all(&rsp->gp_wait);
+		if (WARN_ON_ONCE(need_wait))
+			wait_event(rsp->gp_wait, rsp->gp_state == GP_PASSED);
This wait_event(gp_state == GP_PASSED) is pointless, note that this branch does gp_state = GP_PASSED 2 lines above.

OK, I have removed this one.

Quoted 3 lines

And if we add WARN_ON_ONCE(need_wait), then we should probably also add WARN_ON_ONCE(need_sync) into the next "if (need_wait)" branch just for symmetry.

But in that case, the earlier "if" prevents "need_sync" from ever getting there, unless I lost the thread here.

Quoted 2 lines

So I'd suggest to either turn that BUG_ON(need_wait && need_sync) above into WARN_ON_ONCE(wait && sync) or simply remove it.

I chose WARN_ON_ONCE() for this one.

Quoted 3 lines

Again, the only purpose of this BUG_ON() is to explain to the reader that it is not (must not be) possible that, say, gp_state == GP_IDLE while gp_count != 0.

Good point!

Should I remove the others?

Quoted 10 lines

----------------------------------------------------------------------------
Damn.
This suddenly reminds me that I rewrote this code completely, and you even reviewed the new implementation and (iirc) acked it!
However, I failed to force myself to rewrite the comments, and that is why I didn't send the "official" patch :/
May be some time...

Could you please point me at the last email thread? Yes, I should be able to find it, but I would probably get the wrong one. :-/

Thanx, Paul

Oleg Nesterov

2018-10-22 16:14 UTC #3 lore raw

Quoted 17 lines · Paul E. McKenney

Quoted 5 lines
Quoted 5 lines
kernel/rcu/sync.c +0 -0
@@ -125,12 +125,12 @@ void rcu_sync_enter(struct rcu_sync *rsp)
 		rsp->gp_state = GP_PENDING;
 	spin_unlock_irq(&rsp->rss_lock);
 
Quoted 11 lines
kernel/rcu/sync.c void rcu_sync_enter(struct rcu_sync *rsp) · line 133
Quoted 8 lines
kernel/rcu/sync.c +2 -2
-	BUG_ON(need_wait && need_sync);
-
 	if (need_sync) {
 		gp_ops[rsp->gp_type].sync();
 		rsp->gp_state = GP_PASSED;
 		wake_up_all(&rsp->gp_wait);
+		if (WARN_ON_ONCE(need_wait))
+			wait_event(rsp->gp_wait, rsp->gp_state == GP_PASSED);
This wait_event(gp_state == GP_PASSED) is pointless, note that this branch does gp_state = GP_PASSED 2 lines above.

Quoted 8 lines

OK, I have removed this one.
Quoted 3 lines
And if we add WARN_ON_ONCE(need_wait), then we should probably also add WARN_ON_ONCE(need_sync) into the next "if (need_wait)" branch just for symmetry.
But in that case, the earlier "if" prevents "need_sync" from ever getting there, unless I lost the thread here.

Yes, you are right, we would also need to remove "else",

Quoted 1 line

Should I remove the others?

Up to you, I am fine either way.

IOW, feel free to remove this BUG_ON's altogether, or turn them all into WARN_ON_ONCE's, whatever you like more.

Quoted 6 lines

Quoted 6 lines
----------------------------------------------------------------------------
Damn.
This suddenly reminds me that I rewrote this code completely, and you even reviewed the new implementation and (iirc) acked it!

Quoted 7 lines

Quoted 4 lines
However, I failed to force myself to rewrite the comments, and that is why I didn't send the "official" patch :/
May be some time...
Could you please point me at the last email thread? Yes, I should be able to find it, but I would probably get the wrong one. :-/

probably this one,

	[PATCH] rcu_sync: simplify the state machine, introduce __rcu_sync_enter()
	https://lkml.org/lkml/2016/7/16/150

but I am not sure, will recheck tomorrow.

Oleg.

Paul E. McKenney

author

2018-10-30 17:55 UTC #4 lore raw

Quoted 50 lines · Oleg Nesterov

Quoted 17 lines · Paul E. McKenney
Quoted 5 lines
Quoted 5 lines
(quoted diff) +0 -0
@@ -125,12 +125,12 @@ void rcu_sync_enter(struct rcu_sync *rsp)
 		rsp->gp_state = GP_PENDING;
 	spin_unlock_irq(&rsp->rss_lock);
 
Quoted 11 lines
Review void rcu_sync_enter(struct rcu_sync *rsp) · line 133
Quoted 8 lines
(quoted diff) +2 -2
-	BUG_ON(need_wait && need_sync);
-
 	if (need_sync) {
 		gp_ops[rsp->gp_type].sync();
 		rsp->gp_state = GP_PASSED;
 		wake_up_all(&rsp->gp_wait);
+		if (WARN_ON_ONCE(need_wait))
+			wait_event(rsp->gp_wait, rsp->gp_state == GP_PASSED);
This wait_event(gp_state == GP_PASSED) is pointless, note that this branch does gp_state = GP_PASSED 2 lines above.
Quoted 8 lines
OK, I have removed this one.
Quoted 3 lines
And if we add WARN_ON_ONCE(need_wait), then we should probably also add WARN_ON_ONCE(need_sync) into the next "if (need_wait)" branch just for symmetry.
But in that case, the earlier "if" prevents "need_sync" from ever getting there, unless I lost the thread here.
Yes, you are right, we would also need to remove "else",
Quoted 1 line
Should I remove the others?
Up to you, I am fine either way.
IOW, feel free to remove this BUG_ON's altogether, or turn them all into WARN_ON_ONCE's, whatever you like more.
Quoted 6 lines
Quoted 6 lines
----------------------------------------------------------------------------
Damn.
This suddenly reminds me that I rewrote this code completely, and you even reviewed the new implementation and (iirc) acked it!
Quoted 7 lines
Quoted 4 lines
However, I failed to force myself to rewrite the comments, and that is why I didn't send the "official" patch :/
May be some time...
Could you please point me at the last email thread? Yes, I should be able to find it, but I would probably get the wrong one. :-/

Quoted 6 lines

probably this one,
	[PATCH] rcu_sync: simplify the state machine, introduce __rcu_sync_enter()
	https://lkml.org/lkml/2016/7/16/150
but I am not sure, will recheck tomorrow.

Just following up... Here is what I currently have.

Thanx, Paul

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

commit 1c1d315dfb7049d0233b89948a3fbcb61ea15d26

Author: Dennis Krein <[email protected]>
Date:   Fri Oct 26 07:38:24 2018 -0700

srcu: Lock srcu_data structure in srcu_gp_start()

The srcu_gp_start() function is called with the srcu_struct structure's ->lock held, but not with the srcu_data structure's ->lock. This is problematic because this function accesses and updates the srcu_data structure's ->srcu_cblist, which is protected by that lock. Failing to hold this lock can result in corruption of the SRCU callback lists, which in turn can result in arbitrarily bad results.

This commit therefore makes srcu_gp_start() acquire the srcu_data structure's ->lock across the calls to rcu_segcblist_advance() and rcu_segcblist_accelerate(), thus preventing this corruption.

Reported-by Bart Van Assche <[email protected]>

Reported-by Christoph Hellwig <[email protected]>

Signed-off-by Dennis Krein <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

kernel/rcu/srcutree.c +2 -0

index 60f3236beaf7..697a2d7e8e8a 100644
@@ -451,10 +451,12 @@ static void srcu_gp_start(struct srcu_struct *sp)
 
 	lockdep_assert_held(&ACCESS_PRIVATE(sp, lock));
 	WARN_ON_ONCE(ULONG_CMP_GE(sp->srcu_gp_seq, sp->srcu_gp_seq_needed));
+	spin_lock_rcu_node(sdp);  /* Interrupts already disabled. */
 	rcu_segcblist_advance(&sdp->srcu_cblist,
 			      rcu_seq_current(&sp->srcu_gp_seq));
 	(void)rcu_segcblist_accelerate(&sdp->srcu_cblist,
 				       rcu_seq_snap(&sp->srcu_gp_seq));
+	spin_unlock_rcu_node(sdp);  /* Interrupts remain disabled. */
 	smp_mb(); /* Order prior store to ->srcu_gp_seq_needed vs. GP start. */
 	rcu_seq_start(&sp->srcu_gp_seq);
 	state = rcu_seq_state(READ_ONCE(sp->srcu_gp_seq));

Oleg Nesterov

2018-10-31 17:26 UTC #5 lore raw

Quoted 16 lines · Paul E. McKenney

Quoted 14 lines · Oleg Nesterov
Quoted 13 lines
Quoted 10 lines
----------------------------------------------------------------------------
Damn.
This suddenly reminds me that I rewrote this code completely, and you even reviewed the new implementation and (iirc) acked it!
However, I failed to force myself to rewrite the comments, and that is why I didn't send the "official" patch :/
May be some time...
Could you please point me at the last email thread? Yes, I should be able to find it, but I would probably get the wrong one. :-/

Quoted 8 lines

Quoted 6 lines
probably this one,
	[PATCH] rcu_sync: simplify the state machine, introduce __rcu_sync_enter()
	https://lkml.org/lkml/2016/7/16/150
but I am not sure, will recheck tomorrow.
Just following up... Here is what I currently have.

Hmm. Are you sure you replied to the correct message? ;)

the patch below looks absolutely unrelated...

Quoted 45 lines

Thanx, Paul
------------------------------------------------------------------------
commit 1c1d315dfb7049d0233b89948a3fbcb61ea15d26
Author: Dennis Krein <[email protected]>
Date:   Fri Oct 26 07:38:24 2018 -0700
srcu: Lock srcu_data structure in srcu_gp_start()
The srcu_gp_start() function is called with the srcu_struct structure's ->lock held, but not with the srcu_data structure's ->lock. This is problematic because this function accesses and updates the srcu_data structure's ->srcu_cblist, which is protected by that lock. Failing to hold this lock can result in corruption of the SRCU callback lists, which in turn can result in arbitrarily bad results.
This commit therefore makes srcu_gp_start() acquire the srcu_data structure's ->lock across the calls to rcu_segcblist_advance() and rcu_segcblist_accelerate(), thus preventing this corruption.
Reported-by Bart Van Assche <[email protected]>
Reported-by Christoph Hellwig <[email protected]>
Signed-off-by Dennis Krein <[email protected]>
Signed-off-by Paul E. McKenney <[email protected]>
kernel/rcu/srcutree.c +2 -0
index 60f3236beaf7..697a2d7e8e8a 100644
@@ -451,10 +451,12 @@ static void srcu_gp_start(struct srcu_struct *sp)
 
 	lockdep_assert_held(&ACCESS_PRIVATE(sp, lock));
 	WARN_ON_ONCE(ULONG_CMP_GE(sp->srcu_gp_seq, sp->srcu_gp_seq_needed));
+	spin_lock_rcu_node(sdp);  /* Interrupts already disabled. */
 	rcu_segcblist_advance(&sdp->srcu_cblist,
 			      rcu_seq_current(&sp->srcu_gp_seq));
 	(void)rcu_segcblist_accelerate(&sdp->srcu_cblist,
 				       rcu_seq_snap(&sp->srcu_gp_seq));
+	spin_unlock_rcu_node(sdp);  /* Interrupts remain disabled. */
 	smp_mb(); /* Order prior store to ->srcu_gp_seq_needed vs. GP start. */
 	rcu_seq_start(&sp->srcu_gp_seq);
 	state = rcu_seq_state(READ_ONCE(sp->srcu_gp_seq));

Paul E. McKenney

author

2018-10-31 17:33 UTC #6 lore raw

Quoted 22 lines · Oleg Nesterov

Quoted 21 lines · Paul E. McKenney
Quoted 19 lines · Oleg Nesterov
Quoted 6 lines
Quoted 6 lines
----------------------------------------------------------------------------
Damn.
This suddenly reminds me that I rewrote this code completely, and you even reviewed the new implementation and (iirc) acked it!
Quoted 7 lines
Quoted 4 lines
However, I failed to force myself to rewrite the comments, and that is why I didn't send the "official" patch :/
May be some time...
Could you please point me at the last email thread? Yes, I should be able to find it, but I would probably get the wrong one. :-/
probably this one,
	[PATCH] rcu_sync: simplify the state machine, introduce __rcu_sync_enter()
	https://lkml.org/lkml/2016/7/16/150

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
but I am not sure, will recheck tomorrow.
Just following up... Here is what I currently have.
Hmm. Are you sure you replied to the correct message? ;)
the patch below looks absolutely unrelated...

Oy... Right message, wrong commit.

Does the one below look somewhat more relevant? ;-)

Thanx, Paul

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

commit 10314fde6fcdbaaf7c21b539dd2db5933344344f

Author: Paul E. McKenney <[email protected]>
Date:   Mon Oct 22 07:43:22 2018 -0700

rcu: Eliminate BUG_ON() for sync.c

Reported-by Linus Torvalds <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Peter Zijlstra <[email protected]>

kernel/rcu/sync.c +6 -7

index 3f943efcf61c..a6ba446a9693 100644
@@ -125,8 +125,7 @@ void rcu_sync_enter(struct rcu_sync *rsp)
 		rsp->gp_state = GP_PENDING;
 	spin_unlock_irq(&rsp->rss_lock);
 
-	BUG_ON(need_wait && need_sync);
-
+	WARN_ON_ONCE(need_wait && need_sync);
 	if (need_sync) {
 		gp_ops[rsp->gp_type].sync();
 		rsp->gp_state = GP_PASSED;

@@ -139,7 +138,7 @@ void rcu_sync_enter(struct rcu_sync *rsp)
 		 * Nobody has yet been allowed the 'fast' path and thus we can
 		 * avoid doing any sync(). The callback will get 'dropped'.
 		 */
-		BUG_ON(rsp->gp_state != GP_PASSED);
+		WARN_ON_ONCE(rsp->gp_state != GP_PASSED);
 	}
 }

@@ -166,8 +165,8 @@ static void rcu_sync_func(struct rcu_head *rhp)
 	struct rcu_sync *rsp = container_of(rhp, struct rcu_sync, cb_head);
 	unsigned long flags;
 
-	BUG_ON(rsp->gp_state != GP_PASSED);
-	BUG_ON(rsp->cb_state == CB_IDLE);
+	WARN_ON_ONCE(rsp->gp_state != GP_PASSED);
+	WARN_ON_ONCE(rsp->cb_state == CB_IDLE);
 
 	spin_lock_irqsave(&rsp->rss_lock, flags);
 	if (rsp->gp_count) {

@@ -225,7 +224,7 @@ void rcu_sync_dtor(struct rcu_sync *rsp)
 {
 	int cb_state;
 
-	BUG_ON(rsp->gp_count);
+	WARN_ON_ONCE(rsp->gp_count);
 
 	spin_lock_irq(&rsp->rss_lock);
 	if (rsp->cb_state == CB_REPLAY)

@@ -235,6 +234,6 @@ void rcu_sync_dtor(struct rcu_sync *rsp)
 
 	if (cb_state != CB_IDLE) {
 		gp_ops[rsp->gp_type].wait();
-		BUG_ON(rsp->cb_state != CB_IDLE);
+		WARN_ON_ONCE(rsp->cb_state != CB_IDLE);
 	}
 }

Oleg Nesterov

Acked-by

2018-11-01 14:12 UTC #7 lore raw

Quoted 4 lines · Paul E. McKenney

Oy... Right message, wrong commit.
Does the one below look somewhat more relevant? ;-)

Much more relevant ;) feel free to add

Acked-by Oleg Nesterov <[email protected]>

Paul E. McKenney

author Applied

2018-11-01 14:43 UTC #8 lore raw

Quoted 9 lines · Oleg Nesterov

Quoted 4 lines · Paul E. McKenney
Oy... Right message, wrong commit.
Does the one below look somewhat more relevant? ;-)
Much more relevant ;) feel free to add
Acked-by Oleg Nesterov <[email protected]>

Applied, thank you!

Any news on exactly which patch constituted the reworking of this code some time back?

Thanx, Paul

Oleg Nesterov

2018-11-01 15:45 UTC #9 lore raw

Quoted 3 lines · Paul E. McKenney

Any news on exactly which patch constituted the reworking of this code some time back?

Again, I never sent a patch, I simply showed the new code (more than 2 years ago ;), see below. I need to re-read our discussiong, but iirc your and Peter's reviews were mostly positive.

The new implementation (and the state machine) is simpler, plus the new __rcu_sync_enter(). It can be used instead of rcu_sync_enter_start() hack, and by freeze_super() which currently need 3 GP's to take 3 percpu rwsems.

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

#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 };

#ifdef CONFIG_PROVE_RCU
void rcu_sync_lockdep_assert(struct rcu_sync *rsp)

{
	RCU_LOCKDEP_WARN(!gp_ops[rsp->gp_type].held(),
			 "suspicious rcu_sync_is_idle() usage");
}

#endif

void rcu_sync_init(struct rcu_sync *rsp, enum rcu_sync_type type)

{
	memset(rsp, 0, sizeof(*rsp));
	init_waitqueue_head(&rsp->gp_wait);
	rsp->gp_type = type;
}

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;
		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!.
		 *
		 * 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);
	}
}

Paul E. McKenney

author

2018-11-01 17:03 UTC #10 lore raw

Quoted 8 lines · Oleg Nesterov

Quoted 3 lines · Paul E. McKenney
Any news on exactly which patch constituted the reworking of this code some time back?
Again, I never sent a patch, I simply showed the new code (more than 2 years ago ;), see below. I need to re-read our discussiong, but iirc your and Peter's reviews were mostly positive.

I am glad that I didn't try to apply the various related patches I found, then. ;-)

Quoted 16 lines

The new implementation (and the state machine) is simpler, plus the new __rcu_sync_enter(). It can be used instead of rcu_sync_enter_start() hack, and by freeze_super() which currently need 3 GP's to take 3 percpu rwsems.
Oleg.
-------------------------------------------------------------------------------
#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

Quoted 15 lines

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,

In my -rcu tree, these are now call_rcu and rcu_barrier, courtesy of RCU flavor consolidation.

Quoted 1 line

__INIT_HELD(rcu_read_lock_sched_held)

This remains the same.

Quoted 5 lines

	},
	[RCU_BH_SYNC] = {
		.call = call_rcu_bh,
		.wait = rcu_barrier_bh,
		__INIT_HELD(rcu_read_lock_bh_held)

And same for these three.

Thanx, Paul

Quoted 139 lines

	},
};

#define rss_lock gp_wait.lock

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

#ifdef CONFIG_PROVE_RCU
void rcu_sync_lockdep_assert(struct rcu_sync *rsp)

{
	RCU_LOCKDEP_WARN(!gp_ops[rsp->gp_type].held(),
			 "suspicious rcu_sync_is_idle() usage");
}

#endif

void rcu_sync_init(struct rcu_sync *rsp, enum rcu_sync_type type)

{
	memset(rsp, 0, sizeof(*rsp));
	init_waitqueue_head(&rsp->gp_wait);
	rsp->gp_type = type;
}

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;
		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!.
		 *
		 * 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);
	}
}