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.