lkml.org 
[lkml]   [2019]   [Sep]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] ipc/sem: Fix race between to-be-woken task and waker
From
Date
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

\
 
 \ /
  Last update: 2019-09-29 12:24    [W:0.036 / U:0.248 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site