On 4/11/2017 7:35 AM, Alan Stern wrote:
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.
I see.. I'll update the patch with your analysis.
Thanks Alan,
Thinh Nguyen
--
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
