This series enables the ring-based dirty memory tracking for ARM64. The feature has been available and enabled on x86 for a while. It is beneficial when the number of dirty pages is small in a checkpointing system or live migration scenario. More details can be found from fb04a1eddb1a ("KVM: X86: Implement ring-based dirty memory tracking").
The generic part has been comprehensive, meaning there isn't too much work, needed to extend it to ARM64.
- PATCH[1] introduces KVM_REQ_RING_SOFT_FULL for x86 - PATCH[2] enables the feature on ARM64 - PATCH[3-5] improves kvm/selftests/dirty_log_test
v1: https://lore.kernel.org/lkml/20220819005601.198436-1-gshan@redhat.com
Testing =======
(1) kvm/selftests/dirty_log_test (2) Live migration by QEMU
Changelog ========= v2: * Introduce KVM_REQ_RING_SOFT_FULL (Marc) * Changelog improvement (Marc) * Fix dirty_log_test without knowing host page size (Drew)
Gavin Shan (5): KVM: x86: Introduce KVM_REQ_RING_SOFT_FULL KVM: arm64: Enable ring-based dirty memory tracking KVM: selftests: Use host page size to map ring buffer in dirty_log_test KVM: selftests: Clear dirty ring states between two modes in dirty_log_test KVM: selftests: Automate choosing dirty ring size in dirty_log_test
Documentation/virt/kvm/api.rst | 2 +- arch/arm64/include/uapi/asm/kvm.h | 1 + arch/arm64/kvm/Kconfig | 1 + arch/arm64/kvm/arm.c | 8 +++ arch/x86/kvm/x86.c | 5 +- include/linux/kvm_host.h | 1 + tools/testing/selftests/kvm/dirty_log_test.c | 53 ++++++++++++++------ tools/testing/selftests/kvm/lib/kvm_util.c | 2 +- virt/kvm/dirty_ring.c | 4 ++ 9 files changed, 59 insertions(+), 18 deletions(-)
This adds KVM_REQ_RING_SOFT_FULL, which is raised when the dirty ring of the specific VCPU becomes softly full in kvm_dirty_ring_push(). The VCPU is enforced to exit when the request is raised and its dirty ring is softly full on its entrance.
Suggested-by: Marc Zyngier maz@kernel.org Signed-off-by: Gavin Shan gshan@redhat.com --- arch/x86/kvm/x86.c | 5 +++-- include/linux/kvm_host.h | 1 + virt/kvm/dirty_ring.c | 4 ++++ 3 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 43a6a7efc6ec..7f368f59f033 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -10265,8 +10265,9 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) bool req_immediate_exit = false;
/* Forbid vmenter if vcpu dirty ring is soft-full */ - if (unlikely(vcpu->kvm->dirty_ring_size && - kvm_dirty_ring_soft_full(&vcpu->dirty_ring))) { + if (kvm_check_request(KVM_REQ_RING_SOFT_FULL, vcpu) && + kvm_dirty_ring_soft_full(&vcpu->dirty_ring)) { + kvm_make_request(KVM_REQ_RING_SOFT_FULL, vcpu); vcpu->run->exit_reason = KVM_EXIT_DIRTY_RING_FULL; trace_kvm_dirty_ring_exit(vcpu); r = 0; diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index f4519d3689e1..53fa3134fee0 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -157,6 +157,7 @@ static inline bool is_error_page(struct page *page) #define KVM_REQ_VM_DEAD (1 | KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP) #define KVM_REQ_UNBLOCK 2 #define KVM_REQ_UNHALT 3 +#define KVM_REQ_RING_SOFT_FULL 4 #define KVM_REQUEST_ARCH_BASE 8
/* diff --git a/virt/kvm/dirty_ring.c b/virt/kvm/dirty_ring.c index f4c2a6eb1666..f0e49937bc9e 100644 --- a/virt/kvm/dirty_ring.c +++ b/virt/kvm/dirty_ring.c @@ -149,6 +149,7 @@ int kvm_dirty_ring_reset(struct kvm *kvm, struct kvm_dirty_ring *ring)
void kvm_dirty_ring_push(struct kvm_dirty_ring *ring, u32 slot, u64 offset) { + struct kvm_vcpu *vcpu = container_of(ring, struct kvm_vcpu, dirty_ring); struct kvm_dirty_gfn *entry;
/* It should never get full */ @@ -166,6 +167,9 @@ void kvm_dirty_ring_push(struct kvm_dirty_ring *ring, u32 slot, u64 offset) kvm_dirty_gfn_set_dirtied(entry); ring->dirty_index++; trace_kvm_dirty_ring_push(ring, slot, offset); + + if (kvm_dirty_ring_soft_full(ring)) + kvm_make_request(KVM_REQ_RING_SOFT_FULL, vcpu); }
struct page *kvm_dirty_ring_get_page(struct kvm_dirty_ring *ring, u32 offset)
On Fri, Sep 16, 2022 at 12:51:31PM +0800, Gavin Shan wrote:
This adds KVM_REQ_RING_SOFT_FULL, which is raised when the dirty ring of the specific VCPU becomes softly full in kvm_dirty_ring_push(). The VCPU is enforced to exit when the request is raised and its dirty ring is softly full on its entrance.
Suggested-by: Marc Zyngier maz@kernel.org Signed-off-by: Gavin Shan gshan@redhat.com
arch/x86/kvm/x86.c | 5 +++-- include/linux/kvm_host.h | 1 + virt/kvm/dirty_ring.c | 4 ++++ 3 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 43a6a7efc6ec..7f368f59f033 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -10265,8 +10265,9 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) bool req_immediate_exit = false; /* Forbid vmenter if vcpu dirty ring is soft-full */
- if (unlikely(vcpu->kvm->dirty_ring_size &&
kvm_dirty_ring_soft_full(&vcpu->dirty_ring))) {
- if (kvm_check_request(KVM_REQ_RING_SOFT_FULL, vcpu) &&
kvm_dirty_ring_soft_full(&vcpu->dirty_ring)) {kvm_make_request(KVM_REQ_RING_SOFT_FULL, vcpu);
As commented previously - can we use kvm_test_request() instead? because we don't want to unconditionally clear the bit. Instead of making the request again, we can clear request only if !full.
We can also safely move this into the block of below kvm_request_pending() as Marc used to suggest.
To explicitly use kvm_clear_request(), we may need to be careful on the memory barriers. I'm wondering whether we should have moved smp_mb__after_atomic() into kvm_clear_request() because kvm_clear_request() is used outside kvm_check_request() and IIUC all the call sites should better have that barrier too to be safe.
Side note: when I read the code around I also see some mis-use of clear request where it can be omitted, e.g.:
if (kvm_check_request(KVM_REQ_UNHALT, vcpu)) { kvm_clear_request(KVM_REQ_UNHALT, vcpu); vcpu->run->exit_reason = KVM_EXIT_IRQ_WINDOW_OPEN; }
Maybe it's a sign of bad naming, so we should renamed kvm_check_request() to kvm_test_clear_request() too to show that clearing after that is not needed?
Thanks,
On Fri, 16 Sep 2022 19:09:52 +0100, Peter Xu peterx@redhat.com wrote:
On Fri, Sep 16, 2022 at 12:51:31PM +0800, Gavin Shan wrote:
This adds KVM_REQ_RING_SOFT_FULL, which is raised when the dirty ring of the specific VCPU becomes softly full in kvm_dirty_ring_push(). The VCPU is enforced to exit when the request is raised and its dirty ring is softly full on its entrance.
Suggested-by: Marc Zyngier maz@kernel.org Signed-off-by: Gavin Shan gshan@redhat.com
arch/x86/kvm/x86.c | 5 +++-- include/linux/kvm_host.h | 1 + virt/kvm/dirty_ring.c | 4 ++++ 3 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 43a6a7efc6ec..7f368f59f033 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -10265,8 +10265,9 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) bool req_immediate_exit = false; /* Forbid vmenter if vcpu dirty ring is soft-full */
- if (unlikely(vcpu->kvm->dirty_ring_size &&
kvm_dirty_ring_soft_full(&vcpu->dirty_ring))) {
- if (kvm_check_request(KVM_REQ_RING_SOFT_FULL, vcpu) &&
kvm_dirty_ring_soft_full(&vcpu->dirty_ring)) {kvm_make_request(KVM_REQ_RING_SOFT_FULL, vcpu);As commented previously - can we use kvm_test_request() instead? because we don't want to unconditionally clear the bit. Instead of making the request again, we can clear request only if !full.
I have the feeling that this is a micro-optimisation that won't lead to much benefit in practice. You already have the cache line, just not in exclusive mode, and given that this is per-vcpu, you'd only see the cost if someone else is setting a request to this vcpu while evaluating the local requests.
And now you need extra barriers...
Also, can we please refrain from changing things without data showing that this actually is worse than what we had before? The point below makes me think that this is actually beneficial as is.
We can also safely move this into the block of below kvm_request_pending() as Marc used to suggest.
This, on the other hand, makes sure that we share the cost across all requests. Requests should be extremely rare anyway (and if they aren't, you have a whole lot of performance issues on your hands anyway).
To explicitly use kvm_clear_request(), we may need to be careful on the memory barriers. I'm wondering whether we should have moved smp_mb__after_atomic() into kvm_clear_request() because kvm_clear_request() is used outside kvm_check_request() and IIUC all the call sites should better have that barrier too to be safe.
Side note: when I read the code around I also see some mis-use of clear request where it can be omitted, e.g.:
if (kvm_check_request(KVM_REQ_UNHALT, vcpu)) { kvm_clear_request(KVM_REQ_UNHALT, vcpu); vcpu->run->exit_reason = KVM_EXIT_IRQ_WINDOW_OPEN; }Maybe it's a sign of bad naming, so we should renamed kvm_check_request() to kvm_test_clear_request() too to show that clearing after that is not needed?
Yeah, this kvm_clear_request() is superfluous. But this is rather well documented, for once, and I don't think we should repaint it based on a sample of one.
Thanks,
M.
On 9/18/22 7:00 PM, Marc Zyngier wrote:
On Fri, 16 Sep 2022 19:09:52 +0100, Peter Xu peterx@redhat.com wrote:
On Fri, Sep 16, 2022 at 12:51:31PM +0800, Gavin Shan wrote:
This adds KVM_REQ_RING_SOFT_FULL, which is raised when the dirty ring of the specific VCPU becomes softly full in kvm_dirty_ring_push(). The VCPU is enforced to exit when the request is raised and its dirty ring is softly full on its entrance.
Suggested-by: Marc Zyngier maz@kernel.org Signed-off-by: Gavin Shan gshan@redhat.com
arch/x86/kvm/x86.c | 5 +++-- include/linux/kvm_host.h | 1 + virt/kvm/dirty_ring.c | 4 ++++ 3 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 43a6a7efc6ec..7f368f59f033 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -10265,8 +10265,9 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) bool req_immediate_exit = false; /* Forbid vmenter if vcpu dirty ring is soft-full */
- if (unlikely(vcpu->kvm->dirty_ring_size &&
kvm_dirty_ring_soft_full(&vcpu->dirty_ring))) {
- if (kvm_check_request(KVM_REQ_RING_SOFT_FULL, vcpu) &&
kvm_dirty_ring_soft_full(&vcpu->dirty_ring)) {kvm_make_request(KVM_REQ_RING_SOFT_FULL, vcpu);As commented previously - can we use kvm_test_request() instead? because we don't want to unconditionally clear the bit. Instead of making the request again, we can clear request only if !full.
I have the feeling that this is a micro-optimisation that won't lead to much benefit in practice. You already have the cache line, just not in exclusive mode, and given that this is per-vcpu, you'd only see the cost if someone else is setting a request to this vcpu while evaluating the local requests.
And now you need extra barriers...
Also, can we please refrain from changing things without data showing that this actually is worse than what we had before? The point below makes me think that this is actually beneficial as is.
I think Marc's explanation makes sense. It won't make difference in terms of performance. We need to explicitly handle barrier when kvm_test_request() is used. So I prefer to keep the code if Peter agrees.
We can also safely move this into the block of below kvm_request_pending() as Marc used to suggest.
This, on the other hand, makes sure that we share the cost across all requests. Requests should be extremely rare anyway (and if they aren't, you have a whole lot of performance issues on your hands anyway).
Yeah, We shouldn't have too much requests. I missed the comment from Marc to move this chunk to kvm_request_pending(). I will fix it in v3.
To explicitly use kvm_clear_request(), we may need to be careful on the memory barriers. I'm wondering whether we should have moved smp_mb__after_atomic() into kvm_clear_request() because kvm_clear_request() is used outside kvm_check_request() and IIUC all the call sites should better have that barrier too to be safe.
Side note: when I read the code around I also see some mis-use of clear request where it can be omitted, e.g.:
if (kvm_check_request(KVM_REQ_UNHALT, vcpu)) { kvm_clear_request(KVM_REQ_UNHALT, vcpu); vcpu->run->exit_reason = KVM_EXIT_IRQ_WINDOW_OPEN; }Maybe it's a sign of bad naming, so we should renamed kvm_check_request() to kvm_test_clear_request() too to show that clearing after that is not needed?
Yeah, this kvm_clear_request() is superfluous. But this is rather well documented, for once, and I don't think we should repaint it based on a sample of one.
Yeah, I think Peter is correct that smp_mb__after_atomic() would be part of kvm_clear_request(). Otherwise, the following two cases aren't in same order:
// kvm_check_request() // test and clear kvm_test_request() kvm_test_request() kvm_clear_request() kvm_clear_request() smp_mb__after_atomic()
Thanks, Gavin
On Mon, 19 Sep 2022 00:58:10 +0100, Gavin Shan gshan@redhat.com wrote:
On 9/18/22 7:00 PM, Marc Zyngier wrote:
On Fri, 16 Sep 2022 19:09:52 +0100, Peter Xu peterx@redhat.com wrote:
On Fri, Sep 16, 2022 at 12:51:31PM +0800, Gavin Shan wrote:
This adds KVM_REQ_RING_SOFT_FULL, which is raised when the dirty ring of the specific VCPU becomes softly full in kvm_dirty_ring_push(). The VCPU is enforced to exit when the request is raised and its dirty ring is softly full on its entrance.
Suggested-by: Marc Zyngier maz@kernel.org Signed-off-by: Gavin Shan gshan@redhat.com
arch/x86/kvm/x86.c | 5 +++-- include/linux/kvm_host.h | 1 + virt/kvm/dirty_ring.c | 4 ++++ 3 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 43a6a7efc6ec..7f368f59f033 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -10265,8 +10265,9 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) bool req_immediate_exit = false; /* Forbid vmenter if vcpu dirty ring is soft-full */
- if (unlikely(vcpu->kvm->dirty_ring_size &&
kvm_dirty_ring_soft_full(&vcpu->dirty_ring))) {
- if (kvm_check_request(KVM_REQ_RING_SOFT_FULL, vcpu) &&
kvm_dirty_ring_soft_full(&vcpu->dirty_ring)) {kvm_make_request(KVM_REQ_RING_SOFT_FULL, vcpu);As commented previously - can we use kvm_test_request() instead? because we don't want to unconditionally clear the bit. Instead of making the request again, we can clear request only if !full.
I have the feeling that this is a micro-optimisation that won't lead to much benefit in practice. You already have the cache line, just not in exclusive mode, and given that this is per-vcpu, you'd only see the cost if someone else is setting a request to this vcpu while evaluating the local requests.
And now you need extra barriers...
Also, can we please refrain from changing things without data showing that this actually is worse than what we had before? The point below makes me think that this is actually beneficial as is.
I think Marc's explanation makes sense. It won't make difference in terms of performance. We need to explicitly handle barrier when kvm_test_request() is used. So I prefer to keep the code if Peter agrees.
We can also safely move this into the block of below kvm_request_pending() as Marc used to suggest.
This, on the other hand, makes sure that we share the cost across all requests. Requests should be extremely rare anyway (and if they aren't, you have a whole lot of performance issues on your hands anyway).
Yeah, We shouldn't have too much requests. I missed the comment from Marc to move this chunk to kvm_request_pending(). I will fix it in v3.
To explicitly use kvm_clear_request(), we may need to be careful on the memory barriers. I'm wondering whether we should have moved smp_mb__after_atomic() into kvm_clear_request() because kvm_clear_request() is used outside kvm_check_request() and IIUC all the call sites should better have that barrier too to be safe.
Side note: when I read the code around I also see some mis-use of clear request where it can be omitted, e.g.:
if (kvm_check_request(KVM_REQ_UNHALT, vcpu)) { kvm_clear_request(KVM_REQ_UNHALT, vcpu); vcpu->run->exit_reason = KVM_EXIT_IRQ_WINDOW_OPEN; }Maybe it's a sign of bad naming, so we should renamed kvm_check_request() to kvm_test_clear_request() too to show that clearing after that is not needed?
Yeah, this kvm_clear_request() is superfluous. But this is rather well documented, for once, and I don't think we should repaint it based on a sample of one.
Yeah, I think Peter is correct that smp_mb__after_atomic() would be part of kvm_clear_request(). Otherwise, the following two cases aren't in same order:
// kvm_check_request() // test and clear kvm_test_request() kvm_test_request() kvm_clear_request() kvm_clear_request() smp_mb__after_atomic()
[fixing Drew's email, again]
Oh, I totally agree that a standalone use of kvm_clear_request() must come with a barrier. It is just that without additional data, it isn't obvious to me that there is any need for kvm_clear_request() itself to have the barrier.
In a number of cases, kvm_clear_request() is used on in the context of the same vcpu, and this should be enough to ensure visibility (for example, I don't think kvm_vcpu_wfi() should require this barrier).
But maybe I'm missing something.
Thanks,
M.
On Mon, Sep 19, 2022 at 09:58:10AM +1000, Gavin Shan wrote:
I think Marc's explanation makes sense. It won't make difference in terms of performance. We need to explicitly handle barrier when kvm_test_request() is used. So I prefer to keep the code if Peter agrees.
No strong opinion here. I keep thinking clear+set look awkward even if it's unlikely path to trigger (ring should be recycled when reaching here for any sane user app), but if it's already 2:1 then please go ahead. :)
Thanks,
This enables the ring-based dirty memory tracking on ARM64. The feature is configured by CONFIG_HAVE_KVM_DIRTY_RING, detected and enabled by KVM_CAP_DIRTY_LOG_RING. A ring buffer is created on every VCPU when the feature is enabled. Each entry in the ring buffer is described by 'struct kvm_dirty_gfn'.
A ring buffer entry is pushed when a page becomes dirty on host, and pulled by userspace after the ring buffer is mapped at physical page offset KVM_DIRTY_LOG_PAGE_OFFSET. The specific VCPU is enforced to exit if its ring buffer becomes softly full. Besides, the ring buffer can be reset by ioctl command KVM_RESET_DIRTY_RINGS to release those pulled ring buffer entries.
Signed-off-by: Gavin Shan gshan@redhat.com --- Documentation/virt/kvm/api.rst | 2 +- arch/arm64/include/uapi/asm/kvm.h | 1 + arch/arm64/kvm/Kconfig | 1 + arch/arm64/kvm/arm.c | 8 ++++++++ 4 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst index abd7c32126ce..19fa1ac017ed 100644 --- a/Documentation/virt/kvm/api.rst +++ b/Documentation/virt/kvm/api.rst @@ -8022,7 +8022,7 @@ regardless of what has actually been exposed through the CPUID leaf. 8.29 KVM_CAP_DIRTY_LOG_RING ---------------------------
-:Architectures: x86 +:Architectures: x86, arm64 :Parameters: args[0] - size of the dirty log ring
KVM is capable of tracking dirty memory using ring buffers that are diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h index 316917b98707..a7a857f1784d 100644 --- a/arch/arm64/include/uapi/asm/kvm.h +++ b/arch/arm64/include/uapi/asm/kvm.h @@ -43,6 +43,7 @@ #define __KVM_HAVE_VCPU_EVENTS
#define KVM_COALESCED_MMIO_PAGE_OFFSET 1 +#define KVM_DIRTY_LOG_PAGE_OFFSET 64
#define KVM_REG_SIZE(id) \ (1U << (((id) & KVM_REG_SIZE_MASK) >> KVM_REG_SIZE_SHIFT)) diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig index 815cc118c675..0309b2d0f2da 100644 --- a/arch/arm64/kvm/Kconfig +++ b/arch/arm64/kvm/Kconfig @@ -32,6 +32,7 @@ menuconfig KVM select KVM_VFIO select HAVE_KVM_EVENTFD select HAVE_KVM_IRQFD + select HAVE_KVM_DIRTY_RING select HAVE_KVM_MSI select HAVE_KVM_IRQCHIP select HAVE_KVM_IRQ_ROUTING diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index 2ff0ef62abad..76816f8e082b 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -747,6 +747,14 @@ static int check_vcpu_requests(struct kvm_vcpu *vcpu)
if (kvm_check_request(KVM_REQ_SUSPEND, vcpu)) return kvm_vcpu_suspend(vcpu); + + if (kvm_check_request(KVM_REQ_RING_SOFT_FULL, vcpu) && + kvm_dirty_ring_soft_full(&vcpu->dirty_ring)) { + kvm_make_request(KVM_REQ_RING_SOFT_FULL, vcpu); + vcpu->run->exit_reason = KVM_EXIT_DIRTY_RING_FULL; + trace_kvm_dirty_ring_exit(vcpu); + return 0; + } }
return 1;
Hi Gavin,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on kvm/queue] [also build test WARNING on kvmarm/next linus/master v6.0-rc5 next-20220916] [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/Gavin-Shan/KVM-arm64-Enable-r... base: https://git.kernel.org/pub/scm/virt/kvm/kvm.git queue config: arm64-allyesconfig (https://download.01.org/0day-ci/archive/20220918/202209180726.FLL69aKA-lkp@i...) compiler: aarch64-linux-gcc (GCC) 12.1.0 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/a1a4cd8f6a5e8927f800baff7d9658... git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Gavin-Shan/KVM-arm64-Enable-ring-based-dirty-memory-tracking/20220916-125417 git checkout a1a4cd8f6a5e8927f800baff7d965870a1b7d7ba # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable Reported-by: kernel test robot lkp@intel.com
All warnings (new ones prefixed by >>):
arch/arm64/kvm/../../../virt/kvm/dirty_ring.c:14:12: warning: no previous prototype for 'kvm_cpu_dirty_log_size' [-Wmissing-prototypes]
14 | int __weak kvm_cpu_dirty_log_size(void) | ^~~~~~~~~~~~~~~~~~~~~~
vim +/kvm_cpu_dirty_log_size +14 arch/arm64/kvm/../../../virt/kvm/dirty_ring.c
fb04a1eddb1a65 Peter Xu 2020-09-30 13 fb04a1eddb1a65 Peter Xu 2020-09-30 @14 int __weak kvm_cpu_dirty_log_size(void) fb04a1eddb1a65 Peter Xu 2020-09-30 15 { fb04a1eddb1a65 Peter Xu 2020-09-30 16 return 0; fb04a1eddb1a65 Peter Xu 2020-09-30 17 } fb04a1eddb1a65 Peter Xu 2020-09-30 18
On 9/18/22 1:10 AM, kernel test robot wrote:
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on kvm/queue] [also build test WARNING on kvmarm/next linus/master v6.0-rc5 next-20220916] [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/Gavin-Shan/KVM-arm64-Enable-r... base: https://git.kernel.org/pub/scm/virt/kvm/kvm.git queue config: arm64-allyesconfig (https://download.01.org/0day-ci/archive/20220918/202209180726.FLL69aKA-lkp@i...) compiler: aarch64-linux-gcc (GCC) 12.1.0 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/a1a4cd8f6a5e8927f800baff7d9658... git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Gavin-Shan/KVM-arm64-Enable-ring-based-dirty-memory-tracking/20220916-125417 git checkout a1a4cd8f6a5e8927f800baff7d965870a1b7d7ba # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable Reported-by: kernel test robot lkp@intel.com
All warnings (new ones prefixed by >>):
arch/arm64/kvm/../../../virt/kvm/dirty_ring.c:14:12: warning: no previous prototype for 'kvm_cpu_dirty_log_size' [-Wmissing-prototypes]
14 | int __weak kvm_cpu_dirty_log_size(void) | ^~~~~~~~~~~~~~~~~~~~~~
[...]
Thanks for reporting the issue. The weak function needn't to be overrided by ARM64. In v3, I will have a separate patch to fix the issue by moving its declaration to kvm_dirty_ring.h.
Thanks, Gavin
In vcpu_map_dirty_ring(), the guest's page size is used to figure out the offset in the virtual area. It works fine when we have same page sizes on host and guest. However, it fails when the page sizes on host and guest are different on arm64, like below error messages indicates.
# ./dirty_log_test -M dirty-ring -m 7 Setting log mode to: 'dirty-ring' Test iterations: 32, interval: 10 (ms) Testing guest mode: PA-bits:40, VA-bits:48, 64K pages guest physical test memory offset: 0xffbffc0000 vcpu stops because vcpu is kicked out... Notifying vcpu to continue vcpu continues now. ==== Test Assertion Failure ==== lib/kvm_util.c:1477: addr == MAP_FAILED pid=9000 tid=9000 errno=0 - Success 1 0x0000000000405f5b: vcpu_map_dirty_ring at kvm_util.c:1477 2 0x0000000000402ebb: dirty_ring_collect_dirty_pages at dirty_log_test.c:349 3 0x00000000004029b3: log_mode_collect_dirty_pages at dirty_log_test.c:478 4 (inlined by) run_test at dirty_log_test.c:778 5 (inlined by) run_test at dirty_log_test.c:691 6 0x0000000000403a57: for_each_guest_mode at guest_modes.c:105 7 0x0000000000401ccf: main at dirty_log_test.c:921 8 0x0000ffffb06ec79b: ?? ??:0 9 0x0000ffffb06ec86b: ?? ??:0 10 0x0000000000401def: _start at ??:? Dirty ring mapped private
Fix the issue by using host's page size to map the ring buffer.
Signed-off-by: Gavin Shan gshan@redhat.com --- tools/testing/selftests/kvm/lib/kvm_util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index 9889fe0d8919..4e823cbe6b48 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -1464,7 +1464,7 @@ struct kvm_reg_list *vcpu_get_reg_list(struct kvm_vcpu *vcpu)
void *vcpu_map_dirty_ring(struct kvm_vcpu *vcpu) { - uint32_t page_size = vcpu->vm->page_size; + uint32_t page_size = getpagesize(); uint32_t size = vcpu->vm->dirty_ring_size;
TEST_ASSERT(size > 0, "Should enable dirty ring first");
There are two states, which need to be cleared before next mode is executed. Otherwise, we will hit failure as the following messages indicate.
- The variable 'dirty_ring_vcpu_ring_full' shared by main and vcpu thread. It's indicating if the vcpu exit due to full ring buffer. The value can be carried from previous mode (VM_MODE_P40V48_4K) to current one (VM_MODE_P40V48_64K) when VM_MODE_P40V48_16K isn't supported.
- The current ring buffer index needs to be reset before next mode (VM_MODE_P40V48_64K) is executed. Otherwise, the stale value is carried from previous mode (VM_MODE_P40V48_4K).
# ./dirty_log_test -M dirty-ring Setting log mode to: 'dirty-ring' Test iterations: 32, interval: 10 (ms) Testing guest mode: PA-bits:40, VA-bits:48, 4K pages guest physical test memory offset: 0xffbfffc000 : Dirtied 995328 pages Total bits checked: dirty (1012434), clear (7114123), track_next (966700) Testing guest mode: PA-bits:40, VA-bits:48, 64K pages guest physical test memory offset: 0xffbffc0000 vcpu stops because vcpu is kicked out... vcpu continues now. Notifying vcpu to continue Iteration 1 collected 0 pages vcpu stops because dirty ring is full... vcpu continues now. vcpu stops because dirty ring is full... vcpu continues now. vcpu stops because dirty ring is full... ==== Test Assertion Failure ==== dirty_log_test.c:369: cleared == count pid=10541 tid=10541 errno=22 - Invalid argument 1 0x0000000000403087: dirty_ring_collect_dirty_pages at dirty_log_test.c:369 2 0x0000000000402a0b: log_mode_collect_dirty_pages at dirty_log_test.c:492 3 (inlined by) run_test at dirty_log_test.c:795 4 (inlined by) run_test at dirty_log_test.c:705 5 0x0000000000403a37: for_each_guest_mode at guest_modes.c:100 6 0x0000000000401ccf: main at dirty_log_test.c:938 7 0x0000ffff9ecd279b: ?? ??:0 8 0x0000ffff9ecd286b: ?? ??:0 9 0x0000000000401def: _start at ??:? Reset dirty pages (0) mismatch with collected (35566)
Fix the issues by clearing 'dirty_ring_vcpu_ring_full' and the ring buffer index before next new mode is to be executed.
Signed-off-by: Gavin Shan gshan@redhat.com --- tools/testing/selftests/kvm/dirty_log_test.c | 27 ++++++++++++-------- 1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c index 9c883c94d478..7d91df7e036f 100644 --- a/tools/testing/selftests/kvm/dirty_log_test.c +++ b/tools/testing/selftests/kvm/dirty_log_test.c @@ -225,13 +225,15 @@ static void clear_log_create_vm_done(struct kvm_vm *vm) }
static void dirty_log_collect_dirty_pages(struct kvm_vcpu *vcpu, int slot, - void *bitmap, uint32_t num_pages) + void *bitmap, uint32_t num_pages, + uint32_t *unused) { kvm_vm_get_dirty_log(vcpu->vm, slot, bitmap); }
static void clear_log_collect_dirty_pages(struct kvm_vcpu *vcpu, int slot, - void *bitmap, uint32_t num_pages) + void *bitmap, uint32_t num_pages, + uint32_t *unused) { kvm_vm_get_dirty_log(vcpu->vm, slot, bitmap); kvm_vm_clear_dirty_log(vcpu->vm, slot, bitmap, 0, num_pages); @@ -327,10 +329,9 @@ static void dirty_ring_continue_vcpu(void) }
static void dirty_ring_collect_dirty_pages(struct kvm_vcpu *vcpu, int slot, - void *bitmap, uint32_t num_pages) + void *bitmap, uint32_t num_pages, + uint32_t *ring_buf_idx) { - /* We only have one vcpu */ - static uint32_t fetch_index = 0; uint32_t count = 0, cleared; bool continued_vcpu = false;
@@ -347,7 +348,8 @@ static void dirty_ring_collect_dirty_pages(struct kvm_vcpu *vcpu, int slot,
/* Only have one vcpu */ count = dirty_ring_collect_one(vcpu_map_dirty_ring(vcpu), - slot, bitmap, num_pages, &fetch_index); + slot, bitmap, num_pages, + ring_buf_idx);
cleared = kvm_vm_reset_dirty_ring(vcpu->vm);
@@ -404,7 +406,8 @@ struct log_mode { void (*create_vm_done)(struct kvm_vm *vm); /* Hook to collect the dirty pages into the bitmap provided */ void (*collect_dirty_pages) (struct kvm_vcpu *vcpu, int slot, - void *bitmap, uint32_t num_pages); + void *bitmap, uint32_t num_pages, + uint32_t *ring_buf_idx); /* Hook to call when after each vcpu run */ void (*after_vcpu_run)(struct kvm_vcpu *vcpu, int ret, int err); void (*before_vcpu_join) (void); @@ -469,13 +472,14 @@ static void log_mode_create_vm_done(struct kvm_vm *vm) }
static void log_mode_collect_dirty_pages(struct kvm_vcpu *vcpu, int slot, - void *bitmap, uint32_t num_pages) + void *bitmap, uint32_t num_pages, + uint32_t *ring_buf_idx) { struct log_mode *mode = &log_modes[host_log_mode];
TEST_ASSERT(mode->collect_dirty_pages != NULL, "collect_dirty_pages() is required for any log mode!"); - mode->collect_dirty_pages(vcpu, slot, bitmap, num_pages); + mode->collect_dirty_pages(vcpu, slot, bitmap, num_pages, ring_buf_idx); }
static void log_mode_after_vcpu_run(struct kvm_vcpu *vcpu, int ret, int err) @@ -694,6 +698,7 @@ static void run_test(enum vm_guest_mode mode, void *arg) struct kvm_vcpu *vcpu; struct kvm_vm *vm; unsigned long *bmap; + uint32_t ring_buf_idx = 0;
if (!log_mode_supported()) { print_skip("Log mode '%s' not supported", @@ -769,6 +774,7 @@ static void run_test(enum vm_guest_mode mode, void *arg) host_dirty_count = 0; host_clear_count = 0; host_track_next_count = 0; + WRITE_ONCE(dirty_ring_vcpu_ring_full, false);
pthread_create(&vcpu_thread, NULL, vcpu_worker, vcpu);
@@ -776,7 +782,8 @@ static void run_test(enum vm_guest_mode mode, void *arg) /* Give the vcpu thread some time to dirty some pages */ usleep(p->interval * 1000); log_mode_collect_dirty_pages(vcpu, TEST_MEM_SLOT_INDEX, - bmap, host_num_pages); + bmap, host_num_pages, + &ring_buf_idx);
/* * See vcpu_sync_stop_requested definition for details on why
In the dirty ring case, we rely on vcpu exit due to full dirty ring state. On ARM64 system, there are 4096 host pages when the host page size is 64KB. In this case, the vcpu never exits due to the full dirty ring state. The similar case is 4KB page size on host and 64KB page size on guest. The vcpu corrupts same set of host pages, but the dirty page information isn't collected in the main thread. This leads to infinite loop as the following log shows.
# ./dirty_log_test -M dirty-ring -c 65536 -m 5 Setting log mode to: 'dirty-ring' Test iterations: 32, interval: 10 (ms) Testing guest mode: PA-bits:40, VA-bits:48, 4K pages guest physical test memory offset: 0xffbffe0000 vcpu stops because vcpu is kicked out... Notifying vcpu to continue vcpu continues now. Iteration 1 collected 576 pages <No more output afterwards>
Fix the issue by automatically choosing the best dirty ring size, to ensure vcpu exit due to full dirty ring state. The option '-c' becomes a hint to the dirty ring count, instead of the value of it.
Signed-off-by: Gavin Shan gshan@redhat.com --- tools/testing/selftests/kvm/dirty_log_test.c | 26 +++++++++++++++++--- 1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c index 7d91df7e036f..47ac2c719ade 100644 --- a/tools/testing/selftests/kvm/dirty_log_test.c +++ b/tools/testing/selftests/kvm/dirty_log_test.c @@ -23,6 +23,9 @@ #include "guest_modes.h" #include "processor.h"
+#define DIRTY_MEM_BITS 30 /* 1G */ +#define PAGE_SHIFT_4K 12 + /* The memory slot index to track dirty pages */ #define TEST_MEM_SLOT_INDEX 1
@@ -271,6 +274,24 @@ static bool dirty_ring_supported(void)
static void dirty_ring_create_vm_done(struct kvm_vm *vm) { + uint64_t pages; + uint32_t limit; + + /* + * We rely on vcpu exit due to full dirty ring state. Adjust + * the ring buffer size to ensure we're able to reach the + * full dirty ring state. + */ + pages = (1ul << (DIRTY_MEM_BITS - vm->page_shift)) + 3; + pages = vm_adjust_num_guest_pages(vm->mode, pages); + if (vm->page_size < getpagesize()) + pages = vm_num_host_pages(vm->mode, pages); + + limit = 1 << (31 - __builtin_clz(pages)); + test_dirty_ring_count = 1 << (31 - __builtin_clz(test_dirty_ring_count)); + test_dirty_ring_count = min(limit, test_dirty_ring_count); + pr_info("dirty ring count: 0x%x\n", test_dirty_ring_count); + /* * Switch to dirty ring mode after VM creation but before any * of the vcpu creation. @@ -683,9 +704,6 @@ static struct kvm_vm *create_vm(enum vm_guest_mode mode, struct kvm_vcpu **vcpu, return vm; }
-#define DIRTY_MEM_BITS 30 /* 1G */ -#define PAGE_SHIFT_4K 12 - struct test_params { unsigned long iterations; unsigned long interval; @@ -828,7 +846,7 @@ static void help(char *name) printf("usage: %s [-h] [-i iterations] [-I interval] " "[-p offset] [-m mode]\n", name); puts(""); - printf(" -c: specify dirty ring size, in number of entries\n"); + printf(" -c: hint to dirty ring size, in number of entries\n"); printf(" (only useful for dirty-ring test; default: %"PRIu32")\n", TEST_DIRTY_RING_COUNT); printf(" -i: specify iteration counts (default: %"PRIu64")\n",
linux-kselftest-mirror@lists.linaro.org
-
Gavin Shan -
kernel test robot -
Marc Zyngier -
Peter Xu