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.