Re: [PATCH] sched/completion: completion_done() should serialize with complete()
On 02/16, Peter Zijlstra wrote: > > On Thu, Feb 12, 2015 at 08:59:13PM +0100, Oleg Nesterov wrote: > > Commit de30ec47302c "Remove unnecessary ->wait.lock serialization when > > reading completion state" was not correct, without lock/unlock the code > > like stop_machine_from_inactive_cpu() > > > > while (!completion_done()) > > cpu_relax(); > > > > can return before complete() finishes its spin_unlock() which writes to > > this memory. And spin_unlock_wait(). > > > > While at it, change try_wait_for_completion() to use READ_ONCE(). > > So I share Davidlohrs concern Ah. I forgot to reply to Davidlohr's email. Sorry. > if we should not simply revert that > change; but given we've now gone over it detail I suppose we should just > keep the optimized version. Yes, I was going to say that of course I won't argue if we simply revert that commit. As he rigthly pointed the lockless check doesn't make sense performance-wise. However, this code needs a comment to explain why we can't simply check ->done and return, unlock_wait() is more documentation than optimization. But, > I did add a comment to your patch; and queued the below for > sched/urgent. Thanks! Now this logic is actually documented ;) unlock_wait() alone could confuse the reader too. Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched/completion: completion_done() should serialize with complete()
On Thu, Feb 12, 2015 at 08:59:13PM +0100, Oleg Nesterov wrote: > Commit de30ec47302c "Remove unnecessary ->wait.lock serialization when > reading completion state" was not correct, without lock/unlock the code > like stop_machine_from_inactive_cpu() > > while (!completion_done()) > cpu_relax(); > > can return before complete() finishes its spin_unlock() which writes to > this memory. And spin_unlock_wait(). > > While at it, change try_wait_for_completion() to use READ_ONCE(). So I share Davidlohrs concern if we should not simply revert that change; but given we've now gone over it detail I suppose we should just keep the optimized version. I did add a comment to your patch; and queued the below for sched/urgent. --- Subject: sched/completion: completion_done() should serialize with complete() From: Oleg Nesterov Date: Thu, 12 Feb 2015 20:59:13 +0100 Commit de30ec47302c "Remove unnecessary ->wait.lock serialization when reading completion state" was not correct, without lock/unlock the code like stop_machine_from_inactive_cpu() while (!completion_done()) cpu_relax(); can return before complete() finishes its spin_unlock() which writes to this memory. And spin_unlock_wait(). While at it, change try_wait_for_completion() to use READ_ONCE(). Fixes: de30ec47302c ("sched/completion: Remove unnecessary ->wait.lock serialization when reading completion state") Cc: waiman.l...@hp.com Cc: raghavendra...@linux.vnet.ibm.com Cc: d...@stgolabs.net Cc: Nicholas Mc Guire Cc: Linus Torvalds Reported-by: "Paul E. McKenney" Tested-by: "Paul E. McKenney" Reported-by: Davidlohr Bueso Signed-off-by: Oleg Nesterov [peterz: Add a comment with the barrier] Signed-off-by: Peter Zijlstra (Intel) Link: http://lkml.kernel.org/r/20150212195913.ga30...@redhat.com --- kernel/sched/completion.c | 19 +-- 1 file changed, 17 insertions(+), 2 deletions(-) --- a/kernel/sched/completion.c +++ b/kernel/sched/completion.c @@ -274,7 +274,7 @@ bool try_wait_for_completion(struct comp * first without taking the lock so we can * return early in the blocking case. */ - if (!ACCESS_ONCE(x->done)) + if (!READ_ONCE(x->done)) return 0; spin_lock_irqsave(&x->wait.lock, flags); @@ -297,6 +297,21 @@ EXPORT_SYMBOL(try_wait_for_completion); */ bool completion_done(struct completion *x) { - return !!ACCESS_ONCE(x->done); + if (!READ_ONCE(x->done)) + return false; + + /* +* If ->done, we need to wait for complete() to release ->wait.lock +* otherwise we can end up freeing the completion before complete() +* is done referencing it. +* +* The RMB pairs with complete()'s RELEASE of ->wait.lock and orders +* the loads of ->done and ->wait.lock such that we cannot observe +* the lock before complete() acquires it while observing the ->done +* after it's acquired the lock. +*/ + smp_rmb(); + spin_unlock_wait(&x->wait.lock); + return true; } EXPORT_SYMBOL(completion_done); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched/completion: completion_done() should serialize with complete()
On Fri, 2015-02-13 at 13:56 -0800, Davidlohr Bueso wrote: > On Thu, 2015-02-12 at 20:59 +0100, Oleg Nesterov wrote: > > Commit de30ec47302c "Remove unnecessary ->wait.lock serialization when > > reading completion state" was not correct, without lock/unlock the code > > like stop_machine_from_inactive_cpu() > > > > while (!completion_done()) > > cpu_relax(); > > > > can return before complete() finishes its spin_unlock() which writes to > > this memory. And spin_unlock_wait(). > > How about reverting the patch altogether? > > This was never a problem nor have I ever seen a performance issues in > completions that would merit these lockless checks. The commit changelog > has *zero* information, so I don't know if this was ever a real issue. > hmm I guess you're patch is more optimal tho, because we don't update the lock, less cacheline bouncing issues etc. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched/completion: completion_done() should serialize with complete()
On Thu, 2015-02-12 at 20:59 +0100, Oleg Nesterov wrote: > Commit de30ec47302c "Remove unnecessary ->wait.lock serialization when > reading completion state" was not correct, without lock/unlock the code > like stop_machine_from_inactive_cpu() > > while (!completion_done()) > cpu_relax(); > > can return before complete() finishes its spin_unlock() which writes to > this memory. And spin_unlock_wait(). How about reverting the patch altogether? This was never a problem nor have I ever seen a performance issues in completions that would merit these lockless checks. The commit changelog has *zero* information, so I don't know if this was ever a real issue. Thanks, Davidlohr -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched/completion: completion_done() should serialize with complete()
On Thu, Feb 12, 2015 at 08:59:13PM +0100, Oleg Nesterov wrote: > Commit de30ec47302c "Remove unnecessary ->wait.lock serialization when > reading completion state" was not correct, without lock/unlock the code > like stop_machine_from_inactive_cpu() > > while (!completion_done()) > cpu_relax(); > > can return before complete() finishes its spin_unlock() which writes to > this memory. And spin_unlock_wait(). > > While at it, change try_wait_for_completion() to use READ_ONCE(). > > Reported-by: "Paul E. McKenney" > Reported-by: Davidlohr Bueso > Signed-off-by: Oleg Nesterov So I am having some difficulty reproducing the original problem, but the patch passes rcutorture testing. So... Tested-by: Paul E. McKenney > --- x/kernel/sched/completion.c > +++ x/kernel/sched/completion.c > @@ -274,7 +274,7 @@ bool try_wait_for_completion(struct comp >* first without taking the lock so we can >* return early in the blocking case. >*/ > - if (!ACCESS_ONCE(x->done)) > + if (!READ_ONCE(x->done)) > return 0; > > spin_lock_irqsave(&x->wait.lock, flags); > @@ -297,6 +297,11 @@ EXPORT_SYMBOL(try_wait_for_completion); > */ > bool completion_done(struct completion *x) > { > - return !!ACCESS_ONCE(x->done); > + if (!READ_ONCE(x->done)) > + return false; > + > + smp_rmb(); > + spin_unlock_wait(&x->wait.lock); > + return true; > } > EXPORT_SYMBOL(completion_done); > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/