lkml.org 
[lkml]   [2022]   [Mar]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v4 21/30] KVM: x86/mmu: Zap invalidated roots via asynchronous worker
From
On 3/3/22 22:32, Sean Christopherson wrote:

> The re-queue scenario happens if a root is queued and zapped, but is kept alive
> by a vCPU that hasn't yet put its reference. If another memslot comes along before
> the (sleeping) vCPU drops its reference, this will re-queue the root.
>
> It's not a major problem in this patch as it's a small amount of wasted effort,
> but it will be an issue when the "put" path starts using the queue, as that will
> create a scenario where a memslot update (or NX toggle) can come along while a
> defunct root is in the zap queue.

As of this patch it's not a problem because
kvm_tdp_mmu_invalidate_all_roots()'s caller holds kvm->slots_lock, so
kvm_tdp_mmu_invalidate_all_roots() is guarantee to queue its work items
on an empty workqueue. In effect the workqueue is just a fancy list.
But as you point out in the review to patch 24, it becomes a problem
when there's no kvm->slots_lock to guarantee that. Then it needs to
check that the root isn't already invalid.

>>> The only issue is that kvm_tdp_mmu_invalidate_all_roots() now assumes that
>>> there is at least one reference in kvm->users_count; so if the VM is dying
>>> just go through the slow path, as there is nothing to gain by using the fast
>>> zapping.
>> This isn't actually implemented.:-)
> Oh, and when you implement it (or copy paste), can you also add a lockdep and a
> comment about the check being racy, but that the race is benign? It took me a
> second to realize why it's safe to use a work queue without holding a reference
> to @kvm.

I didn't remove the paragraph from the commit message, but I think it's
unnecessary now. The workqueue is flushed in kvm_mmu_zap_all_fast() and
kvm_mmu_uninit_tdp_mmu(), unlike the buggy patch, so it doesn't need to
take a reference to the VM.

I think I don't even need to check kvm->users_count in the defunct root
case, as long as kvm_mmu_uninit_tdp_mmu() flushes and destroys the
workqueue before it checks that the lists are empty.

I'll wait to hear from you later today before sending out v5.

Paolo

\
 
 \ /
  Last update: 2022-03-04 07:49    [W:0.121 / U:0.168 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site