On Thu, May 22, 2014 at 12:56:38PM +0200, Peter Zijlstra wrote:
> On Thu, May 22, 2014 at 11:40:51AM +0100, Mel Gorman wrote:
> > +void __wake_up_page_bit(wait_queue_head_t *wqh, struct page *page, void *word, int bit)
> > +{
> > + struct wait_bit_key key = __WAIT_BIT_KEY_INITIALIZER(word, bit);
> > + unsigned long flags;
> > +
> > + /* If there is no PG_waiters bit, always take the slow path */
>
> That comment is misleading, this is actually a fast path for
> !PG_waiters.
>
And could have been far better anyway now that you called me on it.
> > + if (!__PG_WAITERS && waitqueue_active(wq)) {
> > + __wake_up(wq, TASK_NORMAL, 1, &key);
> > + return;
> > + }
> > +
> > + /*
> > + * Unlike __wake_up_bit it is necessary to check waitqueue_active to be
> > + * checked under the wqh->lock to avoid races with parallel additions
> > + * to the waitqueue. Otherwise races could result in lost wakeups
> > + */
> > + spin_lock_irqsave(&wqh->lock, flags);
> > + if (waitqueue_active(wqh))
> > + __wake_up_common(wqh, TASK_NORMAL, 1, 0, &key);
> > + else
> > + ClearPageWaiters(page);
> > + spin_unlock_irqrestore(&wqh->lock, flags);
> > +}
>
> So I think you missed one Clear opportunity here that was in my original
> proposal, possibly because you also frobbed PG_writeback in.
>
It got lost in the midst of all the other modifications to make this as
"obvious" as possible.
> <SNIP>
>
> I suppose the one thing to say for the big open coded loop is that its
> much easier to read than this scattered stuff.
>
Sure, but the end result of open coding this is duplicated code that will
be harder to maintain overall. I could split __wake_up_bit and use that
in both but I do not think it would make the code any clearer for the sake
of two lines. Untested but this on top?
diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
index 73cb8c6..d3a8c34 100644
--- a/kernel/sched/wait.c
+++ b/kernel/sched/wait.c
@@ -60,19 +60,26 @@ EXPORT_SYMBOL(remove_wait_queue);
* There are circumstances in which we can try to wake a task which has already
* started to run but is not in state TASK_RUNNING. try_to_wake_up() returns
* zero in this (rare) case, and we handle it by continuing to scan the queue.
+ *
+ * Returns true if a process was woken up
*/
-static void __wake_up_common(wait_queue_head_t *q, unsigned int mode,
+static bool __wake_up_common(wait_queue_head_t *q, unsigned int mode,
int nr_exclusive, int wake_flags, void *key)
{
wait_queue_t *curr, *next;
+ bool woke = false;
list_for_each_entry_safe(curr, next, &q->task_list, task_list) {
unsigned flags = curr->flags;
- if (curr->func(curr, mode, wake_flags, key) &&
- (flags & WQ_FLAG_EXCLUSIVE) && !--nr_exclusive)
- break;
+ if (curr->func(curr, mode, wake_flags, key)) {
+ woke = true;
+ if ((flags & WQ_FLAG_EXCLUSIVE) && !--nr_exclusive)
+ break;
+ }
}
+
+ return woke;
}
/**
@@ -441,9 +448,13 @@ void __wake_up_page_bit(wait_queue_head_t *wqh, struct page *page, void *word, i
struct wait_bit_key key = __WAIT_BIT_KEY_INITIALIZER(word, bit);
unsigned long flags;
- /* If there is no PG_waiters bit, always take the slow path */
- if (!__PG_WAITERS && waitqueue_active(wq)) {
- __wake_up(wq, TASK_NORMAL, 1, &key);
+ /*
+ * If there is no PG_waiters bit (32-bit), then waitqueue_active can be
+ * checked without wqh->lock as there is no PG_waiters race to protect.
+ */
+ if (!__PG_WAITERS) {
+ if (waitqueue_active(wqh))
+ __wake_up(wqh, TASK_NORMAL, 1, &key);
return;
}
@@ -453,9 +464,8 @@ void __wake_up_page_bit(wait_queue_head_t *wqh, struct page *page, void *word, i
* to the waitqueue. Otherwise races could result in lost wakeups
*/
spin_lock_irqsave(&wqh->lock, flags);
- if (waitqueue_active(wqh))
- __wake_up_common(wqh, TASK_NORMAL, 1, 0, &key);
- else
+ if (!waitqueue_active(wqh) ||
+ !__wake_up_common(wqh, TASK_NORMAL, 1, 0, &key))
ClearPageWaiters(page);
spin_unlock_irqrestore(&wqh->lock, flags);
}
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>