Messages in this thread Patch in this message | | | Subject | Re: [PATCH v1 3/8] sched/completion: convert completions to use simple wait queues | From | Daniel Wagner <> | Date | Mon, 12 Oct 2015 12:03:06 +0200 |
| |
On 10/12/2015 11:17 AM, Daniel Wagner wrote: > On 09/09/2015 04:26 PM, Peter Zijlstra wrote: >> On Wed, Sep 09, 2015 at 02:05:29PM +0200, Daniel Wagner wrote: >>> @@ -50,10 +50,10 @@ void complete_all(struct completion *x) >>> { >>> unsigned long flags; >>> >>> - spin_lock_irqsave(&x->wait.lock, flags); >>> + raw_spin_lock_irqsave(&x->wait.lock, flags); >>> x->done += UINT_MAX/2; >>> - __wake_up_locked(&x->wait, TASK_NORMAL, 0); >>> - spin_unlock_irqrestore(&x->wait.lock, flags); >>> + swake_up_locked(&x->wait); >>> + raw_spin_unlock_irqrestore(&x->wait.lock, flags); >>> } >>> EXPORT_SYMBOL(complete_all); >> >> I don't think that's correct; __wake_up_locked(.nr=0) would wake all >> waiters, where swake_up_locked() will only wake one. > > I read that x->done should be protected via wait.lock during the whole > operation. swake_up_all() will release and reacquire the lock while > processing the all waiters. So we need to get > > Could we play a trick like setting the highest bit in done for > indicating the complete_all() operation. The UINT_MAX/2 update looks > like do this by setting a value which has the biggest offset from 0 (but > why adding instead of just going for assigning...).
I had something like this here in mind:
diff --git a/kernel/sched/completion.c b/kernel/sched/completion.c index b020159..c03d4de 100644 --- a/kernel/sched/completion.c +++ b/kernel/sched/completion.c @@ -14,6 +14,9 @@ #include <linux/sched.h> #include <linux/completion.h>
+#define COMPLETION_DONE_ALL (1UL << 31) +#define COMPLETION_DONE_MASK (COMPLETION_DONE_ALL - 1) + /** * complete: - signals a single thread waiting on this completion * @x: holds the state of this particular completion @@ -31,7 +34,7 @@ void complete(struct completion *x) unsigned long flags;
raw_spin_lock_irqsave(&x->wait.lock, flags); - x->done++; + x->done = (x->done + 1) & COMPLETION_DONE_MASK; swake_up_locked(&x->wait); raw_spin_unlock_irqrestore(&x->wait.lock, flags); } @@ -51,9 +54,9 @@ void complete_all(struct completion *x) unsigned long flags;
raw_spin_lock_irqsave(&x->wait.lock, flags); - x->done += UINT_MAX/2; - swake_up_locked(&x->wait); + x->done |= COMPLETION_DONE_ALL; raw_spin_unlock_irqrestore(&x->wait.lock, flags); + swake_up_all(&x->wait); } EXPORT_SYMBOL(complete_all);
@@ -79,7 +82,7 @@ do_wait_for_common(struct completion *x, if (!x->done) return timeout; } - x->done--; + x->done = (x->done - 1) & COMPLETION_DONE_MASK; return timeout ?: 1; }
@@ -281,7 +284,7 @@ bool try_wait_for_completion(struct completion *x) if (!x->done) ret = 0; else - x->done--; + x->done = (x->done - 1) & COMPLETION_DONE_MASK; raw_spin_unlock_irqrestore(&x->wait.lock, flags); return ret; }
| |