Messages in this thread | | | Date | Mon, 22 Oct 2018 08:56:44 -0700 | From | "Paul E. McKenney" <> | Subject | Re: [PATCH RFC kenrel/rcu] Eliminate BUG_ON() for sync.c |
| |
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
| |