| Message ID | 20190205205443.1059-1-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | KVM: x86/mmu: Remove fast invalidate mechanism | expand |
On 05/02/19 21:54, Sean Christopherson wrote: > ...and clean up the MMIO spte generation code. > > === Non-x86 Folks === > Patch 01/27 (which you should also be cc'd on) changes the prototype for > a function that is defined by all architectures but only actually used > by x86. Feel free to skip the rest of this cover letter. > > === x86 Folks === > Though not explicitly stated, for all intents and purposes the fast > invalidate mechanism was added to speed up the scenario where removing > a memslot, e.g. when accessing PCI ROM, caused KVM to flush all shadow > entries[1]. > > The other use cases of "flush everything" are VM teardown and handling > MMIO generation overflow, neither of which is a performance critial > path (see "Other Uses" below). > > For the memslot case, zapping all shadow entries is overkill, i.e. KVM > only needs to zap the entries associated with the memslot, but KVM has > historically used a big hammer because if all you have is a hammer, > everything looks like a nail. > > Rather than zap all pages when removing a memslot, zap only the shadow > entries associated with said memslot. I see a performance improvement > of ~5% when zapping only the pages for the deleted memslot when using a > slightly modified version of the original benchmark[2][3][4] (I don't > have easy access to a system with hundreds of gigs of memory). > > $ cat shell.sh > #!/bin/sh > > echo 3 > /proc/sys/vm/drop_caches > ./mmtest -c 8 -m 2000 -e ./rom.sh > > I.e. running 8 threads and 2gb of memory per thread, time in milliseconds: > > Before: 89.117 > After: 84.768 > > > With the memslot use case gone, maintaining the fast invalidate path > adds a moderate amount of complexity but provides little to no value. > Furhtermore, its existence may give the impression that it's "ok" to zap > all shadow pages. Remove the fast invalidate mechanism to simplify the > code and to discourage future code from zapping all pages as using such > a big hammer should be a last resort. > > Unfortunately, simply reverting the fast invalidate code re-introduces > shortcomings in the previous code, notably that KVM may hold a spinlock > and not schedule for an extended period of time. Releasing the lock to > voluntarily reschedule is perfectly doable, and for the VM teardown case > of "zap everything" it can be added with no additional changes since KVM > doesn't need to provide any ordering guarantees about sptes in that case. > > For the MMIO case, KVM needs to prevent vCPUs from consuming stale sptes > created with an old generation number. This should be a non-issue as > KVM is supposed to prevent consuming stale MMIO sptes regardless of the > zapping mechanism employed, releasing mmu_lock while zapping simply > makes it much more likely to hit any bug leading to stale spte usage. > As luck would have it, there are a number of corner cases in the MMIO > spte invalidation flow that could theoretically result in reusing stale > sptes. So before reworking memslot zapping and reverting to the slow > invalidate mechanism, fix a number of bugs related to MMIO generations > and opportunistically clean up the memslots/MMIO generation code. > > > === History === > Flushing of shadow pages when removing a memslot was originally added > by commit 34d4cb8fca1f ("KVM: MMU: nuke shadowed pgtable pages and ptes > on memslot destruction"), and obviously emphasized functionality over > performance. Commit 2df72e9bc4c5 ("KVM: split kvm_arch_flush_shadow") > added a path to allow flushing only the removed slot's shadow pages, > but x86 just redirected to the "zap all" flow. > > Eventually, it became evident that zapping everything is slow, and so > x86 developed a more efficient hammer in the form of the fast invalidate > mechanism. > > === Other Uses === > When a VM is being destroyed, either there are no active vcpus, i.e. > there's no lock contention, or the VM has ungracefully terminated, in > which case we want to reclaim its pages as quickly as possible, i.e. > not release the MMU lock if there are still CPUs executing in the VM. > > The MMIO generation scenario is almost literally a one-in-a-million > occurrence, i.e. is not a performance sensitive scenario. > > It's worth noting that prior to the "fast invalidate" series being > applied, there was actually another use case of kvm_mmu_zap_all() in > emulator_fix_hypercall() whose existence may have contributed to > improving the performance of "zap all" instead of avoiding it altogether. > But that usage was removed by the series itself in commit 758ccc89b83c > ("KVM: x86: drop calling kvm_mmu_zap_all in emulator_fix_hypercall"). > > [1] https://lkml.kernel.org/r/1369960590-14138-1-git-send-email-xiaoguangrong@linux.vnet.ibm.com > [2] https://lkml.kernel.org/r/1368706673-8530-1-git-send-email-xiaoguangrong@linux.vnet.ibm.com > [3] http://lkml.iu.edu/hypermail/linux/kernel/1305.2/00277.html > [4] http://lkml.iu.edu/hypermail/linux/kernel/1305.2/00277/mmtest.tar.bz2 > > v1: https://patchwork.kernel.org/cover/10756579/ > v2: > - Fix a zap vs flush priority bug in kvm_mmu_remote_flush_or_zap() [me] > - Voluntarily reschedule and/or drop mmu_lock as needed when zapping > all sptes or all MMIO sptes [Paolo] > - Add patches to clean up the memslots/MMIO generation code and fix > a variety of theoretically corner case bugs > > > Sean Christopherson (27): > KVM: Call kvm_arch_memslots_updated() before updating memslots > KVM: x86/mmu: Detect MMIO generation wrap in any address space > KVM: x86/mmu: Do not cache MMIO accesses while memslots are in flux > KVM: Explicitly define the "memslot update in-progress" bit > KVM: x86: Use a u64 when passing the MMIO gen around > KVM: x86: Refactor the MMIO SPTE generation handling > KVM: Remove the hack to trigger memslot generation wraparound > KVM: Move the memslot update in-progress flag to bit 63 > KVM: x86/mmu: Move slot_level_*() helper functions up a few lines > KVM: x86/mmu: Split remote_flush+zap case out of > kvm_mmu_flush_or_zap() > KVM: x86/mmu: Zap only the relevant pages when removing a memslot > Revert "KVM: MMU: document fast invalidate all pages" > Revert "KVM: MMU: drop kvm_mmu_zap_mmio_sptes" > KVM: x86/mmu: Voluntarily reschedule as needed when zapping MMIO sptes > KVM: x86/mmu: Remove is_obsolete() call > Revert "KVM: MMU: reclaim the zapped-obsolete page first" > Revert "KVM: MMU: collapse TLB flushes when zap all pages" > Revert "KVM: MMU: zap pages in batch" > Revert "KVM: MMU: add tracepoint for kvm_mmu_invalidate_all_pages" > Revert "KVM: MMU: show mmu_valid_gen in shadow page related > tracepoints" > Revert "KVM: x86: use the fast way to invalidate all pages" > KVM: x86/mmu: skip over invalid root pages when zapping all sptes > KVM: x86/mmu: Voluntarily reschedule as needed when zapping all sptes > Revert "KVM: MMU: fast invalidate all pages" > KVM: x86/mmu: Differentiate between nr zapped and list unstable > KVM: x86/mmu: WARN if zapping a MMIO spte results in zapping children > KVM: x86/mmu: Consolidate kvm_mmu_zap_all() and > kvm_mmu_zap_mmio_sptes() > > Documentation/virtual/kvm/mmu.txt | 41 +-- > arch/mips/include/asm/kvm_host.h | 2 +- > arch/powerpc/include/asm/kvm_host.h | 2 +- > arch/s390/include/asm/kvm_host.h | 2 +- > arch/x86/include/asm/kvm_host.h | 9 +- > arch/x86/kvm/mmu.c | 442 +++++++++++++--------------- > arch/x86/kvm/mmu.h | 1 - > arch/x86/kvm/mmutrace.h | 42 +-- > arch/x86/kvm/x86.c | 7 +- > arch/x86/kvm/x86.h | 7 +- > include/linux/kvm_host.h | 23 +- > virt/kvm/arm/mmu.c | 2 +- > virt/kvm/kvm_main.c | 39 ++- > 13 files changed, 286 insertions(+), 333 deletions(-) > Queued, thanks. Paolo
...and clean up the MMIO spte generation code. === Non-x86 Folks === Patch 01/27 (which you should also be cc'd on) changes the prototype for a function that is defined by all architectures but only actually used by x86. Feel free to skip the rest of this cover letter. === x86 Folks === Though not explicitly stated, for all intents and purposes the fast invalidate mechanism was added to speed up the scenario where removing a memslot, e.g. when accessing PCI ROM, caused KVM to flush all shadow entries[1]. The other use cases of "flush everything" are VM teardown and handling MMIO generation overflow, neither of which is a performance critial path (see "Other Uses" below). For the memslot case, zapping all shadow entries is overkill, i.e. KVM only needs to zap the entries associated with the memslot, but KVM has historically used a big hammer because if all you have is a hammer, everything looks like a nail. Rather than zap all pages when removing a memslot, zap only the shadow entries associated with said memslot. I see a performance improvement of ~5% when zapping only the pages for the deleted memslot when using a slightly modified version of the original benchmark[2][3][4] (I don't have easy access to a system with hundreds of gigs of memory). $ cat shell.sh #!/bin/sh echo 3 > /proc/sys/vm/drop_caches ./mmtest -c 8 -m 2000 -e ./rom.sh I.e. running 8 threads and 2gb of memory per thread, time in milliseconds: Before: 89.117 After: 84.768 With the memslot use case gone, maintaining the fast invalidate path adds a moderate amount of complexity but provides little to no value. Furhtermore, its existence may give the impression that it's "ok" to zap all shadow pages. Remove the fast invalidate mechanism to simplify the code and to discourage future code from zapping all pages as using such a big hammer should be a last resort. Unfortunately, simply reverting the fast invalidate code re-introduces shortcomings in the previous code, notably that KVM may hold a spinlock and not schedule for an extended period of time. Releasing the lock to voluntarily reschedule is perfectly doable, and for the VM teardown case of "zap everything" it can be added with no additional changes since KVM doesn't need to provide any ordering guarantees about sptes in that case. For the MMIO case, KVM needs to prevent vCPUs from consuming stale sptes created with an old generation number. This should be a non-issue as KVM is supposed to prevent consuming stale MMIO sptes regardless of the zapping mechanism employed, releasing mmu_lock while zapping simply makes it much more likely to hit any bug leading to stale spte usage. As luck would have it, there are a number of corner cases in the MMIO spte invalidation flow that could theoretically result in reusing stale sptes. So before reworking memslot zapping and reverting to the slow invalidate mechanism, fix a number of bugs related to MMIO generations and opportunistically clean up the memslots/MMIO generation code. === History === Flushing of shadow pages when removing a memslot was originally added by commit 34d4cb8fca1f ("KVM: MMU: nuke shadowed pgtable pages and ptes on memslot destruction"), and obviously emphasized functionality over performance. Commit 2df72e9bc4c5 ("KVM: split kvm_arch_flush_shadow") added a path to allow flushing only the removed slot's shadow pages, but x86 just redirected to the "zap all" flow. Eventually, it became evident that zapping everything is slow, and so x86 developed a more efficient hammer in the form of the fast invalidate mechanism. === Other Uses === When a VM is being destroyed, either there are no active vcpus, i.e. there's no lock contention, or the VM has ungracefully terminated, in which case we want to reclaim its pages as quickly as possible, i.e. not release the MMU lock if there are still CPUs executing in the VM. The MMIO generation scenario is almost literally a one-in-a-million occurrence, i.e. is not a performance sensitive scenario. It's worth noting that prior to the "fast invalidate" series being applied, there was actually another use case of kvm_mmu_zap_all() in emulator_fix_hypercall() whose existence may have contributed to improving the performance of "zap all" instead of avoiding it altogether. But that usage was removed by the series itself in commit 758ccc89b83c ("KVM: x86: drop calling kvm_mmu_zap_all in emulator_fix_hypercall"). [1] https://lkml.kernel.org/r/1369960590-14138-1-git-send-email-xiaoguangrong@linux.vnet.ibm.com [2] https://lkml.kernel.org/r/1368706673-8530-1-git-send-email-xiaoguangrong@linux.vnet.ibm.com [3] http://lkml.iu.edu/hypermail/linux/kernel/1305.2/00277.html [4] http://lkml.iu.edu/hypermail/linux/kernel/1305.2/00277/mmtest.tar.bz2 v1: https://patchwork.kernel.org/cover/10756579/ v2: - Fix a zap vs flush priority bug in kvm_mmu_remote_flush_or_zap() [me] - Voluntarily reschedule and/or drop mmu_lock as needed when zapping all sptes or all MMIO sptes [Paolo] - Add patches to clean up the memslots/MMIO generation code and fix a variety of theoretically corner case bugs Sean Christopherson (27): KVM: Call kvm_arch_memslots_updated() before updating memslots KVM: x86/mmu: Detect MMIO generation wrap in any address space KVM: x86/mmu: Do not cache MMIO accesses while memslots are in flux KVM: Explicitly define the "memslot update in-progress" bit KVM: x86: Use a u64 when passing the MMIO gen around KVM: x86: Refactor the MMIO SPTE generation handling KVM: Remove the hack to trigger memslot generation wraparound KVM: Move the memslot update in-progress flag to bit 63 KVM: x86/mmu: Move slot_level_*() helper functions up a few lines KVM: x86/mmu: Split remote_flush+zap case out of kvm_mmu_flush_or_zap() KVM: x86/mmu: Zap only the relevant pages when removing a memslot Revert "KVM: MMU: document fast invalidate all pages" Revert "KVM: MMU: drop kvm_mmu_zap_mmio_sptes" KVM: x86/mmu: Voluntarily reschedule as needed when zapping MMIO sptes KVM: x86/mmu: Remove is_obsolete() call Revert "KVM: MMU: reclaim the zapped-obsolete page first" Revert "KVM: MMU: collapse TLB flushes when zap all pages" Revert "KVM: MMU: zap pages in batch" Revert "KVM: MMU: add tracepoint for kvm_mmu_invalidate_all_pages" Revert "KVM: MMU: show mmu_valid_gen in shadow page related tracepoints" Revert "KVM: x86: use the fast way to invalidate all pages" KVM: x86/mmu: skip over invalid root pages when zapping all sptes KVM: x86/mmu: Voluntarily reschedule as needed when zapping all sptes Revert "KVM: MMU: fast invalidate all pages" KVM: x86/mmu: Differentiate between nr zapped and list unstable KVM: x86/mmu: WARN if zapping a MMIO spte results in zapping children KVM: x86/mmu: Consolidate kvm_mmu_zap_all() and kvm_mmu_zap_mmio_sptes() Documentation/virtual/kvm/mmu.txt | 41 +-- arch/mips/include/asm/kvm_host.h | 2 +- arch/powerpc/include/asm/kvm_host.h | 2 +- arch/s390/include/asm/kvm_host.h | 2 +- arch/x86/include/asm/kvm_host.h | 9 +- arch/x86/kvm/mmu.c | 442 +++++++++++++--------------- arch/x86/kvm/mmu.h | 1 - arch/x86/kvm/mmutrace.h | 42 +-- arch/x86/kvm/x86.c | 7 +- arch/x86/kvm/x86.h | 7 +- include/linux/kvm_host.h | 23 +- virt/kvm/arm/mmu.c | 2 +- virt/kvm/kvm_main.c | 39 ++- 13 files changed, 286 insertions(+), 333 deletions(-)