[PATCH RFC kenrel/rcu] Eliminate BUG_ON() for sync.c

[PATCH RFC kenrel/rcu] Eliminate BUG_ON() for sync.c

[Paul E. McKenney]

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 <torvalds@linux-foundation.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.ibm.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>

diff --git a/kernel/rcu/sync.c b/kernel/rcu/sync.c
index 3f943efcf61c..28ac78ba0656 100644
--- a/kernel/rcu/sync.c
+++ b/kernel/rcu/sync.c
@@ -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);
 	}
 }

Re: [PATCH RFC kenrel/rcu] Eliminate BUG_ON() for sync.c

[Oleg Nesterov]

On 10/22, Paul E. McKenney wrote:
>
> 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.

> @@ -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.

Re: [PATCH RFC kenrel/rcu] Eliminate BUG_ON() for sync.c

[Paul E. McKenney]

On Mon, Oct 22, 2018 at 05:24:07PM +0200, Oleg Nesterov wrote:
> On 10/22, Paul E. McKenney wrote:
> >
> > 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.  ;-)

> > @@ -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.

OK, I have removed this one.

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

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

> 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?

> ----------------------------------------------------------------------------
> 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

Re: [PATCH RFC kenrel/rcu] Eliminate BUG_ON() for sync.c

[Oleg Nesterov]

On 10/22, Paul E. McKenney wrote:
>
> > > @@ -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.
>
> OK, I have removed this one.
>
> > 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",

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

> > ----------------------------------------------------------------------------
> > 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.  :-/

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.	

Re: [PATCH RFC kenrel/rcu] Eliminate BUG_ON() for sync.c

[Paul E. McKenney]

On Mon, Oct 22, 2018 at 06:14:40PM +0200, Oleg Nesterov wrote:
> On 10/22, Paul E. McKenney wrote:
> >
> > > > @@ -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.
> >
> > OK, I have removed this one.
> >
> > > 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",
> 
> > 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.
> 
> > > ----------------------------------------------------------------------------
> > > 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.  :-/
> 
> 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 <Dennis.Krein@netapp.com>
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 <bvanassche@acm.org>
    Reported-by: Christoph Hellwig <hch@infradead.org>
    Signed-off-by: Dennis Krein <Dennis.Krein@netapp.com>
    Signed-off-by: Paul E. McKenney <paulmck@linux.ibm.com>

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 60f3236beaf7..697a2d7e8e8a 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -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));

Re: [PATCH RFC kenrel/rcu] Eliminate BUG_ON() for sync.c

[Oleg Nesterov]

On 10/30, Paul E. McKenney wrote:
>
> On Mon, Oct 22, 2018 at 06:14:40PM +0200, Oleg Nesterov wrote:
> > > > ----------------------------------------------------------------------------
> > > > 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.  :-/
> >
> > 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...

>
> 							Thanx, Paul
>
> ------------------------------------------------------------------------
>
> commit 1c1d315dfb7049d0233b89948a3fbcb61ea15d26
> Author: Dennis Krein <Dennis.Krein@netapp.com>
> 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 <bvanassche@acm.org>
>     Reported-by: Christoph Hellwig <hch@infradead.org>
>     Signed-off-by: Dennis Krein <Dennis.Krein@netapp.com>
>     Signed-off-by: Paul E. McKenney <paulmck@linux.ibm.com>
>
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index 60f3236beaf7..697a2d7e8e8a 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -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));
>

Re: [PATCH RFC kenrel/rcu] Eliminate BUG_ON() for sync.c

[Paul E. McKenney]

On Wed, Oct 31, 2018 at 06:26:05PM +0100, Oleg Nesterov wrote:
> On 10/30, Paul E. McKenney wrote:
> >
> > On Mon, Oct 22, 2018 at 06:14:40PM +0200, Oleg Nesterov wrote:
> > > > > ----------------------------------------------------------------------------
> > > > > 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.  :-/
> > >
> > > 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...

Oy...  Right message, wrong commit.

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

							Thanx, Paul

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

commit 10314fde6fcdbaaf7c21b539dd2db5933344344f
Author: Paul E. McKenney <paulmck@linux.ibm.com>
Date:   Mon Oct 22 07:43:22 2018 -0700

    rcu: 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.
    
    Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
    Signed-off-by: Paul E. McKenney <paulmck@linux.ibm.com>
    Cc: Oleg Nesterov <oleg@redhat.com>
    Cc: Peter Zijlstra <peterz@infradead.org>

diff --git a/kernel/rcu/sync.c b/kernel/rcu/sync.c
index 3f943efcf61c..a6ba446a9693 100644
--- a/kernel/rcu/sync.c
+++ b/kernel/rcu/sync.c
@@ -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);
 	}
 }

Re: [PATCH RFC kenrel/rcu] Eliminate BUG_ON() for sync.c

[Oleg Nesterov]

On 10/31, Paul E. McKenney wrote:
>
> Oy...  Right message, wrong commit.
>
> Does the one below look somewhat more relevant?  ;-)

Much more relevant ;) feel free to add

Acked-by: Oleg Nesterov <oleg@redhat.com>

Re: [PATCH RFC kenrel/rcu] Eliminate BUG_ON() for sync.c

[Paul E. McKenney]

On Thu, Nov 01, 2018 at 03:12:18PM +0100, Oleg Nesterov wrote:
> On 10/31, Paul E. McKenney wrote:
> >
> > Oy...  Right message, wrong commit.
> >
> > Does the one below look somewhat more relevant?  ;-)
> 
> Much more relevant ;) feel free to add
> 
> Acked-by: Oleg Nesterov <oleg@redhat.com>

Applied, thank you!

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

							Thanx, Paul

Re: [PATCH RFC kenrel/rcu] Eliminate BUG_ON() for sync.c

[Oleg Nesterov]

On 11/01, Paul E. McKenney wrote:
>
> 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);
	}
}

Re: [PATCH RFC kenrel/rcu] Eliminate BUG_ON() for sync.c

[Paul E. McKenney]

On Thu, Nov 01, 2018 at 04:45:19PM +0100, Oleg Nesterov wrote:
> On 11/01, Paul E. McKenney wrote:
> >
> > 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.  ;-)

> 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,

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

> 		__INIT_HELD(rcu_read_lock_sched_held)

This remains the same.

> 	},
> 	[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

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