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 <dbu...@suse.de> >> --- >> 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
