Messages in this thread | | | Subject | Re: [PATCH 4/6] x86,kvm: Fix apf_task_wake_one() serialization | From | Paolo Bonzini <> | Date | Sun, 10 Sep 2017 11:20:40 +0200 |
| |
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 <dbueso@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?
Thanks,
Paolo
| |