From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751755AbbJLKDN (ORCPT ); Mon, 12 Oct 2015 06:03:13 -0400 Received: from mail.bmw-carit.de ([62.245.222.98]:47302 "EHLO mail.bmw-carit.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751337AbbJLKDM (ORCPT ); Mon, 12 Oct 2015 06:03:12 -0400 X-CTCH-RefID: str=0001.0A0C0204.561B855B.00D9,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0 Subject: Re: [PATCH v1 3/8] sched/completion: convert completions to use simple wait queues To: Peter Zijlstra References: <1441800334-16609-1-git-send-email-daniel.wagner@bmw-carit.de> <1441800334-16609-4-git-send-email-daniel.wagner@bmw-carit.de> <20150909142608.GP12596@twins.programming.kicks-ass.net> <561B7A9A.3020904@bmw-carit.de> CC: From: Daniel Wagner Message-ID: <561B855A.2060403@bmw-carit.de> Date: Mon, 12 Oct 2015 12:03:06 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <561B7A9A.3020904@bmw-carit.de> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 #include +#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; }