On Mon, 10 Apr 2017, Thinh Nguyen wrote: > >>>> To fix this problem, both the smp_rmb() in sleep_thread() and the > >>>> smp_wmb() in wakeup_thread() should be changed to smp_mb(). > > Wouldn't it be sufficient to have smp_mb() just either in the > sleep_thread() or the wakeup_thread() in this case? I don't think so, not in this case. The real problem here is that the code in sleep_thread() tests thread_wakeup_needed, but the actual condition it wants to see is bh->state == BH_STATE_FULL. This is one of the reasons the code should be rewritten. > With just 1 smp_mb(), either bh->state or thread_wakeup_needed is set in > time to break the loop depending on when the wakeup_thread() enters. > > Case 1: smp_mb() in sleep_thread() > bh->state = BH_STATE_FULL; > smp_wmb(); > thread_wakeup_needed = 0; thread_wakeup_needed = 1; > --> smp_mb(); > if (bh->state != BH_STATE_FULL) > //breaks out of loop from bh->state check This code would be okay on x86, but it wouldn't work on PowerPC. You could end up with thread_wakeup_needed = 0 at the end, but the first CPU could still see bh->state != BH_STATE_FULL. So it would put itself back to sleep and never get woken up. > Case 2: smp_mb() in wakeup_thread() > > thread_wakeup_needed = 0; > smp_rmb(); > if (bh->state != BH_STATE_FULL) > bh->state = BH_STATE_FULL; > --> smp_mb(); > thread_wakeup_needed = 1; > wake_up_process() > set_current_state(intr) //smp_wb > if (thread_wakeup_need) > //breaks out of loop from thread_wakeup_needed check That is not the right description. It should look like this: Case 2: smp_mb() in wakeup_thread() bh->state = BH_STATE_FULL; --> smp_mb(); thread_wakeup_needed = 0; thread_wakeup_needed = 1; smp_rmb(); wake_up_process(); if (bh->state != BH_STATE_FULL) set_current_state(intr) //smp_wb if (thread_wakeup_needed) //breaks out of loop from thread_wakeup_needed check This could fail even on x86; the CPU could still not see BH_STATE_FULL because the smp_rmb and the read of bh->state could be reordered before the write to thread_wakeup_needed. > The issue won't be seen when I tested for both cases with smp_mb() in > either the wakeup_thread() or the sleep_thread(). I notice that in > wait_queue (DEFINED_WAIT_FUNC()), it places the smp_mb() in the wait > function, so I follow its implementation. > > However, my test maybe insufficient and I maybe wrong in my code review.. For now, I think it would be best to put smp_mb() in both places. After I do a more thorough rewrite, the memory barriers will make more sense. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
