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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
