lkml.org 
[lkml]   [2015]   [Oct]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH v1 3/8] sched/completion: convert completions to use simple wait queues
From
Date
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;
}


\
 
 \ /
  Last update: 2015-10-12 12:21    [W:0.137 / U:0.076 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site