* [PATCH RFC kenrel/rcu] Eliminate BUG_ON() for sync.c @ 2018-10-22 14:52 Paul E. McKenney 2018-10-22 15:24 ` Oleg Nesterov 0 siblings, 1 reply; 11+ messages in thread From: Paul E. McKenney @ 2018-10-22 14:52 UTC (permalink / raw) To: oleg, peterz; +Cc: linux-kernel, josh, rostedt, mathieu.desnoyers, jiangshanlai 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); } } ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH RFC kenrel/rcu] Eliminate BUG_ON() for sync.c 2018-10-22 14:52 [PATCH RFC kenrel/rcu] Eliminate BUG_ON() for sync.c Paul E. McKenney @ 2018-10-22 15:24 ` Oleg Nesterov 2018-10-22 15:56 ` Paul E. McKenney 0 siblings, 1 reply; 11+ messages in thread From: Oleg Nesterov @ 2018-10-22 15:24 UTC (permalink / raw) To: Paul E. McKenney Cc: peterz, linux-kernel, josh, rostedt, mathieu.desnoyers, jiangshanlai 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH RFC kenrel/rcu] Eliminate BUG_ON() for sync.c 2018-10-22 15:24 ` Oleg Nesterov @ 2018-10-22 15:56 ` Paul E. McKenney 2018-10-22 16:14 ` Oleg Nesterov 0 siblings, 1 reply; 11+ messages in thread From: Paul E. McKenney @ 2018-10-22 15:56 UTC (permalink / raw) To: Oleg Nesterov Cc: peterz, linux-kernel, josh, rostedt, mathieu.desnoyers, jiangshanlai 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH RFC kenrel/rcu] Eliminate BUG_ON() for sync.c 2018-10-22 15:56 ` Paul E. McKenney @ 2018-10-22 16:14 ` Oleg Nesterov 2018-10-30 17:55 ` Paul E. McKenney 0 siblings, 1 reply; 11+ messages in thread From: Oleg Nesterov @ 2018-10-22 16:14 UTC (permalink / raw) To: Paul E. McKenney Cc: peterz, linux-kernel, josh, rostedt, mathieu.desnoyers, jiangshanlai 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH RFC kenrel/rcu] Eliminate BUG_ON() for sync.c 2018-10-22 16:14 ` Oleg Nesterov @ 2018-10-30 17:55 ` Paul E. McKenney 2018-10-31 17:26 ` Oleg Nesterov 0 siblings, 1 reply; 11+ messages in thread From: Paul E. McKenney @ 2018-10-30 17:55 UTC (permalink / raw) To: Oleg Nesterov Cc: peterz, linux-kernel, josh, rostedt, mathieu.desnoyers, jiangshanlai 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)); ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH RFC kenrel/rcu] Eliminate BUG_ON() for sync.c 2018-10-30 17:55 ` Paul E. McKenney @ 2018-10-31 17:26 ` Oleg Nesterov 2018-10-31 17:33 ` Paul E. McKenney 0 siblings, 1 reply; 11+ messages in thread From: Oleg Nesterov @ 2018-10-31 17:26 UTC (permalink / raw) To: Paul E. McKenney Cc: peterz, linux-kernel, josh, rostedt, mathieu.desnoyers, jiangshanlai 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)); > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH RFC kenrel/rcu] Eliminate BUG_ON() for sync.c 2018-10-31 17:26 ` Oleg Nesterov @ 2018-10-31 17:33 ` Paul E. McKenney 2018-11-01 14:12 ` Oleg Nesterov 0 siblings, 1 reply; 11+ messages in thread From: Paul E. McKenney @ 2018-10-31 17:33 UTC (permalink / raw) To: Oleg Nesterov Cc: peterz, linux-kernel, josh, rostedt, mathieu.desnoyers, jiangshanlai 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); } } ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH RFC kenrel/rcu] Eliminate BUG_ON() for sync.c 2018-10-31 17:33 ` Paul E. McKenney @ 2018-11-01 14:12 ` Oleg Nesterov 2018-11-01 14:42 ` Paul E. McKenney 0 siblings, 1 reply; 11+ messages in thread From: Oleg Nesterov @ 2018-11-01 14:12 UTC (permalink / raw) To: Paul E. McKenney Cc: peterz, linux-kernel, josh, rostedt, mathieu.desnoyers, jiangshanlai 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> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH RFC kenrel/rcu] Eliminate BUG_ON() for sync.c 2018-11-01 14:12 ` Oleg Nesterov @ 2018-11-01 14:42 ` Paul E. McKenney 2018-11-01 15:45 ` Oleg Nesterov 0 siblings, 1 reply; 11+ messages in thread From: Paul E. McKenney @ 2018-11-01 14:42 UTC (permalink / raw) To: Oleg Nesterov Cc: peterz, linux-kernel, josh, rostedt, mathieu.desnoyers, jiangshanlai 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH RFC kenrel/rcu] Eliminate BUG_ON() for sync.c 2018-11-01 14:42 ` Paul E. McKenney @ 2018-11-01 15:45 ` Oleg Nesterov 2018-11-01 17:03 ` Paul E. McKenney 0 siblings, 1 reply; 11+ messages in thread From: Oleg Nesterov @ 2018-11-01 15:45 UTC (permalink / raw) To: Paul E. McKenney Cc: peterz, linux-kernel, josh, rostedt, mathieu.desnoyers, jiangshanlai 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); } } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH RFC kenrel/rcu] Eliminate BUG_ON() for sync.c 2018-11-01 15:45 ` Oleg Nesterov @ 2018-11-01 17:03 ` Paul E. McKenney 0 siblings, 0 replies; 11+ messages in thread From: Paul E. McKenney @ 2018-11-01 17:03 UTC (permalink / raw) To: Oleg Nesterov Cc: peterz, linux-kernel, josh, rostedt, mathieu.desnoyers, jiangshanlai 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); > } > } > > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-11-01 17:03 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-22 14:52 [PATCH RFC kenrel/rcu] Eliminate BUG_ON() for sync.c Paul E. McKenney 2018-10-22 15:24 ` Oleg Nesterov 2018-10-22 15:56 ` Paul E. McKenney 2018-10-22 16:14 ` Oleg Nesterov 2018-10-30 17:55 ` Paul E. McKenney 2018-10-31 17:26 ` Oleg Nesterov 2018-10-31 17:33 ` Paul E. McKenney 2018-11-01 14:12 ` Oleg Nesterov 2018-11-01 14:42 ` Paul E. McKenney 2018-11-01 15:45 ` Oleg Nesterov 2018-11-01 17:03 ` Paul E. McKenney
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).