Messages in this thread |  | | Date | Thu, 12 Dec 2013 12:10:15 -0500 | From | Steven Rostedt <> | Subject | Re: [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
|  |