Re: [PATCH v1 3/8] sched/completion: convert completions to use simple wait queues

[Daniel Wagner]

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;
 }

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/