kvm: Use a new spinlock to avoid atomic operations in kvm_get_dirty_log_protect
RFC PATCH 1/1 kvm: Use a new spinlock to avoid atomic operations in kvm_get_dirty_log_protect
<The objective is to get feedback on the idea, before trying to get a
proper version. Please provide feedback! >
kvm_get_dirty_log_protect is very slow, and it takes a while to get the dirty bitmap when 'manual_dirty_log_protect' is not being used. Current working is something like:
- Memset the 'second_dirty_bitmap' with zeros
- Get KVM_MMU_LOCK to scan the whole bitmap for dirty-bits
- If there is long with a dirty-bit, xchg() it into second_dirty_bitmap
- Reprotect the related memory pages,
- Flush remote tlbs,
- Copy the dirty-bitmap to the user, and return.
The proposal is to remove the memset and the atomics part by using the second_dirty_bitmap as a real dirty-bitmap, instead of a simple buffer:
- When kvm_get_dirty_log_protect runs, before copying anything, switches to the other dirty-bitmap (primary->secondary or secondary->primary).
- (After that, mark_page_dirty_in_slot() will start writing dirty-bits to the other bitmap, so there will be no one using it, and it's fine to modify it without atomics),
- Then, copy the current bitmap to userspace,
- Then get the KVM_MMU_LOCK, scan the bitmap copied, and both protect the page and clean the dirty bits in the same loop.
- Flush remote tlbs, and return.
The bitmap switch (primary <-> secondary) is protected by a spinlock, which is also added into mark_page_dirty_in_slot() to make sure it does not write to the wrong bitmap. Since this spinlock protects just a few instructios, it's should not introduce a lot of delay.
Results:
1 - Average clock count spent in kvm_get_dirty_log_protect() goes from
13608798 to 6734244, representing around 50% less clock cycles.
2 - Average clock count spent in mark_page_dirty_in_slot() goes from
462 to 471, representing around 2% increase, but since this means just a few cycles, it can be an imprecise measure.
Known limitations:
0 - It's preliminary. It has been tested but there are a lot of bad stuff that needs to be fixed, if the idea is interesting.
1 - Spin-Locking in mark_page_dirty_in_slot():
I understand this function happens a lot in the guest and should probably be as fast as possible, so introducing a lock here seems counter-productive, but to be fair, I could not see it any slower than a couple cycles in my current setup (x86_64 machine).
2 - Qemu will use the 'manual_dirty_log_protect'
I understand that more recent versions qemu will use 'manual_dirty_log_protect' when available, so this approach will not benefit this use case, which is quite common.
A counter argument would be: there are other hypervisors that could benefit from it, and that is also applicable for older qemu versions.
3 - On top of that, the overhead in (1) will be unnecessary for (2) case That could be removed by testing for 'manual_dirty_log_protect' in 'mark_page_dirty_in_slot()', and skipping the spinlock, but this was not added just to keep the rfc simpler.
Performance tests were doing using:
- x86_64 VM with 32 vcpus and 16GB RAM
- Systemtap probes in function enter and function return, measuring the number of clock cycles spent between those two, and printing the average.
- Synthetic VM load, that keeps dirtying memory at fixed rate of 8Gbps.
- VM Migration between hosts.
Further info:
- I am also trying to think on improvements for the 'manual_dirty_log_protect' use case, which seems to be very hard to improve. For starters, using the same approach to remove the atomics does not seem to cause any relevant speedup.
--- include/linux/kvm_host.h | 3 ++ virt/kvm/kvm_main.c | 75 ++++++++++++++++++++++++++-------------- 2 files changed, 52 insertions(+), 26 deletions(-)
include/linux/kvm_host.h +3 -0
@@ -571,6 +571,9 @@ struct kvm_memory_slot {gfn_t base_gfn;unsigned long npages;unsigned long *dirty_bitmap; +/* Protects use_secondary, which tells which bitmap to use */ +spinlock_t bm_switch_lock; +bool use_secondary;struct kvm_arch_memory_slot arch;unsigned long userspace_addr;u32 flags;
virt/kvm/kvm_main.c +49 -26
120 lines not on this page · raw message
Signature · 1 line
2.37.1
Paolo Bonzini
maintainerQuoted 5 lines · Leonardo Bras
1 - Spin-Locking in
mark_page_dirty_in_slot():
I understand this function happens a lot in the guest and should probably be as fast as possible, so introducing a lock here seems counter-productive, but to be fair, I could not see it any slower than a couple cycles in my current setup (x86_64machine).
Maybe too small workload? 32 vcpus at 8 GB/s would mean 256 MB/s/vCPU, i.e. 16384 pages/second *at most*. That might not create too much contention.
One possibility here is to use a global (for all VMs) percpu_rw_semaphore, or perhaps even RCU. The write critical section is so short that it could be a win nevertheless.
However...
Quoted 6 lines
2 - Qemu will use the '
manual_dirty_log_protect'
I understand that more recent versions qemu will use 'manual_dirty_log_protect' when available, so this approach will not benefit this use case, which is quite common.
A counter argument would be: there are other hypervisors that could benefit from it, and that is also applicable for older qemu versions.
... that was my first thought indeed. I would just consider the old API to be legacy and not bother with it. Mostly because of the ability to clear a small part of the bitmap(*), and the initially-all-set optimization, manual dirty log ought to be superior even if CLEAR_DIRTY_LOG has to use atomics.
Quoted 4 lines
- I am also trying to think on improvements for the '
manual_dirty_log_protect' use case, which seems to be very hard to improve. For starters, using the same approach to remove the atomics does not seem to cause any relevant speedup.
Yes, there are two issues:
1) CLEAR_DIRTY_LOG does not clear all bits, only those passed in by userspace. This means that the inactive bitmap still has some bits set
2) the ability to clear part of the bitmap makes it hard to do a wholesale switch in CLEAR_DIRTY_LOG; this is the dealbreaker
Thanks,
Paolo
(*) for the old API, that requires a workaround of using multiple small memslots, e.g. 1-32G in size
kernel test robot
build bot[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in…
Hi Leonardo,
[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on kvm/queue]
[also build test ERROR on linus/master v5.19 next-20220805] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Leonardo-Bras/kvm-Use-a-new-spinlock-to-avoid-atomic-operations-in-kvm_get_dirty_log_protect/20220805-153809 base: https://git.kernel.org/pub/scm/virt/kvm/kvm.git queue config: x86_64-randconfig-a014 (https://download.01.org/0day-ci/archive/20220806/[email protected]/config) compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 26dd42705c2af0b8f6e5d6cdb32c9bd5ed9524eb)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/6d5c7b373425e7b50ae4f41f21472ae0ae2a031d
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Leonardo-Bras/kvm-Use-a-new-spinlock-to-avoid-atomic-operations-in-kvm_get_dirty_log_protect/20220805-153809 git checkout 6d5c7b373425e7b50ae4f41f21472ae0ae2a031d
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
All errors (new ones prefixed by >>):
>> arch/x86/kvm/../../../virt/kvm/kvm_main.c:3319:14: error: passing 'const spinlock_t *' (aka 'const struct spinlock *') to parameter of type 'spinlock_t *' (aka 'struct spinlock *') discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
spin_lock(&memslot->bm_switch_lock);
^~~~~~~~~~~~~~~~~~~~~~~~
include/linux/spinlock.h:347:51: note: passing argument to parameter 'lock' here
static __always_inline void spin_lock(spinlock_t *lock)
^
>> arch/x86/kvm/../../../virt/kvm/kvm_main.c:3322:33: error: passing 'const struct kvm_memory_slot *' to parameter of type 'struct kvm_memory_slot *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
kvm_second_dirty_bitmap(memslot));
^~~~~~~
include/linux/kvm_host.h:594:78: note: passing argument to parameter 'memslot' here
static inline unsigned long *kvm_second_dirty_bitmap(struct kvm_memory_slot *memslot)
^
arch/x86/kvm/../../../virt/kvm/kvm_main.c:3326:16: error: passing 'const spinlock_t *' (aka 'const struct spinlock *') to parameter of type 'spinlock_t *' (aka 'struct spinlock *') discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
spin_unlock(&memslot->bm_switch_lock);
^~~~~~~~~~~~~~~~~~~~~~~~
include/linux/spinlock.h:387:53: note: passing argument to parameter 'lock' here
static __always_inline void spin_unlock(spinlock_t *lock)
^3 errors generated.
vim +3319 arch/x86/kvm/../../../virt/kvm/kvm_main.c
3310
3311 if (memslot && kvm_slot_dirty_track_enabled(memslot)) {
3312 unsigned long rel_gfn = gfn - memslot->base_gfn;
3313 u32 slot = (memslot->as_id << 16) | memslot->id;
3314
3315 if (kvm->dirty_ring_size) {
3316 kvm_dirty_ring_push(&vcpu->dirty_ring,
3317 slot, rel_gfn);
3318 } else {
> 3319 spin_lock(&memslot->bm_switch_lock); 3320 if (memslot->use_secondary) 3321 set_bit_le(rel_gfn, > 3322 kvm_second_dirty_bitmap(memslot));
3323 else
3324 set_bit_le(rel_gfn, memslot->dirty_bitmap);
3325
3326 spin_unlock(&memslot->bm_switch_lock);
3327 }
3328 }
3329 }
3330 EXPORT_SYMBOL_GPL(mark_page_dirty_in_slot);
3331
Signature · 2 lines
0-DAY CI Kernel Test Service https://01.org/lkp
kernel test robot
build bot[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in…
Hi Leonardo,
[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on kvm/queue]
[also build test ERROR on linus/master v5.19 next-20220805] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Leonardo-Bras/kvm-Use-a-new-spinlock-to-avoid-atomic-operations-in-kvm_get_dirty_log_protect/20220805-153809 base: https://git.kernel.org/pub/scm/virt/kvm/kvm.git queue config: s390-randconfig-r032-20220805 (https://download.01.org/0day-ci/archive/20220806/[email protected]/config) compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 26dd42705c2af0b8f6e5d6cdb32c9bd5ed9524eb)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install s390 cross compiling tool for clang build
# apt-get install binutils-s390x-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/6d5c7b373425e7b50ae4f41f21472ae0ae2a031d
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Leonardo-Bras/kvm-Use-a-new-spinlock-to-avoid-atomic-operations-in-kvm_get_dirty_log_protect/20220805-153809 git checkout 6d5c7b373425e7b50ae4f41f21472ae0ae2a031d
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
All errors (new ones prefixed by >>):
In file included from arch/s390/kvm/../../../virt/kvm/kvm_main.c:18:
In file included from include/linux/kvm_host.h:41:
In file included from include/linux/kvm_para.h:5:
In file included from include/uapi/linux/kvm_para.h:37:
In file included from arch/s390/include/asm/kvm_para.h:25:
In file included from arch/s390/include/asm/diag.h:12:
In file included from include/linux/if_ether.h:19:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
^
include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
#define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
^
In file included from arch/s390/kvm/../../../virt/kvm/kvm_main.c:18:
In file included from include/linux/kvm_host.h:41:
In file included from include/linux/kvm_para.h:5:
In file included from include/uapi/linux/kvm_para.h:37:
In file included from arch/s390/include/asm/kvm_para.h:25:
In file included from arch/s390/include/asm/diag.h:12:
In file included from include/linux/if_ether.h:19:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
^
include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
#define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
^
In file included from arch/s390/kvm/../../../virt/kvm/kvm_main.c:18:
In file included from include/linux/kvm_host.h:41:
In file included from include/linux/kvm_para.h:5:
In file included from include/uapi/linux/kvm_para.h:37:
In file included from arch/s390/include/asm/kvm_para.h:25:
In file included from arch/s390/include/asm/diag.h:12:
In file included from include/linux/if_ether.h:19:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
>> arch/s390/kvm/../../../virt/kvm/kvm_main.c:3319:14: error: passing 'const spinlock_t *' (aka 'const struct spinlock *') to parameter of type 'spinlock_t *' (aka 'struct spinlock *') discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
spin_lock(&memslot->bm_switch_lock);
^~~~~~~~~~~~~~~~~~~~~~~~
include/linux/spinlock.h:347:51: note: passing argument to parameter 'lock' here
static __always_inline void spin_lock(spinlock_t *lock)
^
>> arch/s390/kvm/../../../virt/kvm/kvm_main.c:3322:33: error: passing 'const struct kvm_memory_slot *' to parameter of type 'struct kvm_memory_slot *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
kvm_second_dirty_bitmap(memslot));
^~~~~~~
include/linux/kvm_host.h:594:78: note: passing argument to parameter 'memslot' here
static inline unsigned long *kvm_second_dirty_bitmap(struct kvm_memory_slot *memslot)
^
arch/s390/kvm/../../../virt/kvm/kvm_main.c:3326:16: error: passing 'const spinlock_t *' (aka 'const struct spinlock *') to parameter of type 'spinlock_t *' (aka 'struct spinlock *') discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
spin_unlock(&memslot->bm_switch_lock);
^~~~~~~~~~~~~~~~~~~~~~~~
include/linux/spinlock.h:387:53: note: passing argument to parameter 'lock' here
static __always_inline void spin_unlock(spinlock_t *lock)
^12 warnings and 3 errors generated.
vim +3319 arch/s390/kvm/../../../virt/kvm/kvm_main.c
3310
3311 if (memslot && kvm_slot_dirty_track_enabled(memslot)) {
3312 unsigned long rel_gfn = gfn - memslot->base_gfn;
3313 u32 slot = (memslot->as_id << 16) | memslot->id;
3314
3315 if (kvm->dirty_ring_size) {
3316 kvm_dirty_ring_push(&vcpu->dirty_ring,
3317 slot, rel_gfn);
3318 } else {
> 3319 spin_lock(&memslot->bm_switch_lock); 3320 if (memslot->use_secondary) 3321 set_bit_le(rel_gfn, > 3322 kvm_second_dirty_bitmap(memslot));
3323 else
3324 set_bit_le(rel_gfn, memslot->dirty_bitmap);
3325
3326 spin_unlock(&memslot->bm_switch_lock);
3327 }
3328 }
3329 }
3330 EXPORT_SYMBOL_GPL(mark_page_dirty_in_slot);
3331
Signature · 2 lines
0-DAY CI Kernel Test Service https://01.org/lkp