Re: + kthread-add-a-missing-memory-barrier-to-kthread_stop.patch added to -mm tree

["Dmitry Adamushko" <dmitry.adamushko@gmail.com>]

On 23/02/2008, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>  On Sat, 23 Feb 2008, Oleg Nesterov wrote:
>  >
>
> > In short: wake_up_process() doesn't imply mb(), this means that _in theory_
>  > the commonly used code like
>  >
>  >       set_current_state(TASK_INTERRUPTIBLE);
>  >       if (CONDITION)
>  >               return;
>  >       schedule();
>  >
>  > is racy wrt
>  >
>  >       CONDITION = 1;
>  >       wake_up_process(p);
>  >
>  > I'll be happy to be wrong though, please correct me.
>
>
> Well, you should be wrong on x86, because the spinlock at the head of
>  wake_up_process() (well, "try_to_wake_up()" to be exact) will be a full
>  memory barrier.
>
>  But yeah, in general spinlocks can have weaker semantics, and let
>  preceding writes percolate into the critical section and thus past the
>  point that actually sets task->state.
>
>  And I do agree that we should *not* add a memory barrier in the caller
>  (that's just going to be really confusing for everybody, and make these
>  things much harder than they should be), and we should make sure that the
>  above sequence is always race-free.
>
>  I also think that a full memory barrier is overkill. We should be ok with
>  just adding a write barrier to the top of wake_up_process(), no?

No, wmb is not enough. I've provided an explanation in the original thread.
(http://groups.google.com/group/fa.linux.kernel/browse_thread/thread/44c45685680585fc/e58785df0eeee6f8?lnk=raot)

Actually, there seems to be _no_ problem at all, provided a task to be
woken up is _not_ running on another CPU at the exact moment of
wakeup.

Why?

shared_data = new;
wake_up_task(p);

(1) try_to_wake_up() holds a lock of the runqueue on which 'p' is to-be-placed ;

(2) it's _guaranteed_ that 'shared_data' will be updated by the moment
any UNLOCK is called  -- in our case, try_to_wake_up(p) calls
unlock(&rq->lock);

(3) for 'p' to start running, something must call schedule() -> which
will take 'rq->lock'... and 'rq' is the same as in (2).

IOW, 'p' can't start running untill try_to_wake_up(p) releases
'rq->lock', and as said in (2), that implies that 'shared_data' will
be up-to-date by this moment.

IOW #2, 'rq->lock' is kind of a synchronization point/'barrier' in this case.

does it make sense now?

Another point is if 'p' is actually _running_ on another CPU at the
time when we do try_to_wake_up()... and I guess, the potential problem
is only relevant for situations like below:

CPU #0:

EIP_1 ---> (*)   /* so 'p' is at this point now */

set_current_state(TASK_INTERRUPTIBLE);

if (shared_data == magic)
        schedule();


CPU #1:

shared_data = magic;
try_to_wake_up(p);

now the problem is if 'p->state' (inside try_to_wake_up()) will be
loaded _before_ 'shared_data' has been updated.

recall, we are about to execute set_current_state(TASK_INTERRUPTIBLE)
on CPU #0...

so

p->state is still TASK_RUNNING, meaning that try_to_wake_up() just
exits ! (nothing to be done)

in the mean time,

on CPU #0: set_current_state(TASK_INTERRUPTIBLE) is called.

shared_data is checked _but_ it's still an old value (CPU #1 is still
inside try_to_wake_up(p))
so we call schedule() and go to sleep...

i.e. we actually lost a wakeup.

to sum it up, for the following scheme to work:

set_current_state(TASK_INTERRUPTIBLE);  <--- here we have a smb_mb()
if (condition)
        schedule();

effectively => (1) MODIFY(current->state) ; (2) LOAD(condition)

and a wake_up path must ensure access (LOAD or MODIFY) to the same
data happens in the _reverse_ order:

condition = new;
smb_mb();
try_to_wake_up();

=> (1) MODIFY(condition); (2) LOAD(current->state)

try_to_wake_up() does not need to be a full mb per se, the only
requirement (and only for situation like above) is that there is a
full mb between possible write ops. that have taken place before
try_to_wake_up() _and_ a load of p->state inside try_to_wake_up().

does it make sense #2 ? :-)

(yeah, maybe I'm just too paranoid :-)


>
>                         Linus
>

-- 
Best regards,
Dmitry Adamushko