Hi Waiman,
I have now written the mail 3 times:
Twice I thought that I found a race, but during further analysis, it
always turns out that the spin_lock() is sufficient.
First, to avoid any obvious things: Until the series with e.g.
27d7be1801a4824e, there was a race inside sem_lock().
Thus it was possible that multiple threads were operating on the same
semaphore array, with obviously arbitrary impact.
On 9/20/19 5:54 PM, Waiman Long wrote:
+ /*
+ * A spurious wakeup at the right moment can cause race
+ * between the to-be-woken task and the waker leading to
+ * missed wakeup. Setting state back to TASK_INTERRUPTIBLE
+ * before checking queue.status will ensure that the race
+ * won't happen.
+ *
+ * CPU0 CPU1
+ *
+ * <spurious wakeup> wake_up_sem_queue_prepare():
+ * state = TASK_INTERRUPTIBLE status = error
+ * try_to_wake_up():
+ * smp_mb() smp_mb()
+ * if (status == -EINTR) if (!(p->state & state))
+ * schedule() goto out
+ */
+ set_current_state(TASK_INTERRUPTIBLE);
+
So the the hypothesis is that we have a race due to the optimization
within try_to_wake_up():
If the status is already TASK_RUNNING, then the wakeup is a nop.
Correct?
The waker wants to use:
lock();
set_conditions();
unlock();
as the wake_q is a shared list, completely asynchroneously this will happen:
smp_mb(); //// ***1
if (current->state = TASK_INTERRUPTIBLE) current->state=TASK_RUNNING;
The only guarantee is that this will happen after lock(), it may happen
before set_conditions().
The task that goes to sleep uses:
lock();
check_conditions();
__set_current_state();
unlock(); //// ***2
schedule();
You propose to change that to:
lock();
set_current_state();
check_conditions();
unlock();
schedule();
I don't see a race anymore, and I don't see how the proposed change will
help.
e.g.: __set_current_state() and smp_mb() have paired memory barriers
***1 and ***2 above.
--
Manfred
