Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751395AbdIJJYC (ORCPT ); Sun, 10 Sep 2017 05:24:02 -0400 Received: from mail-wm0-f65.google.com ([74.125.82.65]:37973 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750951AbdIJJYB (ORCPT ); Sun, 10 Sep 2017 05:24:01 -0400 X-Google-Smtp-Source: ADKCNb6+TsReNvuGXo751VMTQbRHyIRUmmLxfpmX3TKMcD9qd59P/nz4Lg2/lpTCLryZNuXixT24sw== Subject: Re: [PATCH 4/6] x86,kvm: Fix apf_task_wake_one() serialization To: Davidlohr Bueso , mingo@redhat.com, peterz@infradead.org Cc: npiggin@gmail.com, paulmck@linux.vnet.ibm.com, linux-kernel@vger.kernel.org, Davidlohr Bueso References: <20170905190022.1474-1-dave@stgolabs.net> <20170905190022.1474-5-dave@stgolabs.net> From: Paolo Bonzini Message-ID: <31ac35bf-ff52-0422-f274-948dc353d9cf@redhat.com> Date: Sun, 10 Sep 2017 11:24:00 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2035 Lines: 60 On 10/09/2017 11:20, Paolo Bonzini wrote: > On 05/09/2017 21:00, Davidlohr Bueso wrote: >> During code inspection, the following potential race was seen: >> >> CPU0 CPU1 >> kvm_async_pf_task_wait apf_task_wake_one >> [S] prepare_to_swait(&n.wq) >> [L] swait_active(&n->wq) >> [S] hlist_del_init(&n->link); >> [L] if (!hlist_unhahed(&n.link)) >> schedule() >> >> Properly serialize swait_active() checks such that a wakeup is >> not missed. >> >> Signed-off-by: Davidlohr Bueso >> --- >> arch/x86/kernel/kvm.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c >> index 874827b0d7ca..aa60a08b65b1 100644 >> --- a/arch/x86/kernel/kvm.c >> +++ b/arch/x86/kernel/kvm.c >> @@ -180,7 +180,7 @@ static void apf_task_wake_one(struct kvm_task_sleep_node *n) >> hlist_del_init(&n->link); >> if (n->halted) >> smp_send_reschedule(n->cpu); >> - else if (swait_active(&n->wq)) >> + else if (swq_has_sleeper(&n->wq)) >> swake_up(&n->wq); >> } > > After Nick's patch, swake_up starts with: > > smp_mb(); > if (!swait_active(q)) > return; > > so we can just remove the test here (and in patch 2). > > The other patches could also use a better swait API, for example: > > 1) add a public __swake_up routine that omits the memory barrier, and > which can be used in patch 3. Perhaps better: omit the out-of-lock > check in __swake_up: then the caller can use it if it knows there is a > waiter. In those cases the memory barrier is expensive. > > 2) change swake_up and __swake_up to return true if they woke up a > process (or alternatively 0/-EAGAIN). Patches 5 and 6 now need not call > anymore either swq_has_sleepers or swait_active, and that saves a memory > barrier too. > > What do you think? ... doh, I missed PeterZ's remark that the early test is gone in tip. Then the series makes total sense. Peter, if you ack patch 1 I can push it through the KVM tree. Paolo