On Wed, Mar 02, 2022, Paolo Bonzini wrote: > On 3/2/22 20:33, Sean Christopherson wrote: > > What about that idea? Put roots invalidated by "fast zap" on_another_ list? > > My very original idea of moving the roots to a separate list didn't work because > > the roots needed to be reachable by the mmu_notifier. But we could just add > > another list_head (inside the unsync_child_bitmap union) and add the roots to > > _that_ list. > > Perhaps the "separate list" idea could be extended to have a single worker > for all kvm_tdp_mmu_put_root() work, and then indeed replace > kvm_tdp_mmu_zap_invalidated_roots() with a flush of _that_ worker. The > disadvantage is a little less parallelism in zapping invalidated roots; but > what is good for kvm_tdp_mmu_zap_invalidated_roots() is just as good for > kvm_tdp_mmu_put_root(), I suppose. If one wants separate work items, KVM > could have its own workqueue, and then you flush that workqueue. > > For now let's do it the simple but ugly way. Keeping > next_invalidated_root() does not make things worse than the status quo, and > further work will be easier to review if it's kept separate from this > already-complex work. Oof, that's not gonna work. My approach here in v3 doesn't work either. I finally remembered why I had the dedicated tdp_mmu_defunct_root flag and thus the smp_mb_*() dance. kvm_tdp_mmu_zap_invalidated_roots() assumes that it was gifted a reference to _all_ invalid roots by kvm_tdp_mmu_invalidate_all_roots(). This works in the current code base only because kvm->slots_lock is held for the entire duration, i.e. roots can't become invalid between the end of kvm_tdp_mmu_invalidate_all_roots() and the end of kvm_tdp_mmu_zap_invalidated_roots(). Marking a root invalid in kvm_tdp_mmu_put_root() breaks that assumption, e.g. if a new root is created and then dropped, it will be marked invalid but the "fast zap" will not have a reference. The "defunct" flag prevents this scenario by allowing the "fast zap" path to identify invalid roots for which it did not take a reference. By virtue of holding a reference, "fast zap" also guarantees that the roots it needs to invalidate and put can't become defunct. My preference would be to either go back to a variant of v2, or to implement my "second list" idea. I also need to figure out why I didn't encounter errors in v3, because I distinctly remember underflowing the refcount before adding the defunct flag...
