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

[Oleg Nesterov <oleg@redhat.com>]

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.