Hi Manfred,
Sorry, I don't understand, perhaps you misunderstood me too.
On 12/28, Manfred Spraul wrote:
>
> >Even simpler,
> >
> > void wait(void)
> > {
> > DEFINE_WAIT(entry);
> >
> > __set_current_state(XXX);
> > add_wait_queue(WQ, entry);
> >
> > if (!CONDITION)
> > schedule();
> >
> > remove_wait_queue(WQ, entry);
> > __set_current_state(TASK_RUNNING);
> > }
> >
> >This code is ugly but currently correct unless I am totally confused.
What I tried to say: the code above is another (simpler) example of
the currently correct (afaics) code which will be broken by your patch.
Of course, wait() assumes that
void wake(void)
{
CONDITION = 1;
wake_up(WQ);
}
calls __wake_up_common_lock() and takes WQ->lock unconditionally, and
thus wait() doesn't need the additional barries.
> And: Your proposal is in conflict with
>
> https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/kernel/fork.c?h=v2.6.0&id=e220fdf7a39b54a758f4102bdd9d0d5706aa32a7
I proposed nothing ;) But yes sure, this code doesn't match the comment
above waitqueue_active(), and that is why the wake() paths can't check
list_empty() to avoid __wake_up_common_lock().
> But I do not see the issue, the worst possible scenario should be something like:
>
> // add_wait_queue
> spin_lock(WQ->lock);
> LOAD(CONDITION); // false!
> list_add(entry, head);
> STORE(current_state)
> spin_unlock(WQ->lock);
Again, wake() can happen between LOAD() and list_add()...
But sorry again, I guess I completely misunderstood you...
Oleg.
