[PATCH v2 0/5] KVM: arm64: Enable ring-based dirty memory tracking

[PATCH v2 0/5] KVM: arm64: Enable ring-based dirty memory tracking

[Gavin Shan]

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(-)

-- 
2.23.0

[PATCH v2 1/5] KVM: x86: Introduce KVM_REQ_RING_SOFT_FULL

[Gavin Shan]

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)
-- 
2.23.0

[PATCH v2 2/5] KVM: arm64: Enable ring-based dirty memory tracking

[Gavin Shan]

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;
-- 
2.23.0

[PATCH v2 3/5] KVM: selftests: Use host page size to map ring buffer in dirty_log_test

[Gavin Shan]

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");
-- 
2.23.0

[PATCH v2 4/5] KVM: selftests: Clear dirty ring states between two modes in dirty_log_test

[Gavin Shan]

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
-- 
2.23.0

[PATCH v2 5/5] KVM: selftests: Automate choosing dirty ring size in dirty_log_test

[Gavin Shan]

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",
-- 
2.23.0

Re: [PATCH v2 1/5] KVM: x86: Introduce KVM_REQ_RING_SOFT_FULL

[Peter Xu]

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);
>  		vcpu->run->exit_reason = KVM_EXIT_DIRTY_RING_FULL;
>  		trace_kvm_dirty_ring_exit(vcpu);
>  		r = 0;

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,

-- 
Peter Xu

Re: [PATCH v2 2/5] KVM: arm64: Enable ring-based dirty memory tracking

[kernel test robot]

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-ring-based-dirty-memory-tracking/20220916-125417
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@intel.com/config)
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/a1a4cd8f6a5e8927f800baff7d965870a1b7d7ba
        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  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v2 1/5] KVM: x86: Introduce KVM_REQ_RING_SOFT_FULL

[Marc Zyngier]

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);
> >  		vcpu->run->exit_reason = KVM_EXIT_DIRTY_RING_FULL;
> >  		trace_kvm_dirty_ring_exit(vcpu);
> >  		r = 0;
> 
> 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.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v2 1/5] KVM: x86: Introduce KVM_REQ_RING_SOFT_FULL

[Gavin Shan]

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);
>>>   		vcpu->run->exit_reason = KVM_EXIT_DIRTY_RING_FULL;
>>>   		trace_kvm_dirty_ring_exit(vcpu);
>>>   		r = 0;
>>
>> 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

Re: [PATCH v2 2/5] KVM: arm64: Enable ring-based dirty memory tracking

[Gavin Shan]

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-ring-based-dirty-memory-tracking/20220916-125417
> 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@intel.com/config)
> 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/a1a4cd8f6a5e8927f800baff7d965870a1b7d7ba
>          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

Re: [PATCH v2 1/5] KVM: x86: Introduce KVM_REQ_RING_SOFT_FULL

[Marc Zyngier]

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);
> >>>   		vcpu->run->exit_reason = KVM_EXIT_DIRTY_RING_FULL;
> >>>   		trace_kvm_dirty_ring_exit(vcpu);
> >>>   		r = 0;
> >> 
> >> 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.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v2 1/5] KVM: x86: Introduce KVM_REQ_RING_SOFT_FULL

[Peter Xu]

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,

-- 
Peter Xu