On Tue, Sep 01, 2015 at 06:39:23PM +0200, Oleg Nesterov wrote: > On 09/01, Boqun Feng wrote: > > > > On Tue, Sep 01, 2015 at 11:59:23AM +0200, Oleg Nesterov wrote: > > > > > > And just in case, wake_up() differs in a sense that it doesn't even need > > > that STORE-LOAD barrier in try_to_wake_up(), we can rely on > > > wait_queue_head_t->lock. Assuming that wake_up() pairs with the "normal" > > > wait_event()-like code. > > Looks like, you have missed this part of my previous email. See below.
I guess I need to think through this, though I haven't found any problem in wake_up() if we remove the STORE-LOAD barrier in try_to_wake_up(). And I know that in wake_up(), try_to_wake_up() will be called with holding wait_queue_head_t->lock, however, only part of wait_event() holds the same lock, I can't figure out why the barrier is not needed because of the lock.. I will read the code carefully to see whether I miss something. > > > I think maybe I have a misunderstanding of barrier pairing. > > Or me. I can only say how it is supposed to work. > > > think that a barrier pairing can only happen: > > Well, no. See for example https://lkml.org/lkml/2014/7/16/310 This helps a lot, so does the LWN article it references: https://lwn.net/Articles/573436/ The barrier pairing here is scenario 10 in that article. I'm sure that is my misunderstanding of barrier pairing now, thank you! > Or, say, the comment in completion_done(). > > And please do not assume I can answer authoritatively the questions > in this area. Fortunately we have paulmck/peterz in CC, they can > correct me. > Your explanation and references help a lot, now I understand how the barrier works in try_to_wake_up() ;-) > Plus I didn't sleep today, not sure I can understand you correctly > and/or answer your question ;) > > > One example of #2 pairing is the following sequence of events: > > > > Initially X = 0, Y = 0 > > > > CPU 1 CPU 2 > > =========================== ================================ > > WRITE_ONCE(Y, 1); > > smp_mb(); > > r1 = READ_ONCE(X); // r1 == 0 > > WRITE_ONCE(X, 1); > > smp_mb(); > > r2 = READ_ONCE(Y); > > > > ---------------------------------------------------------------- > > { assert(!(r1 == 0 && r2 == 0)); // means if r1 == 0 then r2 == 1} > > > > If CPU 1 _fails_ to read the value of X written by CPU 1, r2 is > > guaranteed to 1, which means assert(!(r1 == 0 && r2 == 0)) afterwards > > wouldn't be triggered in any case. > > > > And this is actually the case of wake_up/wait, assuming that > > prepare_to_wait() is called on CPU 1 and wake_up() is called on CPU 2, > > See above, wake_up/wait_event are fine in any case because they use > the same lock. > I think I wanted to say wake_up_proccess() here, which calls try_to_wake_up() directly, sorry about that mistake.. > But as for try_to_wake_up() you are right, we rely on STORE-MB-LOAD. > Now let me quote another previous email, > > So in fact try_to_wake_up() needs mb() before it does > > if (!(p->state & state)) > goto out; > > But mb() is heavy, we can use wmb() instead, but only in this particular > case: before spin_lock(), which guarantees that LOAD(p->state) can't > leak > out of the critical section. And since spin_lock() itself is the STORE, > this guarantees that STORE(CONDITION) can't leak _into_ the critical > section > and thus it can't be reordered with LOAD(p->state). > This also helps a lot, thank you ;-) > And I think that smp_mb__before_spinlock() + spin_lock() should pair > correctly with mb(). If not - we should redefine it. > > > X is the condition and Y is the task state, > > Yes, > > > and replace smp_mb() with really necessary barriers, right? > > Sorry, can't understand this part... > I just want to be accurate by saying that, because the barrier in try_to_wake_up() is not a smp_mb(), is a smp_mb__before_spinlock() + spin_lock(). Regards, Boqun
signature.asc
Description: PGP signature