lkml.org 
[lkml]   [2013]   [Dec]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/3] wait-simple: Introduce the simple waitqueue implementation
On Thu, 12 Dec 2013 11:51:37 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> >
> > Typically such a barrier comes from set_current_state(), the normal
> > pattern is something like:
> >
> > set_current_state(TASK_UNINTERRUPTIBLE)
> > if (!cond)
> > schedule();
> > __set_current_state(TASK_RUNNING);
> >
> > vs
> >
> > cond = true;
> > wake_up_process(&foo);
>
> Hmm, that __set_current_state() in swait_prepare() does indeed seem
> buggy. I'm surprised that I didn't catch that, as I'm usually a
> stickler with set_current_state() (and I'm very paranoid when it comes
> to using __set_current_state()).
>
> I'll have to dig deeper to see why I didn't change that.

OK, looking at my irc logs discussing this with Paul McKenney, this was
an optimization:

<rostedt> as set_current_state() may be too big of a heavy weight
<rostedt> It's usually to synchronize between wake ups and state stet
<rostedt> set
<rostedt> but both the set state and the wakeup is within the same spin
lock


So if we break up your code above, we have:

raw_spin_lock_irqsave(&head->lock, flags);
w->task = current;
if (list_empty(&w->node)) {
list_add(&w->node, &head->list);
smp_mb();
}
__set_current_state(state);
raw_spin_unlock_irqrestore(&head->lock, flags);

if (!cond)
schedule();


vs

cond = true;

raw_spin_lock_irqsave(&head->lock, flags);
woken = __swait_wake_locked(head, state, num);
raw_spin_unlock_irqrestore(&head->lock, flags);


That is, the change of state with respect to the list is synchronized
by the head->lock. We only need to synchronize seeing the condition
with the adding to the list. Once we are on the list, we get woken up
regardless.

But I think this is a micro optimization, and probably still buggy, as
I can imagine a race if we are already on the list, and we don't call
the memory barrier and miss seeing the condition after being woken up
and resetting ourselves back to a sleeping state.

Paul,

You can remove the smp_mb() from __swait_enqueue() and instead replace
the __set_current_state() to set_current_state() in
swait_prepare_locked().

Thanks!

-- Steve


\
 
 \ /
  Last update: 2013-12-12 18:41    [W:0.068 / U:0.088 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site