* [PATCH] KVM: stats: Add VM dirty_pages stats for the number of dirty pages
@ 2021-08-10 22:32 Jing Zhang
2021-08-10 22:44 ` Peter Feiner
` (2 more replies)
0 siblings, 3 replies; 22+ messages in thread
From: Jing Zhang @ 2021-08-10 22:32 UTC (permalink / raw)
To: KVM, KVMPPC, Paolo Bonzini, Peter Shier, Oliver Upton,
David Matlack, Ben Gardon, Peter Feiner
Cc: Jing Zhang
Add a generic VM stats dirty_pages to record the number of dirty pages
reflected in dirty_bitmap at the moment.
Original-by: Peter Feiner <pfeiner@google.com>
Signed-off-by: Jing Zhang <jingzhangos@google.com>
---
arch/powerpc/kvm/book3s_64_mmu_hv.c | 8 ++++++--
arch/powerpc/kvm/book3s_64_mmu_radix.c | 1 +
arch/powerpc/kvm/book3s_hv_rm_mmu.c | 1 +
include/linux/kvm_host.h | 3 ++-
include/linux/kvm_types.h | 1 +
virt/kvm/kvm_main.c | 26 +++++++++++++++++++++++---
6 files changed, 34 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
index c63e263312a4..e4aafa10efa1 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
@@ -1122,8 +1122,10 @@ long kvmppc_hv_get_dirty_log_hpt(struct kvm *kvm,
* since we always put huge-page HPTEs in the rmap chain
* corresponding to their page base address.
*/
- if (npages)
+ if (npages) {
set_dirty_bits(map, i, npages);
+ kvm->stat.generic.dirty_pages += npages;
+ }
++rmapp;
}
preempt_enable();
@@ -1178,8 +1180,10 @@ void kvmppc_unpin_guest_page(struct kvm *kvm, void *va, unsigned long gpa,
gfn = gpa >> PAGE_SHIFT;
srcu_idx = srcu_read_lock(&kvm->srcu);
memslot = gfn_to_memslot(kvm, gfn);
- if (memslot && memslot->dirty_bitmap)
+ if (memslot && memslot->dirty_bitmap) {
set_bit_le(gfn - memslot->base_gfn, memslot->dirty_bitmap);
+ ++kvm->stat.generic.dirty_pages;
+ }
srcu_read_unlock(&kvm->srcu, srcu_idx);
}
diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c
index b5905ae4377c..3a6cb3854a44 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_radix.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c
@@ -1150,6 +1150,7 @@ long kvmppc_hv_get_dirty_log_radix(struct kvm *kvm,
j = i + 1;
if (npages) {
set_dirty_bits(map, i, npages);
+ kvm->stat.generic.dirty_pages += npages;
j = i + npages;
}
}
diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
index 632b2545072b..16806bc473fa 100644
--- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
+++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
@@ -109,6 +109,7 @@ void kvmppc_update_dirty_map(const struct kvm_memory_slot *memslot,
npages = (psize + PAGE_SIZE - 1) / PAGE_SIZE;
gfn -= memslot->base_gfn;
set_dirty_bits_atomic(memslot->dirty_bitmap, gfn, npages);
+ kvm->stat.generic.dirty_pages += npages;
}
EXPORT_SYMBOL_GPL(kvmppc_update_dirty_map);
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index f50bfcf225f0..1e8e66fb915b 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1421,7 +1421,8 @@ struct _kvm_stats_desc {
KVM_STATS_BASE_POW10, -9)
#define KVM_GENERIC_VM_STATS() \
- STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush)
+ STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush), \
+ STATS_DESC_COUNTER(VM_GENERIC, dirty_pages)
#define KVM_GENERIC_VCPU_STATS() \
STATS_DESC_COUNTER(VCPU_GENERIC, halt_successful_poll), \
diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
index ed6a985c5680..6c05df00aebf 100644
--- a/include/linux/kvm_types.h
+++ b/include/linux/kvm_types.h
@@ -78,6 +78,7 @@ struct kvm_mmu_memory_cache {
struct kvm_vm_stat_generic {
u64 remote_tlb_flush;
+ u64 dirty_pages;
};
struct kvm_vcpu_stat_generic {
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index a438a7a3774a..93f0ca2ea326 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1228,6 +1228,19 @@ static int kvm_alloc_dirty_bitmap(struct kvm_memory_slot *memslot)
return 0;
}
+static inline unsigned long hweight_dirty_bitmap(
+ struct kvm_memory_slot *memslot)
+{
+ unsigned long i;
+ unsigned long count = 0;
+ unsigned long n = kvm_dirty_bitmap_bytes(memslot);
+
+ for (i = 0; i < n / sizeof(long); ++i)
+ count += hweight_long(memslot->dirty_bitmap[i]);
+
+ return count;
+}
+
/*
* Delete a memslot by decrementing the number of used slots and shifting all
* other entries in the array forward one spot.
@@ -1612,6 +1625,7 @@ static int kvm_delete_memslot(struct kvm *kvm,
if (r)
return r;
+ kvm->stat.generic.dirty_pages -= hweight_dirty_bitmap(old);
kvm_free_memslot(kvm, old);
return 0;
}
@@ -1733,8 +1747,10 @@ int __kvm_set_memory_region(struct kvm *kvm,
if (r)
goto out_bitmap;
- if (old.dirty_bitmap && !new.dirty_bitmap)
+ if (old.dirty_bitmap && !new.dirty_bitmap) {
+ kvm->stat.generic.dirty_pages -= hweight_dirty_bitmap(&old);
kvm_destroy_dirty_bitmap(&old);
+ }
return 0;
out_bitmap:
@@ -1895,6 +1911,7 @@ static int kvm_get_dirty_log_protect(struct kvm *kvm, struct kvm_dirty_log *log)
offset = i * BITS_PER_LONG;
kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot,
offset, mask);
+ kvm->stat.generic.dirty_pages -= hweight_long(mask);
}
KVM_MMU_UNLOCK(kvm);
}
@@ -2012,6 +2029,7 @@ static int kvm_clear_dirty_log_protect(struct kvm *kvm,
flush = true;
kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot,
offset, mask);
+ kvm->stat.generic.dirty_pages -= hweight_long(mask);
}
}
KVM_MMU_UNLOCK(kvm);
@@ -3062,11 +3080,13 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
unsigned long rel_gfn = gfn - memslot->base_gfn;
u32 slot = (memslot->as_id << 16) | memslot->id;
- if (kvm->dirty_ring_size)
+ if (kvm->dirty_ring_size) {
kvm_dirty_ring_push(kvm_dirty_ring_get(kvm),
slot, rel_gfn);
- else
+ } else {
set_bit_le(rel_gfn, memslot->dirty_bitmap);
+ ++kvm->stat.generic.dirty_pages;
+ }
}
}
EXPORT_SYMBOL_GPL(mark_page_dirty_in_slot);
base-commit: d0732b0f8884d9cc0eca0082bbaef043f3fef7fb
--
2.32.0.605.g8dce9f2422-goog
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: stats: Add VM dirty_pages stats for the number of dirty pages
2021-08-10 22:32 [PATCH] KVM: stats: Add VM dirty_pages stats for the number of dirty pages Jing Zhang
@ 2021-08-10 22:44 ` Peter Feiner
2021-08-10 22:56 ` Oliver Upton
2021-08-11 12:26 ` kernel test robot
2 siblings, 0 replies; 22+ messages in thread
From: Peter Feiner @ 2021-08-10 22:44 UTC (permalink / raw)
To: Jing Zhang
Cc: KVM, KVMPPC, Paolo Bonzini, Peter Shier, Oliver Upton,
David Matlack, Ben Gardon
On Tue, Aug 10, 2021 at 3:32 PM Jing Zhang <jingzhangos@google.com> wrote:
>
> Add a generic VM stats dirty_pages to record the number of dirty pages
> reflected in dirty_bitmap at the moment.
>
> Original-by: Peter Feiner <pfeiner@google.com>
> Signed-off-by: Jing Zhang <jingzhangos@google.com>
> ---
> arch/powerpc/kvm/book3s_64_mmu_hv.c | 8 ++++++--
> arch/powerpc/kvm/book3s_64_mmu_radix.c | 1 +
> arch/powerpc/kvm/book3s_hv_rm_mmu.c | 1 +
> include/linux/kvm_host.h | 3 ++-
> include/linux/kvm_types.h | 1 +
> virt/kvm/kvm_main.c | 26 +++++++++++++++++++++++---
> 6 files changed, 34 insertions(+), 6 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> index c63e263312a4..e4aafa10efa1 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> @@ -1122,8 +1122,10 @@ long kvmppc_hv_get_dirty_log_hpt(struct kvm *kvm,
> * since we always put huge-page HPTEs in the rmap chain
> * corresponding to their page base address.
> */
> - if (npages)
> + if (npages) {
> set_dirty_bits(map, i, npages);
> + kvm->stat.generic.dirty_pages += npages;
> + }
> ++rmapp;
> }
> preempt_enable();
> @@ -1178,8 +1180,10 @@ void kvmppc_unpin_guest_page(struct kvm *kvm, void *va, unsigned long gpa,
> gfn = gpa >> PAGE_SHIFT;
> srcu_idx = srcu_read_lock(&kvm->srcu);
> memslot = gfn_to_memslot(kvm, gfn);
> - if (memslot && memslot->dirty_bitmap)
> + if (memslot && memslot->dirty_bitmap) {
> set_bit_le(gfn - memslot->base_gfn, memslot->dirty_bitmap);
> + ++kvm->stat.generic.dirty_pages;
> + }
> srcu_read_unlock(&kvm->srcu, srcu_idx);
> }
>
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> index b5905ae4377c..3a6cb3854a44 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_radix.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> @@ -1150,6 +1150,7 @@ long kvmppc_hv_get_dirty_log_radix(struct kvm *kvm,
> j = i + 1;
> if (npages) {
> set_dirty_bits(map, i, npages);
> + kvm->stat.generic.dirty_pages += npages;
> j = i + npages;
> }
> }
> diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> index 632b2545072b..16806bc473fa 100644
> --- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> +++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> @@ -109,6 +109,7 @@ void kvmppc_update_dirty_map(const struct kvm_memory_slot *memslot,
> npages = (psize + PAGE_SIZE - 1) / PAGE_SIZE;
> gfn -= memslot->base_gfn;
> set_dirty_bits_atomic(memslot->dirty_bitmap, gfn, npages);
> + kvm->stat.generic.dirty_pages += npages;
> }
> EXPORT_SYMBOL_GPL(kvmppc_update_dirty_map);
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index f50bfcf225f0..1e8e66fb915b 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1421,7 +1421,8 @@ struct _kvm_stats_desc {
> KVM_STATS_BASE_POW10, -9)
>
> #define KVM_GENERIC_VM_STATS() \
> - STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush)
> + STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush), \
> + STATS_DESC_COUNTER(VM_GENERIC, dirty_pages)
>
> #define KVM_GENERIC_VCPU_STATS() \
> STATS_DESC_COUNTER(VCPU_GENERIC, halt_successful_poll), \
> diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
> index ed6a985c5680..6c05df00aebf 100644
> --- a/include/linux/kvm_types.h
> +++ b/include/linux/kvm_types.h
> @@ -78,6 +78,7 @@ struct kvm_mmu_memory_cache {
>
> struct kvm_vm_stat_generic {
> u64 remote_tlb_flush;
> + u64 dirty_pages;
> };
>
> struct kvm_vcpu_stat_generic {
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index a438a7a3774a..93f0ca2ea326 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1228,6 +1228,19 @@ static int kvm_alloc_dirty_bitmap(struct kvm_memory_slot *memslot)
> return 0;
> }
>
> +static inline unsigned long hweight_dirty_bitmap(
> + struct kvm_memory_slot *memslot)
> +{
> + unsigned long i;
> + unsigned long count = 0;
> + unsigned long n = kvm_dirty_bitmap_bytes(memslot);
> +
> + for (i = 0; i < n / sizeof(long); ++i)
> + count += hweight_long(memslot->dirty_bitmap[i]);
> +
> + return count;
> +}
> +
> /*
> * Delete a memslot by decrementing the number of used slots and shifting all
> * other entries in the array forward one spot.
> @@ -1612,6 +1625,7 @@ static int kvm_delete_memslot(struct kvm *kvm,
> if (r)
> return r;
>
> + kvm->stat.generic.dirty_pages -= hweight_dirty_bitmap(old);
> kvm_free_memslot(kvm, old);
> return 0;
> }
> @@ -1733,8 +1747,10 @@ int __kvm_set_memory_region(struct kvm *kvm,
> if (r)
> goto out_bitmap;
>
> - if (old.dirty_bitmap && !new.dirty_bitmap)
> + if (old.dirty_bitmap && !new.dirty_bitmap) {
> + kvm->stat.generic.dirty_pages -= hweight_dirty_bitmap(&old);
> kvm_destroy_dirty_bitmap(&old);
> + }
> return 0;
>
> out_bitmap:
> @@ -1895,6 +1911,7 @@ static int kvm_get_dirty_log_protect(struct kvm *kvm, struct kvm_dirty_log *log)
> offset = i * BITS_PER_LONG;
> kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot,
> offset, mask);
> + kvm->stat.generic.dirty_pages -= hweight_long(mask);
> }
> KVM_MMU_UNLOCK(kvm);
> }
> @@ -2012,6 +2029,7 @@ static int kvm_clear_dirty_log_protect(struct kvm *kvm,
> flush = true;
> kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot,
> offset, mask);
> + kvm->stat.generic.dirty_pages -= hweight_long(mask);
> }
> }
> KVM_MMU_UNLOCK(kvm);
> @@ -3062,11 +3080,13 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
> unsigned long rel_gfn = gfn - memslot->base_gfn;
> u32 slot = (memslot->as_id << 16) | memslot->id;
>
> - if (kvm->dirty_ring_size)
> + if (kvm->dirty_ring_size) {
> kvm_dirty_ring_push(kvm_dirty_ring_get(kvm),
> slot, rel_gfn);
> - else
> + } else {
> set_bit_le(rel_gfn, memslot->dirty_bitmap);
> + ++kvm->stat.generic.dirty_pages;
Couple of problems here:
- Calls to mark_page_dirty_in_slot aren't serialized by the mmu_lock,
so these updates will race.
- The page might already be dirty in the bitmap, so you're potentially
double counting here
You can fix both of these problems by changing set_bit_le to a
test_and_set_bit_le (might not be the function name -- but you get the
idea) and conditionally incrementing dirty_pages.
> + }
> }
> }
> EXPORT_SYMBOL_GPL(mark_page_dirty_in_slot);
>
> base-commit: d0732b0f8884d9cc0eca0082bbaef043f3fef7fb
> --
> 2.32.0.605.g8dce9f2422-goog
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: stats: Add VM dirty_pages stats for the number of dirty pages
@ 2021-08-10 22:44 ` Peter Feiner
0 siblings, 0 replies; 22+ messages in thread
From: Peter Feiner @ 2021-08-10 22:44 UTC (permalink / raw)
To: Jing Zhang
Cc: KVM, KVMPPC, Paolo Bonzini, Peter Shier, Oliver Upton,
David Matlack, Ben Gardon
On Tue, Aug 10, 2021 at 3:32 PM Jing Zhang <jingzhangos@google.com> wrote:
>
> Add a generic VM stats dirty_pages to record the number of dirty pages
> reflected in dirty_bitmap at the moment.
>
> Original-by: Peter Feiner <pfeiner@google.com>
> Signed-off-by: Jing Zhang <jingzhangos@google.com>
> ---
> arch/powerpc/kvm/book3s_64_mmu_hv.c | 8 ++++++--
> arch/powerpc/kvm/book3s_64_mmu_radix.c | 1 +
> arch/powerpc/kvm/book3s_hv_rm_mmu.c | 1 +
> include/linux/kvm_host.h | 3 ++-
> include/linux/kvm_types.h | 1 +
> virt/kvm/kvm_main.c | 26 +++++++++++++++++++++++---
> 6 files changed, 34 insertions(+), 6 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> index c63e263312a4..e4aafa10efa1 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> @@ -1122,8 +1122,10 @@ long kvmppc_hv_get_dirty_log_hpt(struct kvm *kvm,
> * since we always put huge-page HPTEs in the rmap chain
> * corresponding to their page base address.
> */
> - if (npages)
> + if (npages) {
> set_dirty_bits(map, i, npages);
> + kvm->stat.generic.dirty_pages += npages;
> + }
> ++rmapp;
> }
> preempt_enable();
> @@ -1178,8 +1180,10 @@ void kvmppc_unpin_guest_page(struct kvm *kvm, void *va, unsigned long gpa,
> gfn = gpa >> PAGE_SHIFT;
> srcu_idx = srcu_read_lock(&kvm->srcu);
> memslot = gfn_to_memslot(kvm, gfn);
> - if (memslot && memslot->dirty_bitmap)
> + if (memslot && memslot->dirty_bitmap) {
> set_bit_le(gfn - memslot->base_gfn, memslot->dirty_bitmap);
> + ++kvm->stat.generic.dirty_pages;
> + }
> srcu_read_unlock(&kvm->srcu, srcu_idx);
> }
>
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> index b5905ae4377c..3a6cb3854a44 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_radix.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> @@ -1150,6 +1150,7 @@ long kvmppc_hv_get_dirty_log_radix(struct kvm *kvm,
> j = i + 1;
> if (npages) {
> set_dirty_bits(map, i, npages);
> + kvm->stat.generic.dirty_pages += npages;
> j = i + npages;
> }
> }
> diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> index 632b2545072b..16806bc473fa 100644
> --- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> +++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> @@ -109,6 +109,7 @@ void kvmppc_update_dirty_map(const struct kvm_memory_slot *memslot,
> npages = (psize + PAGE_SIZE - 1) / PAGE_SIZE;
> gfn -= memslot->base_gfn;
> set_dirty_bits_atomic(memslot->dirty_bitmap, gfn, npages);
> + kvm->stat.generic.dirty_pages += npages;
> }
> EXPORT_SYMBOL_GPL(kvmppc_update_dirty_map);
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index f50bfcf225f0..1e8e66fb915b 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1421,7 +1421,8 @@ struct _kvm_stats_desc {
> KVM_STATS_BASE_POW10, -9)
>
> #define KVM_GENERIC_VM_STATS() \
> - STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush)
> + STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush), \
> + STATS_DESC_COUNTER(VM_GENERIC, dirty_pages)
>
> #define KVM_GENERIC_VCPU_STATS() \
> STATS_DESC_COUNTER(VCPU_GENERIC, halt_successful_poll), \
> diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
> index ed6a985c5680..6c05df00aebf 100644
> --- a/include/linux/kvm_types.h
> +++ b/include/linux/kvm_types.h
> @@ -78,6 +78,7 @@ struct kvm_mmu_memory_cache {
>
> struct kvm_vm_stat_generic {
> u64 remote_tlb_flush;
> + u64 dirty_pages;
> };
>
> struct kvm_vcpu_stat_generic {
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index a438a7a3774a..93f0ca2ea326 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1228,6 +1228,19 @@ static int kvm_alloc_dirty_bitmap(struct kvm_memory_slot *memslot)
> return 0;
> }
>
> +static inline unsigned long hweight_dirty_bitmap(
> + struct kvm_memory_slot *memslot)
> +{
> + unsigned long i;
> + unsigned long count = 0;
> + unsigned long n = kvm_dirty_bitmap_bytes(memslot);
> +
> + for (i = 0; i < n / sizeof(long); ++i)
> + count += hweight_long(memslot->dirty_bitmap[i]);
> +
> + return count;
> +}
> +
> /*
> * Delete a memslot by decrementing the number of used slots and shifting all
> * other entries in the array forward one spot.
> @@ -1612,6 +1625,7 @@ static int kvm_delete_memslot(struct kvm *kvm,
> if (r)
> return r;
>
> + kvm->stat.generic.dirty_pages -= hweight_dirty_bitmap(old);
> kvm_free_memslot(kvm, old);
> return 0;
> }
> @@ -1733,8 +1747,10 @@ int __kvm_set_memory_region(struct kvm *kvm,
> if (r)
> goto out_bitmap;
>
> - if (old.dirty_bitmap && !new.dirty_bitmap)
> + if (old.dirty_bitmap && !new.dirty_bitmap) {
> + kvm->stat.generic.dirty_pages -= hweight_dirty_bitmap(&old);
> kvm_destroy_dirty_bitmap(&old);
> + }
> return 0;
>
> out_bitmap:
> @@ -1895,6 +1911,7 @@ static int kvm_get_dirty_log_protect(struct kvm *kvm, struct kvm_dirty_log *log)
> offset = i * BITS_PER_LONG;
> kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot,
> offset, mask);
> + kvm->stat.generic.dirty_pages -= hweight_long(mask);
> }
> KVM_MMU_UNLOCK(kvm);
> }
> @@ -2012,6 +2029,7 @@ static int kvm_clear_dirty_log_protect(struct kvm *kvm,
> flush = true;
> kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot,
> offset, mask);
> + kvm->stat.generic.dirty_pages -= hweight_long(mask);
> }
> }
> KVM_MMU_UNLOCK(kvm);
> @@ -3062,11 +3080,13 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
> unsigned long rel_gfn = gfn - memslot->base_gfn;
> u32 slot = (memslot->as_id << 16) | memslot->id;
>
> - if (kvm->dirty_ring_size)
> + if (kvm->dirty_ring_size) {
> kvm_dirty_ring_push(kvm_dirty_ring_get(kvm),
> slot, rel_gfn);
> - else
> + } else {
> set_bit_le(rel_gfn, memslot->dirty_bitmap);
> + ++kvm->stat.generic.dirty_pages;
Couple of problems here:
- Calls to mark_page_dirty_in_slot aren't serialized by the mmu_lock,
so these updates will race.
- The page might already be dirty in the bitmap, so you're potentially
double counting here
You can fix both of these problems by changing set_bit_le to a
test_and_set_bit_le (might not be the function name -- but you get the
idea) and conditionally incrementing dirty_pages.
> + }
> }
> }
> EXPORT_SYMBOL_GPL(mark_page_dirty_in_slot);
>
> base-commit: d0732b0f8884d9cc0eca0082bbaef043f3fef7fb
> --
> 2.32.0.605.g8dce9f2422-goog
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: stats: Add VM dirty_pages stats for the number of dirty pages
2021-08-10 22:44 ` Peter Feiner
@ 2021-08-10 22:51 ` Jing Zhang
-1 siblings, 0 replies; 22+ messages in thread
From: Jing Zhang @ 2021-08-10 22:51 UTC (permalink / raw)
To: Peter Feiner
Cc: KVM, KVMPPC, Paolo Bonzini, Peter Shier, Oliver Upton,
David Matlack, Ben Gardon
On Tue, Aug 10, 2021 at 3:45 PM Peter Feiner <pfeiner@google.com> wrote:
>
> On Tue, Aug 10, 2021 at 3:32 PM Jing Zhang <jingzhangos@google.com> wrote:
> >
> > Add a generic VM stats dirty_pages to record the number of dirty pages
> > reflected in dirty_bitmap at the moment.
> >
> > Original-by: Peter Feiner <pfeiner@google.com>
> > Signed-off-by: Jing Zhang <jingzhangos@google.com>
> > ---
> > arch/powerpc/kvm/book3s_64_mmu_hv.c | 8 ++++++--
> > arch/powerpc/kvm/book3s_64_mmu_radix.c | 1 +
> > arch/powerpc/kvm/book3s_hv_rm_mmu.c | 1 +
> > include/linux/kvm_host.h | 3 ++-
> > include/linux/kvm_types.h | 1 +
> > virt/kvm/kvm_main.c | 26 +++++++++++++++++++++++---
> > 6 files changed, 34 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > index c63e263312a4..e4aafa10efa1 100644
> > --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > @@ -1122,8 +1122,10 @@ long kvmppc_hv_get_dirty_log_hpt(struct kvm *kvm,
> > * since we always put huge-page HPTEs in the rmap chain
> > * corresponding to their page base address.
> > */
> > - if (npages)
> > + if (npages) {
> > set_dirty_bits(map, i, npages);
> > + kvm->stat.generic.dirty_pages += npages;
> > + }
> > ++rmapp;
> > }
> > preempt_enable();
> > @@ -1178,8 +1180,10 @@ void kvmppc_unpin_guest_page(struct kvm *kvm, void *va, unsigned long gpa,
> > gfn = gpa >> PAGE_SHIFT;
> > srcu_idx = srcu_read_lock(&kvm->srcu);
> > memslot = gfn_to_memslot(kvm, gfn);
> > - if (memslot && memslot->dirty_bitmap)
> > + if (memslot && memslot->dirty_bitmap) {
> > set_bit_le(gfn - memslot->base_gfn, memslot->dirty_bitmap);
> > + ++kvm->stat.generic.dirty_pages;
> > + }
> > srcu_read_unlock(&kvm->srcu, srcu_idx);
> > }
> >
> > diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > index b5905ae4377c..3a6cb3854a44 100644
> > --- a/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > +++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > @@ -1150,6 +1150,7 @@ long kvmppc_hv_get_dirty_log_radix(struct kvm *kvm,
> > j = i + 1;
> > if (npages) {
> > set_dirty_bits(map, i, npages);
> > + kvm->stat.generic.dirty_pages += npages;
> > j = i + npages;
> > }
> > }
> > diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > index 632b2545072b..16806bc473fa 100644
> > --- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > +++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > @@ -109,6 +109,7 @@ void kvmppc_update_dirty_map(const struct kvm_memory_slot *memslot,
> > npages = (psize + PAGE_SIZE - 1) / PAGE_SIZE;
> > gfn -= memslot->base_gfn;
> > set_dirty_bits_atomic(memslot->dirty_bitmap, gfn, npages);
> > + kvm->stat.generic.dirty_pages += npages;
> > }
> > EXPORT_SYMBOL_GPL(kvmppc_update_dirty_map);
> >
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index f50bfcf225f0..1e8e66fb915b 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -1421,7 +1421,8 @@ struct _kvm_stats_desc {
> > KVM_STATS_BASE_POW10, -9)
> >
> > #define KVM_GENERIC_VM_STATS() \
> > - STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush)
> > + STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush), \
> > + STATS_DESC_COUNTER(VM_GENERIC, dirty_pages)
> >
> > #define KVM_GENERIC_VCPU_STATS() \
> > STATS_DESC_COUNTER(VCPU_GENERIC, halt_successful_poll), \
> > diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
> > index ed6a985c5680..6c05df00aebf 100644
> > --- a/include/linux/kvm_types.h
> > +++ b/include/linux/kvm_types.h
> > @@ -78,6 +78,7 @@ struct kvm_mmu_memory_cache {
> >
> > struct kvm_vm_stat_generic {
> > u64 remote_tlb_flush;
> > + u64 dirty_pages;
> > };
> >
> > struct kvm_vcpu_stat_generic {
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index a438a7a3774a..93f0ca2ea326 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -1228,6 +1228,19 @@ static int kvm_alloc_dirty_bitmap(struct kvm_memory_slot *memslot)
> > return 0;
> > }
> >
> > +static inline unsigned long hweight_dirty_bitmap(
> > + struct kvm_memory_slot *memslot)
> > +{
> > + unsigned long i;
> > + unsigned long count = 0;
> > + unsigned long n = kvm_dirty_bitmap_bytes(memslot);
> > +
> > + for (i = 0; i < n / sizeof(long); ++i)
> > + count += hweight_long(memslot->dirty_bitmap[i]);
> > +
> > + return count;
> > +}
> > +
> > /*
> > * Delete a memslot by decrementing the number of used slots and shifting all
> > * other entries in the array forward one spot.
> > @@ -1612,6 +1625,7 @@ static int kvm_delete_memslot(struct kvm *kvm,
> > if (r)
> > return r;
> >
> > + kvm->stat.generic.dirty_pages -= hweight_dirty_bitmap(old);
> > kvm_free_memslot(kvm, old);
> > return 0;
> > }
> > @@ -1733,8 +1747,10 @@ int __kvm_set_memory_region(struct kvm *kvm,
> > if (r)
> > goto out_bitmap;
> >
> > - if (old.dirty_bitmap && !new.dirty_bitmap)
> > + if (old.dirty_bitmap && !new.dirty_bitmap) {
> > + kvm->stat.generic.dirty_pages -= hweight_dirty_bitmap(&old);
> > kvm_destroy_dirty_bitmap(&old);
> > + }
> > return 0;
> >
> > out_bitmap:
> > @@ -1895,6 +1911,7 @@ static int kvm_get_dirty_log_protect(struct kvm *kvm, struct kvm_dirty_log *log)
> > offset = i * BITS_PER_LONG;
> > kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot,
> > offset, mask);
> > + kvm->stat.generic.dirty_pages -= hweight_long(mask);
> > }
> > KVM_MMU_UNLOCK(kvm);
> > }
> > @@ -2012,6 +2029,7 @@ static int kvm_clear_dirty_log_protect(struct kvm *kvm,
> > flush = true;
> > kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot,
> > offset, mask);
> > + kvm->stat.generic.dirty_pages -= hweight_long(mask);
> > }
> > }
> > KVM_MMU_UNLOCK(kvm);
> > @@ -3062,11 +3080,13 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
> > unsigned long rel_gfn = gfn - memslot->base_gfn;
> > u32 slot = (memslot->as_id << 16) | memslot->id;
> >
> > - if (kvm->dirty_ring_size)
> > + if (kvm->dirty_ring_size) {
> > kvm_dirty_ring_push(kvm_dirty_ring_get(kvm),
> > slot, rel_gfn);
> > - else
> > + } else {
> > set_bit_le(rel_gfn, memslot->dirty_bitmap);
> > + ++kvm->stat.generic.dirty_pages;
>
> Couple of problems here:
>
> - Calls to mark_page_dirty_in_slot aren't serialized by the mmu_lock,
> so these updates will race.
> - The page might already be dirty in the bitmap, so you're potentially
> double counting here
>
> You can fix both of these problems by changing set_bit_le to a
> test_and_set_bit_le (might not be the function name -- but you get the
> idea) and conditionally incrementing dirty_pages.
Thanks Peter, will do that.
>
>
> > + }
> > }
> > }
> > EXPORT_SYMBOL_GPL(mark_page_dirty_in_slot);
> >
> > base-commit: d0732b0f8884d9cc0eca0082bbaef043f3fef7fb
> > --
> > 2.32.0.605.g8dce9f2422-goog
> >
Jing
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: stats: Add VM dirty_pages stats for the number of dirty pages
@ 2021-08-10 22:51 ` Jing Zhang
0 siblings, 0 replies; 22+ messages in thread
From: Jing Zhang @ 2021-08-10 22:51 UTC (permalink / raw)
To: Peter Feiner
Cc: KVM, KVMPPC, Paolo Bonzini, Peter Shier, Oliver Upton,
David Matlack, Ben Gardon
On Tue, Aug 10, 2021 at 3:45 PM Peter Feiner <pfeiner@google.com> wrote:
>
> On Tue, Aug 10, 2021 at 3:32 PM Jing Zhang <jingzhangos@google.com> wrote:
> >
> > Add a generic VM stats dirty_pages to record the number of dirty pages
> > reflected in dirty_bitmap at the moment.
> >
> > Original-by: Peter Feiner <pfeiner@google.com>
> > Signed-off-by: Jing Zhang <jingzhangos@google.com>
> > ---
> > arch/powerpc/kvm/book3s_64_mmu_hv.c | 8 ++++++--
> > arch/powerpc/kvm/book3s_64_mmu_radix.c | 1 +
> > arch/powerpc/kvm/book3s_hv_rm_mmu.c | 1 +
> > include/linux/kvm_host.h | 3 ++-
> > include/linux/kvm_types.h | 1 +
> > virt/kvm/kvm_main.c | 26 +++++++++++++++++++++++---
> > 6 files changed, 34 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > index c63e263312a4..e4aafa10efa1 100644
> > --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > @@ -1122,8 +1122,10 @@ long kvmppc_hv_get_dirty_log_hpt(struct kvm *kvm,
> > * since we always put huge-page HPTEs in the rmap chain
> > * corresponding to their page base address.
> > */
> > - if (npages)
> > + if (npages) {
> > set_dirty_bits(map, i, npages);
> > + kvm->stat.generic.dirty_pages += npages;
> > + }
> > ++rmapp;
> > }
> > preempt_enable();
> > @@ -1178,8 +1180,10 @@ void kvmppc_unpin_guest_page(struct kvm *kvm, void *va, unsigned long gpa,
> > gfn = gpa >> PAGE_SHIFT;
> > srcu_idx = srcu_read_lock(&kvm->srcu);
> > memslot = gfn_to_memslot(kvm, gfn);
> > - if (memslot && memslot->dirty_bitmap)
> > + if (memslot && memslot->dirty_bitmap) {
> > set_bit_le(gfn - memslot->base_gfn, memslot->dirty_bitmap);
> > + ++kvm->stat.generic.dirty_pages;
> > + }
> > srcu_read_unlock(&kvm->srcu, srcu_idx);
> > }
> >
> > diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > index b5905ae4377c..3a6cb3854a44 100644
> > --- a/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > +++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > @@ -1150,6 +1150,7 @@ long kvmppc_hv_get_dirty_log_radix(struct kvm *kvm,
> > j = i + 1;
> > if (npages) {
> > set_dirty_bits(map, i, npages);
> > + kvm->stat.generic.dirty_pages += npages;
> > j = i + npages;
> > }
> > }
> > diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > index 632b2545072b..16806bc473fa 100644
> > --- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > +++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > @@ -109,6 +109,7 @@ void kvmppc_update_dirty_map(const struct kvm_memory_slot *memslot,
> > npages = (psize + PAGE_SIZE - 1) / PAGE_SIZE;
> > gfn -= memslot->base_gfn;
> > set_dirty_bits_atomic(memslot->dirty_bitmap, gfn, npages);
> > + kvm->stat.generic.dirty_pages += npages;
> > }
> > EXPORT_SYMBOL_GPL(kvmppc_update_dirty_map);
> >
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index f50bfcf225f0..1e8e66fb915b 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -1421,7 +1421,8 @@ struct _kvm_stats_desc {
> > KVM_STATS_BASE_POW10, -9)
> >
> > #define KVM_GENERIC_VM_STATS() \
> > - STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush)
> > + STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush), \
> > + STATS_DESC_COUNTER(VM_GENERIC, dirty_pages)
> >
> > #define KVM_GENERIC_VCPU_STATS() \
> > STATS_DESC_COUNTER(VCPU_GENERIC, halt_successful_poll), \
> > diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
> > index ed6a985c5680..6c05df00aebf 100644
> > --- a/include/linux/kvm_types.h
> > +++ b/include/linux/kvm_types.h
> > @@ -78,6 +78,7 @@ struct kvm_mmu_memory_cache {
> >
> > struct kvm_vm_stat_generic {
> > u64 remote_tlb_flush;
> > + u64 dirty_pages;
> > };
> >
> > struct kvm_vcpu_stat_generic {
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index a438a7a3774a..93f0ca2ea326 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -1228,6 +1228,19 @@ static int kvm_alloc_dirty_bitmap(struct kvm_memory_slot *memslot)
> > return 0;
> > }
> >
> > +static inline unsigned long hweight_dirty_bitmap(
> > + struct kvm_memory_slot *memslot)
> > +{
> > + unsigned long i;
> > + unsigned long count = 0;
> > + unsigned long n = kvm_dirty_bitmap_bytes(memslot);
> > +
> > + for (i = 0; i < n / sizeof(long); ++i)
> > + count += hweight_long(memslot->dirty_bitmap[i]);
> > +
> > + return count;
> > +}
> > +
> > /*
> > * Delete a memslot by decrementing the number of used slots and shifting all
> > * other entries in the array forward one spot.
> > @@ -1612,6 +1625,7 @@ static int kvm_delete_memslot(struct kvm *kvm,
> > if (r)
> > return r;
> >
> > + kvm->stat.generic.dirty_pages -= hweight_dirty_bitmap(old);
> > kvm_free_memslot(kvm, old);
> > return 0;
> > }
> > @@ -1733,8 +1747,10 @@ int __kvm_set_memory_region(struct kvm *kvm,
> > if (r)
> > goto out_bitmap;
> >
> > - if (old.dirty_bitmap && !new.dirty_bitmap)
> > + if (old.dirty_bitmap && !new.dirty_bitmap) {
> > + kvm->stat.generic.dirty_pages -= hweight_dirty_bitmap(&old);
> > kvm_destroy_dirty_bitmap(&old);
> > + }
> > return 0;
> >
> > out_bitmap:
> > @@ -1895,6 +1911,7 @@ static int kvm_get_dirty_log_protect(struct kvm *kvm, struct kvm_dirty_log *log)
> > offset = i * BITS_PER_LONG;
> > kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot,
> > offset, mask);
> > + kvm->stat.generic.dirty_pages -= hweight_long(mask);
> > }
> > KVM_MMU_UNLOCK(kvm);
> > }
> > @@ -2012,6 +2029,7 @@ static int kvm_clear_dirty_log_protect(struct kvm *kvm,
> > flush = true;
> > kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot,
> > offset, mask);
> > + kvm->stat.generic.dirty_pages -= hweight_long(mask);
> > }
> > }
> > KVM_MMU_UNLOCK(kvm);
> > @@ -3062,11 +3080,13 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
> > unsigned long rel_gfn = gfn - memslot->base_gfn;
> > u32 slot = (memslot->as_id << 16) | memslot->id;
> >
> > - if (kvm->dirty_ring_size)
> > + if (kvm->dirty_ring_size) {
> > kvm_dirty_ring_push(kvm_dirty_ring_get(kvm),
> > slot, rel_gfn);
> > - else
> > + } else {
> > set_bit_le(rel_gfn, memslot->dirty_bitmap);
> > + ++kvm->stat.generic.dirty_pages;
>
> Couple of problems here:
>
> - Calls to mark_page_dirty_in_slot aren't serialized by the mmu_lock,
> so these updates will race.
> - The page might already be dirty in the bitmap, so you're potentially
> double counting here
>
> You can fix both of these problems by changing set_bit_le to a
> test_and_set_bit_le (might not be the function name -- but you get the
> idea) and conditionally incrementing dirty_pages.
Thanks Peter, will do that.
>
>
> > + }
> > }
> > }
> > EXPORT_SYMBOL_GPL(mark_page_dirty_in_slot);
> >
> > base-commit: d0732b0f8884d9cc0eca0082bbaef043f3fef7fb
> > --
> > 2.32.0.605.g8dce9f2422-goog
> >
Jing
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: stats: Add VM dirty_pages stats for the number of dirty pages
2021-08-10 22:32 [PATCH] KVM: stats: Add VM dirty_pages stats for the number of dirty pages Jing Zhang
@ 2021-08-10 22:56 ` Oliver Upton
2021-08-10 22:56 ` Oliver Upton
2021-08-11 12:26 ` kernel test robot
2 siblings, 0 replies; 22+ messages in thread
From: Oliver Upton @ 2021-08-10 22:56 UTC (permalink / raw)
To: Jing Zhang
Cc: KVM, KVMPPC, Paolo Bonzini, Peter Shier, David Matlack,
Ben Gardon, Peter Feiner
Hi Jing,
On Tue, Aug 10, 2021 at 3:32 PM Jing Zhang <jingzhangos@google.com> wrote:
>
> Add a generic VM stats dirty_pages to record the number of dirty pages
> reflected in dirty_bitmap at the moment.
There can be multiple dirty bitmaps in a VM, one per memslot.
> Original-by: Peter Feiner <pfeiner@google.com>
> Signed-off-by: Jing Zhang <jingzhangos@google.com>
> ---
> arch/powerpc/kvm/book3s_64_mmu_hv.c | 8 ++++++--
> arch/powerpc/kvm/book3s_64_mmu_radix.c | 1 +
> arch/powerpc/kvm/book3s_hv_rm_mmu.c | 1 +
> include/linux/kvm_host.h | 3 ++-
> include/linux/kvm_types.h | 1 +
> virt/kvm/kvm_main.c | 26 +++++++++++++++++++++++---
> 6 files changed, 34 insertions(+), 6 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> index c63e263312a4..e4aafa10efa1 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> @@ -1122,8 +1122,10 @@ long kvmppc_hv_get_dirty_log_hpt(struct kvm *kvm,
> * since we always put huge-page HPTEs in the rmap chain
> * corresponding to their page base address.
> */
> - if (npages)
> + if (npages) {
> set_dirty_bits(map, i, npages);
> + kvm->stat.generic.dirty_pages += npages;
> + }
> ++rmapp;
> }
> preempt_enable();
> @@ -1178,8 +1180,10 @@ void kvmppc_unpin_guest_page(struct kvm *kvm, void *va, unsigned long gpa,
> gfn = gpa >> PAGE_SHIFT;
> srcu_idx = srcu_read_lock(&kvm->srcu);
> memslot = gfn_to_memslot(kvm, gfn);
> - if (memslot && memslot->dirty_bitmap)
> + if (memslot && memslot->dirty_bitmap) {
> set_bit_le(gfn - memslot->base_gfn, memslot->dirty_bitmap);
> + ++kvm->stat.generic.dirty_pages;
> + }
> srcu_read_unlock(&kvm->srcu, srcu_idx);
> }
>
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> index b5905ae4377c..3a6cb3854a44 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_radix.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> @@ -1150,6 +1150,7 @@ long kvmppc_hv_get_dirty_log_radix(struct kvm *kvm,
> j = i + 1;
> if (npages) {
> set_dirty_bits(map, i, npages);
> + kvm->stat.generic.dirty_pages += npages;
> j = i + npages;
> }
> }
> diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> index 632b2545072b..16806bc473fa 100644
> --- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> +++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> @@ -109,6 +109,7 @@ void kvmppc_update_dirty_map(const struct kvm_memory_slot *memslot,
> npages = (psize + PAGE_SIZE - 1) / PAGE_SIZE;
> gfn -= memslot->base_gfn;
> set_dirty_bits_atomic(memslot->dirty_bitmap, gfn, npages);
> + kvm->stat.generic.dirty_pages += npages;
> }
> EXPORT_SYMBOL_GPL(kvmppc_update_dirty_map);
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index f50bfcf225f0..1e8e66fb915b 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1421,7 +1421,8 @@ struct _kvm_stats_desc {
> KVM_STATS_BASE_POW10, -9)
>
> #define KVM_GENERIC_VM_STATS() \
> - STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush)
> + STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush), \
> + STATS_DESC_COUNTER(VM_GENERIC, dirty_pages)
>
> #define KVM_GENERIC_VCPU_STATS() \
> STATS_DESC_COUNTER(VCPU_GENERIC, halt_successful_poll), \
> diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
> index ed6a985c5680..6c05df00aebf 100644
> --- a/include/linux/kvm_types.h
> +++ b/include/linux/kvm_types.h
> @@ -78,6 +78,7 @@ struct kvm_mmu_memory_cache {
>
> struct kvm_vm_stat_generic {
> u64 remote_tlb_flush;
> + u64 dirty_pages;
> };
>
> struct kvm_vcpu_stat_generic {
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index a438a7a3774a..93f0ca2ea326 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1228,6 +1228,19 @@ static int kvm_alloc_dirty_bitmap(struct kvm_memory_slot *memslot)
> return 0;
> }
>
> +static inline unsigned long hweight_dirty_bitmap(
> + struct kvm_memory_slot *memslot)
> +{
> + unsigned long i;
> + unsigned long count = 0;
> + unsigned long n = kvm_dirty_bitmap_bytes(memslot);
> +
> + for (i = 0; i < n / sizeof(long); ++i)
> + count += hweight_long(memslot->dirty_bitmap[i]);
> +
> + return count;
> +}
Hrm, this seems like a decent amount of work for a statistic.
> +
> /*
> * Delete a memslot by decrementing the number of used slots and shifting all
> * other entries in the array forward one spot.
> @@ -1612,6 +1625,7 @@ static int kvm_delete_memslot(struct kvm *kvm,
> if (r)
> return r;
>
> + kvm->stat.generic.dirty_pages -= hweight_dirty_bitmap(old);
> kvm_free_memslot(kvm, old);
> return 0;
> }
> @@ -1733,8 +1747,10 @@ int __kvm_set_memory_region(struct kvm *kvm,
> if (r)
> goto out_bitmap;
>
> - if (old.dirty_bitmap && !new.dirty_bitmap)
> + if (old.dirty_bitmap && !new.dirty_bitmap) {
> + kvm->stat.generic.dirty_pages -= hweight_dirty_bitmap(&old);
> kvm_destroy_dirty_bitmap(&old);
> + }
Races to increment by a few pages might be OK, so long as imprecision
is acceptable, but decrementing by an entire bitmap could cause the
stat to get waaay off from the state of the VM.
What if the statistic was 'dirtied_pages', which records the number of
pages dirtied in the lifetime of a VM? Userspace could just record the
value each time it blows away the dirty bitmaps and subtract that
value next time it reads the stat. It would circumvent the need to
walk the entire dirty bitmap to keep the statistic sane.
> return 0;
>
> out_bitmap:
> @@ -1895,6 +1911,7 @@ static int kvm_get_dirty_log_protect(struct kvm *kvm, struct kvm_dirty_log *log)
> offset = i * BITS_PER_LONG;
> kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot,
> offset, mask);
> + kvm->stat.generic.dirty_pages -= hweight_long(mask);
> }
> KVM_MMU_UNLOCK(kvm);
> }
> @@ -2012,6 +2029,7 @@ static int kvm_clear_dirty_log_protect(struct kvm *kvm,
> flush = true;
> kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot,
> offset, mask);
> + kvm->stat.generic.dirty_pages -= hweight_long(mask);
> }
> }
> KVM_MMU_UNLOCK(kvm);
> @@ -3062,11 +3080,13 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
> unsigned long rel_gfn = gfn - memslot->base_gfn;
> u32 slot = (memslot->as_id << 16) | memslot->id;
>
> - if (kvm->dirty_ring_size)
> + if (kvm->dirty_ring_size) {
> kvm_dirty_ring_push(kvm_dirty_ring_get(kvm),
> slot, rel_gfn);
> - else
> + } else {
> set_bit_le(rel_gfn, memslot->dirty_bitmap);
> + ++kvm->stat.generic.dirty_pages;
> + }
Aren't pages being pushed out to the dirty ring just as dirty? :-)
> }
> }
> EXPORT_SYMBOL_GPL(mark_page_dirty_in_slot);
>
> base-commit: d0732b0f8884d9cc0eca0082bbaef043f3fef7fb
> --
> 2.32.0.605.g8dce9f2422-goog
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: stats: Add VM dirty_pages stats for the number of dirty pages
@ 2021-08-10 22:56 ` Oliver Upton
0 siblings, 0 replies; 22+ messages in thread
From: Oliver Upton @ 2021-08-10 22:56 UTC (permalink / raw)
To: Jing Zhang
Cc: KVM, KVMPPC, Paolo Bonzini, Peter Shier, David Matlack,
Ben Gardon, Peter Feiner
Hi Jing,
On Tue, Aug 10, 2021 at 3:32 PM Jing Zhang <jingzhangos@google.com> wrote:
>
> Add a generic VM stats dirty_pages to record the number of dirty pages
> reflected in dirty_bitmap at the moment.
There can be multiple dirty bitmaps in a VM, one per memslot.
> Original-by: Peter Feiner <pfeiner@google.com>
> Signed-off-by: Jing Zhang <jingzhangos@google.com>
> ---
> arch/powerpc/kvm/book3s_64_mmu_hv.c | 8 ++++++--
> arch/powerpc/kvm/book3s_64_mmu_radix.c | 1 +
> arch/powerpc/kvm/book3s_hv_rm_mmu.c | 1 +
> include/linux/kvm_host.h | 3 ++-
> include/linux/kvm_types.h | 1 +
> virt/kvm/kvm_main.c | 26 +++++++++++++++++++++++---
> 6 files changed, 34 insertions(+), 6 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> index c63e263312a4..e4aafa10efa1 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> @@ -1122,8 +1122,10 @@ long kvmppc_hv_get_dirty_log_hpt(struct kvm *kvm,
> * since we always put huge-page HPTEs in the rmap chain
> * corresponding to their page base address.
> */
> - if (npages)
> + if (npages) {
> set_dirty_bits(map, i, npages);
> + kvm->stat.generic.dirty_pages += npages;
> + }
> ++rmapp;
> }
> preempt_enable();
> @@ -1178,8 +1180,10 @@ void kvmppc_unpin_guest_page(struct kvm *kvm, void *va, unsigned long gpa,
> gfn = gpa >> PAGE_SHIFT;
> srcu_idx = srcu_read_lock(&kvm->srcu);
> memslot = gfn_to_memslot(kvm, gfn);
> - if (memslot && memslot->dirty_bitmap)
> + if (memslot && memslot->dirty_bitmap) {
> set_bit_le(gfn - memslot->base_gfn, memslot->dirty_bitmap);
> + ++kvm->stat.generic.dirty_pages;
> + }
> srcu_read_unlock(&kvm->srcu, srcu_idx);
> }
>
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> index b5905ae4377c..3a6cb3854a44 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_radix.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> @@ -1150,6 +1150,7 @@ long kvmppc_hv_get_dirty_log_radix(struct kvm *kvm,
> j = i + 1;
> if (npages) {
> set_dirty_bits(map, i, npages);
> + kvm->stat.generic.dirty_pages += npages;
> j = i + npages;
> }
> }
> diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> index 632b2545072b..16806bc473fa 100644
> --- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> +++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> @@ -109,6 +109,7 @@ void kvmppc_update_dirty_map(const struct kvm_memory_slot *memslot,
> npages = (psize + PAGE_SIZE - 1) / PAGE_SIZE;
> gfn -= memslot->base_gfn;
> set_dirty_bits_atomic(memslot->dirty_bitmap, gfn, npages);
> + kvm->stat.generic.dirty_pages += npages;
> }
> EXPORT_SYMBOL_GPL(kvmppc_update_dirty_map);
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index f50bfcf225f0..1e8e66fb915b 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1421,7 +1421,8 @@ struct _kvm_stats_desc {
> KVM_STATS_BASE_POW10, -9)
>
> #define KVM_GENERIC_VM_STATS() \
> - STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush)
> + STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush), \
> + STATS_DESC_COUNTER(VM_GENERIC, dirty_pages)
>
> #define KVM_GENERIC_VCPU_STATS() \
> STATS_DESC_COUNTER(VCPU_GENERIC, halt_successful_poll), \
> diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
> index ed6a985c5680..6c05df00aebf 100644
> --- a/include/linux/kvm_types.h
> +++ b/include/linux/kvm_types.h
> @@ -78,6 +78,7 @@ struct kvm_mmu_memory_cache {
>
> struct kvm_vm_stat_generic {
> u64 remote_tlb_flush;
> + u64 dirty_pages;
> };
>
> struct kvm_vcpu_stat_generic {
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index a438a7a3774a..93f0ca2ea326 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1228,6 +1228,19 @@ static int kvm_alloc_dirty_bitmap(struct kvm_memory_slot *memslot)
> return 0;
> }
>
> +static inline unsigned long hweight_dirty_bitmap(
> + struct kvm_memory_slot *memslot)
> +{
> + unsigned long i;
> + unsigned long count = 0;
> + unsigned long n = kvm_dirty_bitmap_bytes(memslot);
> +
> + for (i = 0; i < n / sizeof(long); ++i)
> + count += hweight_long(memslot->dirty_bitmap[i]);
> +
> + return count;
> +}
Hrm, this seems like a decent amount of work for a statistic.
> +
> /*
> * Delete a memslot by decrementing the number of used slots and shifting all
> * other entries in the array forward one spot.
> @@ -1612,6 +1625,7 @@ static int kvm_delete_memslot(struct kvm *kvm,
> if (r)
> return r;
>
> + kvm->stat.generic.dirty_pages -= hweight_dirty_bitmap(old);
> kvm_free_memslot(kvm, old);
> return 0;
> }
> @@ -1733,8 +1747,10 @@ int __kvm_set_memory_region(struct kvm *kvm,
> if (r)
> goto out_bitmap;
>
> - if (old.dirty_bitmap && !new.dirty_bitmap)
> + if (old.dirty_bitmap && !new.dirty_bitmap) {
> + kvm->stat.generic.dirty_pages -= hweight_dirty_bitmap(&old);
> kvm_destroy_dirty_bitmap(&old);
> + }
Races to increment by a few pages might be OK, so long as imprecision
is acceptable, but decrementing by an entire bitmap could cause the
stat to get waaay off from the state of the VM.
What if the statistic was 'dirtied_pages', which records the number of
pages dirtied in the lifetime of a VM? Userspace could just record the
value each time it blows away the dirty bitmaps and subtract that
value next time it reads the stat. It would circumvent the need to
walk the entire dirty bitmap to keep the statistic sane.
> return 0;
>
> out_bitmap:
> @@ -1895,6 +1911,7 @@ static int kvm_get_dirty_log_protect(struct kvm *kvm, struct kvm_dirty_log *log)
> offset = i * BITS_PER_LONG;
> kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot,
> offset, mask);
> + kvm->stat.generic.dirty_pages -= hweight_long(mask);
> }
> KVM_MMU_UNLOCK(kvm);
> }
> @@ -2012,6 +2029,7 @@ static int kvm_clear_dirty_log_protect(struct kvm *kvm,
> flush = true;
> kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot,
> offset, mask);
> + kvm->stat.generic.dirty_pages -= hweight_long(mask);
> }
> }
> KVM_MMU_UNLOCK(kvm);
> @@ -3062,11 +3080,13 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
> unsigned long rel_gfn = gfn - memslot->base_gfn;
> u32 slot = (memslot->as_id << 16) | memslot->id;
>
> - if (kvm->dirty_ring_size)
> + if (kvm->dirty_ring_size) {
> kvm_dirty_ring_push(kvm_dirty_ring_get(kvm),
> slot, rel_gfn);
> - else
> + } else {
> set_bit_le(rel_gfn, memslot->dirty_bitmap);
> + ++kvm->stat.generic.dirty_pages;
> + }
Aren't pages being pushed out to the dirty ring just as dirty? :-)
> }
> }
> EXPORT_SYMBOL_GPL(mark_page_dirty_in_slot);
>
> base-commit: d0732b0f8884d9cc0eca0082bbaef043f3fef7fb
> --
> 2.32.0.605.g8dce9f2422-goog
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: stats: Add VM dirty_pages stats for the number of dirty pages
2021-08-10 22:56 ` Oliver Upton
@ 2021-08-10 23:15 ` Peter Feiner
-1 siblings, 0 replies; 22+ messages in thread
From: Peter Feiner @ 2021-08-10 23:15 UTC (permalink / raw)
To: Oliver Upton
Cc: Jing Zhang, KVM, KVMPPC, Paolo Bonzini, Peter Shier,
David Matlack, Ben Gardon
On Tue, Aug 10, 2021 at 3:56 PM Oliver Upton <oupton@google.com> wrote:
>
> Hi Jing,
>
> On Tue, Aug 10, 2021 at 3:32 PM Jing Zhang <jingzhangos@google.com> wrote:
> >
> > Add a generic VM stats dirty_pages to record the number of dirty pages
> > reflected in dirty_bitmap at the moment.
>
> There can be multiple dirty bitmaps in a VM, one per memslot.
>
> > Original-by: Peter Feiner <pfeiner@google.com>
> > Signed-off-by: Jing Zhang <jingzhangos@google.com>
> > ---
> > arch/powerpc/kvm/book3s_64_mmu_hv.c | 8 ++++++--
> > arch/powerpc/kvm/book3s_64_mmu_radix.c | 1 +
> > arch/powerpc/kvm/book3s_hv_rm_mmu.c | 1 +
> > include/linux/kvm_host.h | 3 ++-
> > include/linux/kvm_types.h | 1 +
> > virt/kvm/kvm_main.c | 26 +++++++++++++++++++++++---
> > 6 files changed, 34 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > index c63e263312a4..e4aafa10efa1 100644
> > --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > @@ -1122,8 +1122,10 @@ long kvmppc_hv_get_dirty_log_hpt(struct kvm *kvm,
> > * since we always put huge-page HPTEs in the rmap chain
> > * corresponding to their page base address.
> > */
> > - if (npages)
> > + if (npages) {
> > set_dirty_bits(map, i, npages);
> > + kvm->stat.generic.dirty_pages += npages;
> > + }
> > ++rmapp;
> > }
> > preempt_enable();
> > @@ -1178,8 +1180,10 @@ void kvmppc_unpin_guest_page(struct kvm *kvm, void *va, unsigned long gpa,
> > gfn = gpa >> PAGE_SHIFT;
> > srcu_idx = srcu_read_lock(&kvm->srcu);
> > memslot = gfn_to_memslot(kvm, gfn);
> > - if (memslot && memslot->dirty_bitmap)
> > + if (memslot && memslot->dirty_bitmap) {
> > set_bit_le(gfn - memslot->base_gfn, memslot->dirty_bitmap);
> > + ++kvm->stat.generic.dirty_pages;
> > + }
> > srcu_read_unlock(&kvm->srcu, srcu_idx);
> > }
> >
> > diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > index b5905ae4377c..3a6cb3854a44 100644
> > --- a/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > +++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > @@ -1150,6 +1150,7 @@ long kvmppc_hv_get_dirty_log_radix(struct kvm *kvm,
> > j = i + 1;
> > if (npages) {
> > set_dirty_bits(map, i, npages);
> > + kvm->stat.generic.dirty_pages += npages;
> > j = i + npages;
> > }
> > }
> > diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > index 632b2545072b..16806bc473fa 100644
> > --- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > +++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > @@ -109,6 +109,7 @@ void kvmppc_update_dirty_map(const struct kvm_memory_slot *memslot,
> > npages = (psize + PAGE_SIZE - 1) / PAGE_SIZE;
> > gfn -= memslot->base_gfn;
> > set_dirty_bits_atomic(memslot->dirty_bitmap, gfn, npages);
> > + kvm->stat.generic.dirty_pages += npages;
> > }
> > EXPORT_SYMBOL_GPL(kvmppc_update_dirty_map);
> >
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index f50bfcf225f0..1e8e66fb915b 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -1421,7 +1421,8 @@ struct _kvm_stats_desc {
> > KVM_STATS_BASE_POW10, -9)
> >
> > #define KVM_GENERIC_VM_STATS() \
> > - STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush)
> > + STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush), \
> > + STATS_DESC_COUNTER(VM_GENERIC, dirty_pages)
> >
> > #define KVM_GENERIC_VCPU_STATS() \
> > STATS_DESC_COUNTER(VCPU_GENERIC, halt_successful_poll), \
> > diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
> > index ed6a985c5680..6c05df00aebf 100644
> > --- a/include/linux/kvm_types.h
> > +++ b/include/linux/kvm_types.h
> > @@ -78,6 +78,7 @@ struct kvm_mmu_memory_cache {
> >
> > struct kvm_vm_stat_generic {
> > u64 remote_tlb_flush;
> > + u64 dirty_pages;
> > };
> >
> > struct kvm_vcpu_stat_generic {
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index a438a7a3774a..93f0ca2ea326 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -1228,6 +1228,19 @@ static int kvm_alloc_dirty_bitmap(struct kvm_memory_slot *memslot)
> > return 0;
> > }
> >
> > +static inline unsigned long hweight_dirty_bitmap(
> > + struct kvm_memory_slot *memslot)
> > +{
> > + unsigned long i;
> > + unsigned long count = 0;
> > + unsigned long n = kvm_dirty_bitmap_bytes(memslot);
> > +
> > + for (i = 0; i < n / sizeof(long); ++i)
> > + count += hweight_long(memslot->dirty_bitmap[i]);
> > +
> > + return count;
> > +}
>
> Hrm, this seems like a decent amount of work for a statistic.
>
> > +
> > /*
> > * Delete a memslot by decrementing the number of used slots and shifting all
> > * other entries in the array forward one spot.
> > @@ -1612,6 +1625,7 @@ static int kvm_delete_memslot(struct kvm *kvm,
> > if (r)
> > return r;
> >
> > + kvm->stat.generic.dirty_pages -= hweight_dirty_bitmap(old);
> > kvm_free_memslot(kvm, old);
> > return 0;
> > }
> > @@ -1733,8 +1747,10 @@ int __kvm_set_memory_region(struct kvm *kvm,
> > if (r)
> > goto out_bitmap;
> >
> > - if (old.dirty_bitmap && !new.dirty_bitmap)
> > + if (old.dirty_bitmap && !new.dirty_bitmap) {
> > + kvm->stat.generic.dirty_pages -= hweight_dirty_bitmap(&old);
> > kvm_destroy_dirty_bitmap(&old);
> > + }
>
> Races to increment by a few pages might be OK, so long as imprecision
> is acceptable, but decrementing by an entire bitmap could cause the
> stat to get waaay off from the state of the VM.
My original use case was to know the rate at which memory was being
dirtied to predict how expensive a live migration would be. I didn't
need full precision, but I would have needed a bound on the slop. A
"few pages" isn't a bound :-)
IMO, this patch isn't worth figuring out without a use case. It's
complex and has perf overhead. Maybe just drop it?
>
> What if the statistic was 'dirtied_pages', which records the number of
> pages dirtied in the lifetime of a VM? Userspace could just record the
> value each time it blows away the dirty bitmaps and subtract that
> value next time it reads the stat. It would circumvent the need to
> walk the entire dirty bitmap to keep the statistic sane.
>
> > return 0;
> >
> > out_bitmap:
> > @@ -1895,6 +1911,7 @@ static int kvm_get_dirty_log_protect(struct kvm *kvm, struct kvm_dirty_log *log)
> > offset = i * BITS_PER_LONG;
> > kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot,
> > offset, mask);
> > + kvm->stat.generic.dirty_pages -= hweight_long(mask);
> > }
> > KVM_MMU_UNLOCK(kvm);
> > }
> > @@ -2012,6 +2029,7 @@ static int kvm_clear_dirty_log_protect(struct kvm *kvm,
> > flush = true;
> > kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot,
> > offset, mask);
> > + kvm->stat.generic.dirty_pages -= hweight_long(mask);
> > }
> > }
> > KVM_MMU_UNLOCK(kvm);
> > @@ -3062,11 +3080,13 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
> > unsigned long rel_gfn = gfn - memslot->base_gfn;
> > u32 slot = (memslot->as_id << 16) | memslot->id;
> >
> > - if (kvm->dirty_ring_size)
> > + if (kvm->dirty_ring_size) {
> > kvm_dirty_ring_push(kvm_dirty_ring_get(kvm),
> > slot, rel_gfn);
> > - else
> > + } else {
> > set_bit_le(rel_gfn, memslot->dirty_bitmap);
> > + ++kvm->stat.generic.dirty_pages;
> > + }
>
> Aren't pages being pushed out to the dirty ring just as dirty? :-)
>
> > }
> > }
> > EXPORT_SYMBOL_GPL(mark_page_dirty_in_slot);
> >
> > base-commit: d0732b0f8884d9cc0eca0082bbaef043f3fef7fb
> > --
> > 2.32.0.605.g8dce9f2422-goog
> >
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: stats: Add VM dirty_pages stats for the number of dirty pages
@ 2021-08-10 23:15 ` Peter Feiner
0 siblings, 0 replies; 22+ messages in thread
From: Peter Feiner @ 2021-08-10 23:15 UTC (permalink / raw)
To: Oliver Upton
Cc: Jing Zhang, KVM, KVMPPC, Paolo Bonzini, Peter Shier,
David Matlack, Ben Gardon
On Tue, Aug 10, 2021 at 3:56 PM Oliver Upton <oupton@google.com> wrote:
>
> Hi Jing,
>
> On Tue, Aug 10, 2021 at 3:32 PM Jing Zhang <jingzhangos@google.com> wrote:
> >
> > Add a generic VM stats dirty_pages to record the number of dirty pages
> > reflected in dirty_bitmap at the moment.
>
> There can be multiple dirty bitmaps in a VM, one per memslot.
>
> > Original-by: Peter Feiner <pfeiner@google.com>
> > Signed-off-by: Jing Zhang <jingzhangos@google.com>
> > ---
> > arch/powerpc/kvm/book3s_64_mmu_hv.c | 8 ++++++--
> > arch/powerpc/kvm/book3s_64_mmu_radix.c | 1 +
> > arch/powerpc/kvm/book3s_hv_rm_mmu.c | 1 +
> > include/linux/kvm_host.h | 3 ++-
> > include/linux/kvm_types.h | 1 +
> > virt/kvm/kvm_main.c | 26 +++++++++++++++++++++++---
> > 6 files changed, 34 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > index c63e263312a4..e4aafa10efa1 100644
> > --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > @@ -1122,8 +1122,10 @@ long kvmppc_hv_get_dirty_log_hpt(struct kvm *kvm,
> > * since we always put huge-page HPTEs in the rmap chain
> > * corresponding to their page base address.
> > */
> > - if (npages)
> > + if (npages) {
> > set_dirty_bits(map, i, npages);
> > + kvm->stat.generic.dirty_pages += npages;
> > + }
> > ++rmapp;
> > }
> > preempt_enable();
> > @@ -1178,8 +1180,10 @@ void kvmppc_unpin_guest_page(struct kvm *kvm, void *va, unsigned long gpa,
> > gfn = gpa >> PAGE_SHIFT;
> > srcu_idx = srcu_read_lock(&kvm->srcu);
> > memslot = gfn_to_memslot(kvm, gfn);
> > - if (memslot && memslot->dirty_bitmap)
> > + if (memslot && memslot->dirty_bitmap) {
> > set_bit_le(gfn - memslot->base_gfn, memslot->dirty_bitmap);
> > + ++kvm->stat.generic.dirty_pages;
> > + }
> > srcu_read_unlock(&kvm->srcu, srcu_idx);
> > }
> >
> > diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > index b5905ae4377c..3a6cb3854a44 100644
> > --- a/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > +++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > @@ -1150,6 +1150,7 @@ long kvmppc_hv_get_dirty_log_radix(struct kvm *kvm,
> > j = i + 1;
> > if (npages) {
> > set_dirty_bits(map, i, npages);
> > + kvm->stat.generic.dirty_pages += npages;
> > j = i + npages;
> > }
> > }
> > diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > index 632b2545072b..16806bc473fa 100644
> > --- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > +++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > @@ -109,6 +109,7 @@ void kvmppc_update_dirty_map(const struct kvm_memory_slot *memslot,
> > npages = (psize + PAGE_SIZE - 1) / PAGE_SIZE;
> > gfn -= memslot->base_gfn;
> > set_dirty_bits_atomic(memslot->dirty_bitmap, gfn, npages);
> > + kvm->stat.generic.dirty_pages += npages;
> > }
> > EXPORT_SYMBOL_GPL(kvmppc_update_dirty_map);
> >
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index f50bfcf225f0..1e8e66fb915b 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -1421,7 +1421,8 @@ struct _kvm_stats_desc {
> > KVM_STATS_BASE_POW10, -9)
> >
> > #define KVM_GENERIC_VM_STATS() \
> > - STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush)
> > + STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush), \
> > + STATS_DESC_COUNTER(VM_GENERIC, dirty_pages)
> >
> > #define KVM_GENERIC_VCPU_STATS() \
> > STATS_DESC_COUNTER(VCPU_GENERIC, halt_successful_poll), \
> > diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
> > index ed6a985c5680..6c05df00aebf 100644
> > --- a/include/linux/kvm_types.h
> > +++ b/include/linux/kvm_types.h
> > @@ -78,6 +78,7 @@ struct kvm_mmu_memory_cache {
> >
> > struct kvm_vm_stat_generic {
> > u64 remote_tlb_flush;
> > + u64 dirty_pages;
> > };
> >
> > struct kvm_vcpu_stat_generic {
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index a438a7a3774a..93f0ca2ea326 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -1228,6 +1228,19 @@ static int kvm_alloc_dirty_bitmap(struct kvm_memory_slot *memslot)
> > return 0;
> > }
> >
> > +static inline unsigned long hweight_dirty_bitmap(
> > + struct kvm_memory_slot *memslot)
> > +{
> > + unsigned long i;
> > + unsigned long count = 0;
> > + unsigned long n = kvm_dirty_bitmap_bytes(memslot);
> > +
> > + for (i = 0; i < n / sizeof(long); ++i)
> > + count += hweight_long(memslot->dirty_bitmap[i]);
> > +
> > + return count;
> > +}
>
> Hrm, this seems like a decent amount of work for a statistic.
>
> > +
> > /*
> > * Delete a memslot by decrementing the number of used slots and shifting all
> > * other entries in the array forward one spot.
> > @@ -1612,6 +1625,7 @@ static int kvm_delete_memslot(struct kvm *kvm,
> > if (r)
> > return r;
> >
> > + kvm->stat.generic.dirty_pages -= hweight_dirty_bitmap(old);
> > kvm_free_memslot(kvm, old);
> > return 0;
> > }
> > @@ -1733,8 +1747,10 @@ int __kvm_set_memory_region(struct kvm *kvm,
> > if (r)
> > goto out_bitmap;
> >
> > - if (old.dirty_bitmap && !new.dirty_bitmap)
> > + if (old.dirty_bitmap && !new.dirty_bitmap) {
> > + kvm->stat.generic.dirty_pages -= hweight_dirty_bitmap(&old);
> > kvm_destroy_dirty_bitmap(&old);
> > + }
>
> Races to increment by a few pages might be OK, so long as imprecision
> is acceptable, but decrementing by an entire bitmap could cause the
> stat to get waaay off from the state of the VM.
My original use case was to know the rate at which memory was being
dirtied to predict how expensive a live migration would be. I didn't
need full precision, but I would have needed a bound on the slop. A
"few pages" isn't a bound :-)
IMO, this patch isn't worth figuring out without a use case. It's
complex and has perf overhead. Maybe just drop it?
>
> What if the statistic was 'dirtied_pages', which records the number of
> pages dirtied in the lifetime of a VM? Userspace could just record the
> value each time it blows away the dirty bitmaps and subtract that
> value next time it reads the stat. It would circumvent the need to
> walk the entire dirty bitmap to keep the statistic sane.
>
> > return 0;
> >
> > out_bitmap:
> > @@ -1895,6 +1911,7 @@ static int kvm_get_dirty_log_protect(struct kvm *kvm, struct kvm_dirty_log *log)
> > offset = i * BITS_PER_LONG;
> > kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot,
> > offset, mask);
> > + kvm->stat.generic.dirty_pages -= hweight_long(mask);
> > }
> > KVM_MMU_UNLOCK(kvm);
> > }
> > @@ -2012,6 +2029,7 @@ static int kvm_clear_dirty_log_protect(struct kvm *kvm,
> > flush = true;
> > kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot,
> > offset, mask);
> > + kvm->stat.generic.dirty_pages -= hweight_long(mask);
> > }
> > }
> > KVM_MMU_UNLOCK(kvm);
> > @@ -3062,11 +3080,13 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
> > unsigned long rel_gfn = gfn - memslot->base_gfn;
> > u32 slot = (memslot->as_id << 16) | memslot->id;
> >
> > - if (kvm->dirty_ring_size)
> > + if (kvm->dirty_ring_size) {
> > kvm_dirty_ring_push(kvm_dirty_ring_get(kvm),
> > slot, rel_gfn);
> > - else
> > + } else {
> > set_bit_le(rel_gfn, memslot->dirty_bitmap);
> > + ++kvm->stat.generic.dirty_pages;
> > + }
>
> Aren't pages being pushed out to the dirty ring just as dirty? :-)
>
> > }
> > }
> > EXPORT_SYMBOL_GPL(mark_page_dirty_in_slot);
> >
> > base-commit: d0732b0f8884d9cc0eca0082bbaef043f3fef7fb
> > --
> > 2.32.0.605.g8dce9f2422-goog
> >
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: stats: Add VM dirty_pages stats for the number of dirty pages
2021-08-10 23:15 ` Peter Feiner
@ 2021-08-10 23:26 ` Oliver Upton
-1 siblings, 0 replies; 22+ messages in thread
From: Oliver Upton @ 2021-08-10 23:26 UTC (permalink / raw)
To: Peter Feiner
Cc: Jing Zhang, KVM, KVMPPC, Paolo Bonzini, Peter Shier,
David Matlack, Ben Gardon
On Tue, Aug 10, 2021 at 4:15 PM Peter Feiner <pfeiner@google.com> wrote:
> > Races to increment by a few pages might be OK, so long as imprecision
> > is acceptable, but decrementing by an entire bitmap could cause the
> > stat to get waaay off from the state of the VM.
>
> My original use case was to know the rate at which memory was being
> dirtied to predict how expensive a live migration would be. I didn't
> need full precision, but I would have needed a bound on the slop. A
> "few pages" isn't a bound :-)
I think the agreement with VM-scoped statistics is that slop is OK,
better than paying the cost of locking/atomics. If we want to be
exact, it'd have to be a vCPU stat.
> IMO, this patch isn't worth figuring out without a use case. It's
> complex and has perf overhead. Maybe just drop it?
Knowing the approximate rate at which pages are being dirtied would be
a nice-to-have for debugging, IMO. Just treating this stat as
monotonic would greatly simplify it and avoid the overhead.
--
Thanks,
Oliver
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: stats: Add VM dirty_pages stats for the number of dirty pages
@ 2021-08-10 23:26 ` Oliver Upton
0 siblings, 0 replies; 22+ messages in thread
From: Oliver Upton @ 2021-08-10 23:26 UTC (permalink / raw)
To: Peter Feiner
Cc: Jing Zhang, KVM, KVMPPC, Paolo Bonzini, Peter Shier,
David Matlack, Ben Gardon
On Tue, Aug 10, 2021 at 4:15 PM Peter Feiner <pfeiner@google.com> wrote:
> > Races to increment by a few pages might be OK, so long as imprecision
> > is acceptable, but decrementing by an entire bitmap could cause the
> > stat to get waaay off from the state of the VM.
>
> My original use case was to know the rate at which memory was being
> dirtied to predict how expensive a live migration would be. I didn't
> need full precision, but I would have needed a bound on the slop. A
> "few pages" isn't a bound :-)
I think the agreement with VM-scoped statistics is that slop is OK,
better than paying the cost of locking/atomics. If we want to be
exact, it'd have to be a vCPU stat.
> IMO, this patch isn't worth figuring out without a use case. It's
> complex and has perf overhead. Maybe just drop it?
Knowing the approximate rate at which pages are being dirtied would be
a nice-to-have for debugging, IMO. Just treating this stat as
monotonic would greatly simplify it and avoid the overhead.
--
Thanks,
Oliver
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: stats: Add VM dirty_pages stats for the number of dirty pages
2021-08-10 22:56 ` Oliver Upton
@ 2021-08-11 10:20 ` Paolo Bonzini
-1 siblings, 0 replies; 22+ messages in thread
From: Paolo Bonzini @ 2021-08-11 10:20 UTC (permalink / raw)
To: Oliver Upton, Jing Zhang
Cc: KVM, KVMPPC, Peter Shier, David Matlack, Ben Gardon, Peter Feiner
On 11/08/21 00:56, Oliver Upton wrote:
> What if the statistic was 'dirtied_pages', which records the number of
> pages dirtied in the lifetime of a VM? Userspace could just record the
> value each time it blows away the dirty bitmaps and subtract that
> value next time it reads the stat. It would circumvent the need to
> walk the entire dirty bitmap to keep the statistic sane.
Yeah, that'd be much better also because the "number of dirty pages"
statistic is not well defined in init-all-dirty mode.
Making it a vCPU stat works in fact, because mark_page_dirty_in_slot is
only called with kvm_get_running_vcpu() != NULL; see
kvm_dirty_ring_get() in virt/kvm/dirty_ring.c.
>>
>> + if (kvm->dirty_ring_size) {
>> kvm_dirty_ring_push(kvm_dirty_ring_get(kvm),
>> slot, rel_gfn);
>> - else
>> + } else {
>> set_bit_le(rel_gfn, memslot->dirty_bitmap);
>> + ++kvm->stat.generic.dirty_pages;
>> + }
>
> Aren't pages being pushed out to the dirty ring just as dirty?
>
Yes, they are.
Paolo
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: stats: Add VM dirty_pages stats for the number of dirty pages
@ 2021-08-11 10:20 ` Paolo Bonzini
0 siblings, 0 replies; 22+ messages in thread
From: Paolo Bonzini @ 2021-08-11 10:20 UTC (permalink / raw)
To: Oliver Upton, Jing Zhang
Cc: KVM, KVMPPC, Peter Shier, David Matlack, Ben Gardon, Peter Feiner
On 11/08/21 00:56, Oliver Upton wrote:
> What if the statistic was 'dirtied_pages', which records the number of
> pages dirtied in the lifetime of a VM? Userspace could just record the
> value each time it blows away the dirty bitmaps and subtract that
> value next time it reads the stat. It would circumvent the need to
> walk the entire dirty bitmap to keep the statistic sane.
Yeah, that'd be much better also because the "number of dirty pages"
statistic is not well defined in init-all-dirty mode.
Making it a vCPU stat works in fact, because mark_page_dirty_in_slot is
only called with kvm_get_running_vcpu() != NULL; see
kvm_dirty_ring_get() in virt/kvm/dirty_ring.c.
>>
>> + if (kvm->dirty_ring_size) {
>> kvm_dirty_ring_push(kvm_dirty_ring_get(kvm),
>> slot, rel_gfn);
>> - else
>> + } else {
>> set_bit_le(rel_gfn, memslot->dirty_bitmap);
>> + ++kvm->stat.generic.dirty_pages;
>> + }
>
> Aren't pages being pushed out to the dirty ring just as dirty?
>
Yes, they are.
Paolo
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: stats: Add VM dirty_pages stats for the number of dirty pages
2021-08-10 22:32 [PATCH] KVM: stats: Add VM dirty_pages stats for the number of dirty pages Jing Zhang
2021-08-10 22:44 ` Peter Feiner
2021-08-10 22:56 ` Oliver Upton
@ 2021-08-11 12:26 ` kernel test robot
2 siblings, 0 replies; 22+ messages in thread
From: kernel test robot @ 2021-08-11 12:26 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 2459 bytes --]
Hi Jing,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on d0732b0f8884d9cc0eca0082bbaef043f3fef7fb]
url: https://github.com/0day-ci/linux/commits/Jing-Zhang/KVM-stats-Add-VM-dirty_pages-stats-for-the-number-of-dirty-pages/20210811-063544
base: d0732b0f8884d9cc0eca0082bbaef043f3fef7fb
config: powerpc-allyesconfig (attached as .config)
compiler: powerpc-linux-gcc (GCC) 10.3.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/0day-ci/linux/commit/85679ee0cff16eaae940baba21e08ad6e9944698
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Jing-Zhang/KVM-stats-Add-VM-dirty_pages-stats-for-the-number-of-dirty-pages/20210811-063544
git checkout 85679ee0cff16eaae940baba21e08ad6e9944698
# save the attached .config to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross O=build_dir ARCH=powerpc SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
arch/powerpc/kvm/book3s_hv_rm_mmu.c: In function 'kvmppc_update_dirty_map':
>> arch/powerpc/kvm/book3s_hv_rm_mmu.c:112:2: error: 'kvm' undeclared (first use in this function)
112 | kvm->stat.generic.dirty_pages += npages;
| ^~~
arch/powerpc/kvm/book3s_hv_rm_mmu.c:112:2: note: each undeclared identifier is reported only once for each function it appears in
vim +/kvm +112 arch/powerpc/kvm/book3s_hv_rm_mmu.c
100
101 /* Update the dirty bitmap of a memslot */
102 void kvmppc_update_dirty_map(const struct kvm_memory_slot *memslot,
103 unsigned long gfn, unsigned long psize)
104 {
105 unsigned long npages;
106
107 if (!psize || !memslot->dirty_bitmap)
108 return;
109 npages = (psize + PAGE_SIZE - 1) / PAGE_SIZE;
110 gfn -= memslot->base_gfn;
111 set_dirty_bits_atomic(memslot->dirty_bitmap, gfn, npages);
> 112 kvm->stat.generic.dirty_pages += npages;
113 }
114 EXPORT_SYMBOL_GPL(kvmppc_update_dirty_map);
115
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 73326 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: stats: Add VM dirty_pages stats for the number of dirty pages
2021-08-11 10:20 ` Paolo Bonzini
@ 2021-08-11 20:14 ` Jing Zhang
-1 siblings, 0 replies; 22+ messages in thread
From: Jing Zhang @ 2021-08-11 20:14 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Oliver Upton, KVM, KVMPPC, Peter Shier, David Matlack,
Ben Gardon, Peter Feiner
On Wed, Aug 11, 2021 at 3:20 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 11/08/21 00:56, Oliver Upton wrote:
> > What if the statistic was 'dirtied_pages', which records the number of
> > pages dirtied in the lifetime of a VM? Userspace could just record the
> > value each time it blows away the dirty bitmaps and subtract that
> > value next time it reads the stat. It would circumvent the need to
> > walk the entire dirty bitmap to keep the statistic sane.
>
> Yeah, that'd be much better also because the "number of dirty pages"
> statistic is not well defined in init-all-dirty mode.
>
> Making it a vCPU stat works in fact, because mark_page_dirty_in_slot is
> only called with kvm_get_running_vcpu() != NULL; see
> kvm_dirty_ring_get() in virt/kvm/dirty_ring.c.
>
> >>
> >> + if (kvm->dirty_ring_size) {
> >> kvm_dirty_ring_push(kvm_dirty_ring_get(kvm),
> >> slot, rel_gfn);
> >> - else
> >> + } else {
> >> set_bit_le(rel_gfn, memslot->dirty_bitmap);
> >> + ++kvm->stat.generic.dirty_pages;
> >> + }
> >
> > Aren't pages being pushed out to the dirty ring just as dirty?
> >
>
> Yes, they are.
>
> Paolo
>
According to Oliver's idea, let's define the "dirty_pages" as the
number of dirtied pages during the life cycle of a VM to avoid the
overhead caused by walking the entire dirty_bitmap.
I didn't consider the dirty ring in this patch, but will do in the next.
Will still define dirty_pages as a VM scoped stat in the next patch.
From there, we will see if it is necessary to define it as a VCPU
scoped stat. (Both KVM code and user code would be simpler with it
being as a VM scoped stat).
Thanks,
Jing
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: stats: Add VM dirty_pages stats for the number of dirty pages
@ 2021-08-11 20:14 ` Jing Zhang
0 siblings, 0 replies; 22+ messages in thread
From: Jing Zhang @ 2021-08-11 20:14 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Oliver Upton, KVM, KVMPPC, Peter Shier, David Matlack,
Ben Gardon, Peter Feiner
On Wed, Aug 11, 2021 at 3:20 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 11/08/21 00:56, Oliver Upton wrote:
> > What if the statistic was 'dirtied_pages', which records the number of
> > pages dirtied in the lifetime of a VM? Userspace could just record the
> > value each time it blows away the dirty bitmaps and subtract that
> > value next time it reads the stat. It would circumvent the need to
> > walk the entire dirty bitmap to keep the statistic sane.
>
> Yeah, that'd be much better also because the "number of dirty pages"
> statistic is not well defined in init-all-dirty mode.
>
> Making it a vCPU stat works in fact, because mark_page_dirty_in_slot is
> only called with kvm_get_running_vcpu() != NULL; see
> kvm_dirty_ring_get() in virt/kvm/dirty_ring.c.
>
> >>
> >> + if (kvm->dirty_ring_size) {
> >> kvm_dirty_ring_push(kvm_dirty_ring_get(kvm),
> >> slot, rel_gfn);
> >> - else
> >> + } else {
> >> set_bit_le(rel_gfn, memslot->dirty_bitmap);
> >> + ++kvm->stat.generic.dirty_pages;
> >> + }
> >
> > Aren't pages being pushed out to the dirty ring just as dirty?
> >
>
> Yes, they are.
>
> Paolo
>
According to Oliver's idea, let's define the "dirty_pages" as the
number of dirtied pages during the life cycle of a VM to avoid the
overhead caused by walking the entire dirty_bitmap.
I didn't consider the dirty ring in this patch, but will do in the next.
Will still define dirty_pages as a VM scoped stat in the next patch.
From there, we will see if it is necessary to define it as a VCPU
scoped stat. (Both KVM code and user code would be simpler with it
being as a VM scoped stat).
Thanks,
Jing
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: stats: Add VM dirty_pages stats for the number of dirty pages
2021-08-10 22:44 ` Peter Feiner
@ 2021-08-11 22:52 ` Jing Zhang
-1 siblings, 0 replies; 22+ messages in thread
From: Jing Zhang @ 2021-08-11 22:52 UTC (permalink / raw)
To: Peter Feiner
Cc: KVM, KVMPPC, Paolo Bonzini, Peter Shier, Oliver Upton,
David Matlack, Ben Gardon
Hi Peter,
On Tue, Aug 10, 2021 at 3:45 PM Peter Feiner <pfeiner@google.com> wrote:
>
> On Tue, Aug 10, 2021 at 3:32 PM Jing Zhang <jingzhangos@google.com> wrote:
> >
> > Add a generic VM stats dirty_pages to record the number of dirty pages
> > reflected in dirty_bitmap at the moment.
> >
> > Original-by: Peter Feiner <pfeiner@google.com>
> > Signed-off-by: Jing Zhang <jingzhangos@google.com>
> > ---
> > arch/powerpc/kvm/book3s_64_mmu_hv.c | 8 ++++++--
> > arch/powerpc/kvm/book3s_64_mmu_radix.c | 1 +
> > arch/powerpc/kvm/book3s_hv_rm_mmu.c | 1 +
> > include/linux/kvm_host.h | 3 ++-
> > include/linux/kvm_types.h | 1 +
> > virt/kvm/kvm_main.c | 26 +++++++++++++++++++++++---
> > 6 files changed, 34 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > index c63e263312a4..e4aafa10efa1 100644
> > --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > @@ -1122,8 +1122,10 @@ long kvmppc_hv_get_dirty_log_hpt(struct kvm *kvm,
> > * since we always put huge-page HPTEs in the rmap chain
> > * corresponding to their page base address.
> > */
> > - if (npages)
> > + if (npages) {
> > set_dirty_bits(map, i, npages);
> > + kvm->stat.generic.dirty_pages += npages;
> > + }
> > ++rmapp;
> > }
> > preempt_enable();
> > @@ -1178,8 +1180,10 @@ void kvmppc_unpin_guest_page(struct kvm *kvm, void *va, unsigned long gpa,
> > gfn = gpa >> PAGE_SHIFT;
> > srcu_idx = srcu_read_lock(&kvm->srcu);
> > memslot = gfn_to_memslot(kvm, gfn);
> > - if (memslot && memslot->dirty_bitmap)
> > + if (memslot && memslot->dirty_bitmap) {
> > set_bit_le(gfn - memslot->base_gfn, memslot->dirty_bitmap);
> > + ++kvm->stat.generic.dirty_pages;
> > + }
> > srcu_read_unlock(&kvm->srcu, srcu_idx);
> > }
> >
> > diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > index b5905ae4377c..3a6cb3854a44 100644
> > --- a/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > +++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > @@ -1150,6 +1150,7 @@ long kvmppc_hv_get_dirty_log_radix(struct kvm *kvm,
> > j = i + 1;
> > if (npages) {
> > set_dirty_bits(map, i, npages);
> > + kvm->stat.generic.dirty_pages += npages;
> > j = i + npages;
> > }
> > }
> > diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > index 632b2545072b..16806bc473fa 100644
> > --- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > +++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > @@ -109,6 +109,7 @@ void kvmppc_update_dirty_map(const struct kvm_memory_slot *memslot,
> > npages = (psize + PAGE_SIZE - 1) / PAGE_SIZE;
> > gfn -= memslot->base_gfn;
> > set_dirty_bits_atomic(memslot->dirty_bitmap, gfn, npages);
> > + kvm->stat.generic.dirty_pages += npages;
> > }
> > EXPORT_SYMBOL_GPL(kvmppc_update_dirty_map);
> >
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index f50bfcf225f0..1e8e66fb915b 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -1421,7 +1421,8 @@ struct _kvm_stats_desc {
> > KVM_STATS_BASE_POW10, -9)
> >
> > #define KVM_GENERIC_VM_STATS() \
> > - STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush)
> > + STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush), \
> > + STATS_DESC_COUNTER(VM_GENERIC, dirty_pages)
> >
> > #define KVM_GENERIC_VCPU_STATS() \
> > STATS_DESC_COUNTER(VCPU_GENERIC, halt_successful_poll), \
> > diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
> > index ed6a985c5680..6c05df00aebf 100644
> > --- a/include/linux/kvm_types.h
> > +++ b/include/linux/kvm_types.h
> > @@ -78,6 +78,7 @@ struct kvm_mmu_memory_cache {
> >
> > struct kvm_vm_stat_generic {
> > u64 remote_tlb_flush;
> > + u64 dirty_pages;
> > };
> >
> > struct kvm_vcpu_stat_generic {
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index a438a7a3774a..93f0ca2ea326 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -1228,6 +1228,19 @@ static int kvm_alloc_dirty_bitmap(struct kvm_memory_slot *memslot)
> > return 0;
> > }
> >
> > +static inline unsigned long hweight_dirty_bitmap(
> > + struct kvm_memory_slot *memslot)
> > +{
> > + unsigned long i;
> > + unsigned long count = 0;
> > + unsigned long n = kvm_dirty_bitmap_bytes(memslot);
> > +
> > + for (i = 0; i < n / sizeof(long); ++i)
> > + count += hweight_long(memslot->dirty_bitmap[i]);
> > +
> > + return count;
> > +}
> > +
> > /*
> > * Delete a memslot by decrementing the number of used slots and shifting all
> > * other entries in the array forward one spot.
> > @@ -1612,6 +1625,7 @@ static int kvm_delete_memslot(struct kvm *kvm,
> > if (r)
> > return r;
> >
> > + kvm->stat.generic.dirty_pages -= hweight_dirty_bitmap(old);
> > kvm_free_memslot(kvm, old);
> > return 0;
> > }
> > @@ -1733,8 +1747,10 @@ int __kvm_set_memory_region(struct kvm *kvm,
> > if (r)
> > goto out_bitmap;
> >
> > - if (old.dirty_bitmap && !new.dirty_bitmap)
> > + if (old.dirty_bitmap && !new.dirty_bitmap) {
> > + kvm->stat.generic.dirty_pages -= hweight_dirty_bitmap(&old);
> > kvm_destroy_dirty_bitmap(&old);
> > + }
> > return 0;
> >
> > out_bitmap:
> > @@ -1895,6 +1911,7 @@ static int kvm_get_dirty_log_protect(struct kvm *kvm, struct kvm_dirty_log *log)
> > offset = i * BITS_PER_LONG;
> > kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot,
> > offset, mask);
> > + kvm->stat.generic.dirty_pages -= hweight_long(mask);
> > }
> > KVM_MMU_UNLOCK(kvm);
> > }
> > @@ -2012,6 +2029,7 @@ static int kvm_clear_dirty_log_protect(struct kvm *kvm,
> > flush = true;
> > kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot,
> > offset, mask);
> > + kvm->stat.generic.dirty_pages -= hweight_long(mask);
> > }
> > }
> > KVM_MMU_UNLOCK(kvm);
> > @@ -3062,11 +3080,13 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
> > unsigned long rel_gfn = gfn - memslot->base_gfn;
> > u32 slot = (memslot->as_id << 16) | memslot->id;
> >
> > - if (kvm->dirty_ring_size)
> > + if (kvm->dirty_ring_size) {
> > kvm_dirty_ring_push(kvm_dirty_ring_get(kvm),
> > slot, rel_gfn);
> > - else
> > + } else {
> > set_bit_le(rel_gfn, memslot->dirty_bitmap);
> > + ++kvm->stat.generic.dirty_pages;
>
> Couple of problems here:
>
> - Calls to mark_page_dirty_in_slot aren't serialized by the mmu_lock,
> so these updates will race.
> - The page might already be dirty in the bitmap, so you're potentially
> double counting here
>
> You can fix both of these problems by changing set_bit_le to a
> test_and_set_bit_le (might not be the function name -- but you get the
> idea) and conditionally incrementing dirty_pages.
>
>
Since we need to consider the dirty ring situation and it is not
trivial to check if the page is already in the dirty ring, I'll not
change set_bit_le. Anyway, we are going to define the stat
"dirty_pages" as dirtied pages in the life cycle of a VM, we only care
about its growth rate for your use case.
I'll send out another version of the patch and we'll see if it is good enough.
> > + }
> > }
> > }
> > EXPORT_SYMBOL_GPL(mark_page_dirty_in_slot);
> >
> > base-commit: d0732b0f8884d9cc0eca0082bbaef043f3fef7fb
> > --
> > 2.32.0.605.g8dce9f2422-goog
> >
Thanks,
Jing
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: stats: Add VM dirty_pages stats for the number of dirty pages
@ 2021-08-11 22:52 ` Jing Zhang
0 siblings, 0 replies; 22+ messages in thread
From: Jing Zhang @ 2021-08-11 22:52 UTC (permalink / raw)
To: Peter Feiner
Cc: KVM, KVMPPC, Paolo Bonzini, Peter Shier, Oliver Upton,
David Matlack, Ben Gardon
Hi Peter,
On Tue, Aug 10, 2021 at 3:45 PM Peter Feiner <pfeiner@google.com> wrote:
>
> On Tue, Aug 10, 2021 at 3:32 PM Jing Zhang <jingzhangos@google.com> wrote:
> >
> > Add a generic VM stats dirty_pages to record the number of dirty pages
> > reflected in dirty_bitmap at the moment.
> >
> > Original-by: Peter Feiner <pfeiner@google.com>
> > Signed-off-by: Jing Zhang <jingzhangos@google.com>
> > ---
> > arch/powerpc/kvm/book3s_64_mmu_hv.c | 8 ++++++--
> > arch/powerpc/kvm/book3s_64_mmu_radix.c | 1 +
> > arch/powerpc/kvm/book3s_hv_rm_mmu.c | 1 +
> > include/linux/kvm_host.h | 3 ++-
> > include/linux/kvm_types.h | 1 +
> > virt/kvm/kvm_main.c | 26 +++++++++++++++++++++++---
> > 6 files changed, 34 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > index c63e263312a4..e4aafa10efa1 100644
> > --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > @@ -1122,8 +1122,10 @@ long kvmppc_hv_get_dirty_log_hpt(struct kvm *kvm,
> > * since we always put huge-page HPTEs in the rmap chain
> > * corresponding to their page base address.
> > */
> > - if (npages)
> > + if (npages) {
> > set_dirty_bits(map, i, npages);
> > + kvm->stat.generic.dirty_pages += npages;
> > + }
> > ++rmapp;
> > }
> > preempt_enable();
> > @@ -1178,8 +1180,10 @@ void kvmppc_unpin_guest_page(struct kvm *kvm, void *va, unsigned long gpa,
> > gfn = gpa >> PAGE_SHIFT;
> > srcu_idx = srcu_read_lock(&kvm->srcu);
> > memslot = gfn_to_memslot(kvm, gfn);
> > - if (memslot && memslot->dirty_bitmap)
> > + if (memslot && memslot->dirty_bitmap) {
> > set_bit_le(gfn - memslot->base_gfn, memslot->dirty_bitmap);
> > + ++kvm->stat.generic.dirty_pages;
> > + }
> > srcu_read_unlock(&kvm->srcu, srcu_idx);
> > }
> >
> > diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > index b5905ae4377c..3a6cb3854a44 100644
> > --- a/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > +++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > @@ -1150,6 +1150,7 @@ long kvmppc_hv_get_dirty_log_radix(struct kvm *kvm,
> > j = i + 1;
> > if (npages) {
> > set_dirty_bits(map, i, npages);
> > + kvm->stat.generic.dirty_pages += npages;
> > j = i + npages;
> > }
> > }
> > diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > index 632b2545072b..16806bc473fa 100644
> > --- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > +++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > @@ -109,6 +109,7 @@ void kvmppc_update_dirty_map(const struct kvm_memory_slot *memslot,
> > npages = (psize + PAGE_SIZE - 1) / PAGE_SIZE;
> > gfn -= memslot->base_gfn;
> > set_dirty_bits_atomic(memslot->dirty_bitmap, gfn, npages);
> > + kvm->stat.generic.dirty_pages += npages;
> > }
> > EXPORT_SYMBOL_GPL(kvmppc_update_dirty_map);
> >
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index f50bfcf225f0..1e8e66fb915b 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -1421,7 +1421,8 @@ struct _kvm_stats_desc {
> > KVM_STATS_BASE_POW10, -9)
> >
> > #define KVM_GENERIC_VM_STATS() \
> > - STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush)
> > + STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush), \
> > + STATS_DESC_COUNTER(VM_GENERIC, dirty_pages)
> >
> > #define KVM_GENERIC_VCPU_STATS() \
> > STATS_DESC_COUNTER(VCPU_GENERIC, halt_successful_poll), \
> > diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
> > index ed6a985c5680..6c05df00aebf 100644
> > --- a/include/linux/kvm_types.h
> > +++ b/include/linux/kvm_types.h
> > @@ -78,6 +78,7 @@ struct kvm_mmu_memory_cache {
> >
> > struct kvm_vm_stat_generic {
> > u64 remote_tlb_flush;
> > + u64 dirty_pages;
> > };
> >
> > struct kvm_vcpu_stat_generic {
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index a438a7a3774a..93f0ca2ea326 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -1228,6 +1228,19 @@ static int kvm_alloc_dirty_bitmap(struct kvm_memory_slot *memslot)
> > return 0;
> > }
> >
> > +static inline unsigned long hweight_dirty_bitmap(
> > + struct kvm_memory_slot *memslot)
> > +{
> > + unsigned long i;
> > + unsigned long count = 0;
> > + unsigned long n = kvm_dirty_bitmap_bytes(memslot);
> > +
> > + for (i = 0; i < n / sizeof(long); ++i)
> > + count += hweight_long(memslot->dirty_bitmap[i]);
> > +
> > + return count;
> > +}
> > +
> > /*
> > * Delete a memslot by decrementing the number of used slots and shifting all
> > * other entries in the array forward one spot.
> > @@ -1612,6 +1625,7 @@ static int kvm_delete_memslot(struct kvm *kvm,
> > if (r)
> > return r;
> >
> > + kvm->stat.generic.dirty_pages -= hweight_dirty_bitmap(old);
> > kvm_free_memslot(kvm, old);
> > return 0;
> > }
> > @@ -1733,8 +1747,10 @@ int __kvm_set_memory_region(struct kvm *kvm,
> > if (r)
> > goto out_bitmap;
> >
> > - if (old.dirty_bitmap && !new.dirty_bitmap)
> > + if (old.dirty_bitmap && !new.dirty_bitmap) {
> > + kvm->stat.generic.dirty_pages -= hweight_dirty_bitmap(&old);
> > kvm_destroy_dirty_bitmap(&old);
> > + }
> > return 0;
> >
> > out_bitmap:
> > @@ -1895,6 +1911,7 @@ static int kvm_get_dirty_log_protect(struct kvm *kvm, struct kvm_dirty_log *log)
> > offset = i * BITS_PER_LONG;
> > kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot,
> > offset, mask);
> > + kvm->stat.generic.dirty_pages -= hweight_long(mask);
> > }
> > KVM_MMU_UNLOCK(kvm);
> > }
> > @@ -2012,6 +2029,7 @@ static int kvm_clear_dirty_log_protect(struct kvm *kvm,
> > flush = true;
> > kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot,
> > offset, mask);
> > + kvm->stat.generic.dirty_pages -= hweight_long(mask);
> > }
> > }
> > KVM_MMU_UNLOCK(kvm);
> > @@ -3062,11 +3080,13 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
> > unsigned long rel_gfn = gfn - memslot->base_gfn;
> > u32 slot = (memslot->as_id << 16) | memslot->id;
> >
> > - if (kvm->dirty_ring_size)
> > + if (kvm->dirty_ring_size) {
> > kvm_dirty_ring_push(kvm_dirty_ring_get(kvm),
> > slot, rel_gfn);
> > - else
> > + } else {
> > set_bit_le(rel_gfn, memslot->dirty_bitmap);
> > + ++kvm->stat.generic.dirty_pages;
>
> Couple of problems here:
>
> - Calls to mark_page_dirty_in_slot aren't serialized by the mmu_lock,
> so these updates will race.
> - The page might already be dirty in the bitmap, so you're potentially
> double counting here
>
> You can fix both of these problems by changing set_bit_le to a
> test_and_set_bit_le (might not be the function name -- but you get the
> idea) and conditionally incrementing dirty_pages.
>
>
Since we need to consider the dirty ring situation and it is not
trivial to check if the page is already in the dirty ring, I'll not
change set_bit_le. Anyway, we are going to define the stat
"dirty_pages" as dirtied pages in the life cycle of a VM, we only care
about its growth rate for your use case.
I'll send out another version of the patch and we'll see if it is good enough.
> > + }
> > }
> > }
> > EXPORT_SYMBOL_GPL(mark_page_dirty_in_slot);
> >
> > base-commit: d0732b0f8884d9cc0eca0082bbaef043f3fef7fb
> > --
> > 2.32.0.605.g8dce9f2422-goog
> >
Thanks,
Jing
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: stats: Add VM dirty_pages stats for the number of dirty pages
2021-08-11 22:52 ` Jing Zhang
@ 2021-08-11 23:19 ` Peter Feiner
-1 siblings, 0 replies; 22+ messages in thread
From: Peter Feiner @ 2021-08-11 23:19 UTC (permalink / raw)
To: Jing Zhang
Cc: KVM, KVMPPC, Paolo Bonzini, Peter Shier, Oliver Upton,
David Matlack, Ben Gardon
On Wed, Aug 11, 2021 at 3:53 PM Jing Zhang <jingzhangos@google.com> wrote:
>
> Hi Peter,
>
> On Tue, Aug 10, 2021 at 3:45 PM Peter Feiner <pfeiner@google.com> wrote:
> >
> > On Tue, Aug 10, 2021 at 3:32 PM Jing Zhang <jingzhangos@google.com> wrote:
> > >
> > > Add a generic VM stats dirty_pages to record the number of dirty pages
> > > reflected in dirty_bitmap at the moment.
> > >
> > > Original-by: Peter Feiner <pfeiner@google.com>
> > > Signed-off-by: Jing Zhang <jingzhangos@google.com>
> > > ---
> > > arch/powerpc/kvm/book3s_64_mmu_hv.c | 8 ++++++--
> > > arch/powerpc/kvm/book3s_64_mmu_radix.c | 1 +
> > > arch/powerpc/kvm/book3s_hv_rm_mmu.c | 1 +
> > > include/linux/kvm_host.h | 3 ++-
> > > include/linux/kvm_types.h | 1 +
> > > virt/kvm/kvm_main.c | 26 +++++++++++++++++++++++---
> > > 6 files changed, 34 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > > index c63e263312a4..e4aafa10efa1 100644
> > > --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > > +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > > @@ -1122,8 +1122,10 @@ long kvmppc_hv_get_dirty_log_hpt(struct kvm *kvm,
> > > * since we always put huge-page HPTEs in the rmap chain
> > > * corresponding to their page base address.
> > > */
> > > - if (npages)
> > > + if (npages) {
> > > set_dirty_bits(map, i, npages);
> > > + kvm->stat.generic.dirty_pages += npages;
> > > + }
> > > ++rmapp;
> > > }
> > > preempt_enable();
> > > @@ -1178,8 +1180,10 @@ void kvmppc_unpin_guest_page(struct kvm *kvm, void *va, unsigned long gpa,
> > > gfn = gpa >> PAGE_SHIFT;
> > > srcu_idx = srcu_read_lock(&kvm->srcu);
> > > memslot = gfn_to_memslot(kvm, gfn);
> > > - if (memslot && memslot->dirty_bitmap)
> > > + if (memslot && memslot->dirty_bitmap) {
> > > set_bit_le(gfn - memslot->base_gfn, memslot->dirty_bitmap);
> > > + ++kvm->stat.generic.dirty_pages;
> > > + }
> > > srcu_read_unlock(&kvm->srcu, srcu_idx);
> > > }
> > >
> > > diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > > index b5905ae4377c..3a6cb3854a44 100644
> > > --- a/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > > +++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > > @@ -1150,6 +1150,7 @@ long kvmppc_hv_get_dirty_log_radix(struct kvm *kvm,
> > > j = i + 1;
> > > if (npages) {
> > > set_dirty_bits(map, i, npages);
> > > + kvm->stat.generic.dirty_pages += npages;
> > > j = i + npages;
> > > }
> > > }
> > > diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > > index 632b2545072b..16806bc473fa 100644
> > > --- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > > +++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > > @@ -109,6 +109,7 @@ void kvmppc_update_dirty_map(const struct kvm_memory_slot *memslot,
> > > npages = (psize + PAGE_SIZE - 1) / PAGE_SIZE;
> > > gfn -= memslot->base_gfn;
> > > set_dirty_bits_atomic(memslot->dirty_bitmap, gfn, npages);
> > > + kvm->stat.generic.dirty_pages += npages;
> > > }
> > > EXPORT_SYMBOL_GPL(kvmppc_update_dirty_map);
> > >
> > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > > index f50bfcf225f0..1e8e66fb915b 100644
> > > --- a/include/linux/kvm_host.h
> > > +++ b/include/linux/kvm_host.h
> > > @@ -1421,7 +1421,8 @@ struct _kvm_stats_desc {
> > > KVM_STATS_BASE_POW10, -9)
> > >
> > > #define KVM_GENERIC_VM_STATS() \
> > > - STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush)
> > > + STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush), \
> > > + STATS_DESC_COUNTER(VM_GENERIC, dirty_pages)
> > >
> > > #define KVM_GENERIC_VCPU_STATS() \
> > > STATS_DESC_COUNTER(VCPU_GENERIC, halt_successful_poll), \
> > > diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
> > > index ed6a985c5680..6c05df00aebf 100644
> > > --- a/include/linux/kvm_types.h
> > > +++ b/include/linux/kvm_types.h
> > > @@ -78,6 +78,7 @@ struct kvm_mmu_memory_cache {
> > >
> > > struct kvm_vm_stat_generic {
> > > u64 remote_tlb_flush;
> > > + u64 dirty_pages;
> > > };
> > >
> > > struct kvm_vcpu_stat_generic {
> > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > > index a438a7a3774a..93f0ca2ea326 100644
> > > --- a/virt/kvm/kvm_main.c
> > > +++ b/virt/kvm/kvm_main.c
> > > @@ -1228,6 +1228,19 @@ static int kvm_alloc_dirty_bitmap(struct kvm_memory_slot *memslot)
> > > return 0;
> > > }
> > >
> > > +static inline unsigned long hweight_dirty_bitmap(
> > > + struct kvm_memory_slot *memslot)
> > > +{
> > > + unsigned long i;
> > > + unsigned long count = 0;
> > > + unsigned long n = kvm_dirty_bitmap_bytes(memslot);
> > > +
> > > + for (i = 0; i < n / sizeof(long); ++i)
> > > + count += hweight_long(memslot->dirty_bitmap[i]);
> > > +
> > > + return count;
> > > +}
> > > +
> > > /*
> > > * Delete a memslot by decrementing the number of used slots and shifting all
> > > * other entries in the array forward one spot.
> > > @@ -1612,6 +1625,7 @@ static int kvm_delete_memslot(struct kvm *kvm,
> > > if (r)
> > > return r;
> > >
> > > + kvm->stat.generic.dirty_pages -= hweight_dirty_bitmap(old);
> > > kvm_free_memslot(kvm, old);
> > > return 0;
> > > }
> > > @@ -1733,8 +1747,10 @@ int __kvm_set_memory_region(struct kvm *kvm,
> > > if (r)
> > > goto out_bitmap;
> > >
> > > - if (old.dirty_bitmap && !new.dirty_bitmap)
> > > + if (old.dirty_bitmap && !new.dirty_bitmap) {
> > > + kvm->stat.generic.dirty_pages -= hweight_dirty_bitmap(&old);
> > > kvm_destroy_dirty_bitmap(&old);
> > > + }
> > > return 0;
> > >
> > > out_bitmap:
> > > @@ -1895,6 +1911,7 @@ static int kvm_get_dirty_log_protect(struct kvm *kvm, struct kvm_dirty_log *log)
> > > offset = i * BITS_PER_LONG;
> > > kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot,
> > > offset, mask);
> > > + kvm->stat.generic.dirty_pages -= hweight_long(mask);
> > > }
> > > KVM_MMU_UNLOCK(kvm);
> > > }
> > > @@ -2012,6 +2029,7 @@ static int kvm_clear_dirty_log_protect(struct kvm *kvm,
> > > flush = true;
> > > kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot,
> > > offset, mask);
> > > + kvm->stat.generic.dirty_pages -= hweight_long(mask);
> > > }
> > > }
> > > KVM_MMU_UNLOCK(kvm);
> > > @@ -3062,11 +3080,13 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
> > > unsigned long rel_gfn = gfn - memslot->base_gfn;
> > > u32 slot = (memslot->as_id << 16) | memslot->id;
> > >
> > > - if (kvm->dirty_ring_size)
> > > + if (kvm->dirty_ring_size) {
> > > kvm_dirty_ring_push(kvm_dirty_ring_get(kvm),
> > > slot, rel_gfn);
> > > - else
> > > + } else {
> > > set_bit_le(rel_gfn, memslot->dirty_bitmap);
> > > + ++kvm->stat.generic.dirty_pages;
> >
> > Couple of problems here:
> >
> > - Calls to mark_page_dirty_in_slot aren't serialized by the mmu_lock,
> > so these updates will race.
> > - The page might already be dirty in the bitmap, so you're potentially
> > double counting here
> >
> > You can fix both of these problems by changing set_bit_le to a
> > test_and_set_bit_le (might not be the function name -- but you get the
> > idea) and conditionally incrementing dirty_pages.
> >
> >
> Since we need to consider the dirty ring situation and it is not
> trivial to check if the page is already in the dirty ring, I'll not
> change set_bit_le. Anyway, we are going to define the stat
> "dirty_pages" as dirtied pages in the life cycle of a VM, we only care
> about its growth rate for your use case.
To be clear, I don't have a use case anymore. It was years ago that I
came up with the idea of doing a kind of light memory dirty tracking
to predict how expensive pre-copy migration was going to be. However,
I didn't follow through on the idea; so if this never saw the light of
day in an upstream kernel, I'd be no worse for wear.
With that said, the use case I imagined did require knowing how many
pages were in the dirty bitmap at any moment. The number of pages
dirtied across the lifetime of the VM wouldn't have served that
purpose. So unless you have a use case for this new stat that's been
thought through end-to-end, I would suggest just dropping this patch
here (and from Google's kernel fork as well).
> I'll send out another version of the patch and we'll see if it is good enough.
> > > + }
> > > }
> > > }
> > > EXPORT_SYMBOL_GPL(mark_page_dirty_in_slot);
> > >
> > > base-commit: d0732b0f8884d9cc0eca0082bbaef043f3fef7fb
> > > --
> > > 2.32.0.605.g8dce9f2422-goog
> > >
>
> Thanks,
> Jing
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: stats: Add VM dirty_pages stats for the number of dirty pages
@ 2021-08-11 23:19 ` Peter Feiner
0 siblings, 0 replies; 22+ messages in thread
From: Peter Feiner @ 2021-08-11 23:19 UTC (permalink / raw)
To: Jing Zhang
Cc: KVM, KVMPPC, Paolo Bonzini, Peter Shier, Oliver Upton,
David Matlack, Ben Gardon
On Wed, Aug 11, 2021 at 3:53 PM Jing Zhang <jingzhangos@google.com> wrote:
>
> Hi Peter,
>
> On Tue, Aug 10, 2021 at 3:45 PM Peter Feiner <pfeiner@google.com> wrote:
> >
> > On Tue, Aug 10, 2021 at 3:32 PM Jing Zhang <jingzhangos@google.com> wrote:
> > >
> > > Add a generic VM stats dirty_pages to record the number of dirty pages
> > > reflected in dirty_bitmap at the moment.
> > >
> > > Original-by: Peter Feiner <pfeiner@google.com>
> > > Signed-off-by: Jing Zhang <jingzhangos@google.com>
> > > ---
> > > arch/powerpc/kvm/book3s_64_mmu_hv.c | 8 ++++++--
> > > arch/powerpc/kvm/book3s_64_mmu_radix.c | 1 +
> > > arch/powerpc/kvm/book3s_hv_rm_mmu.c | 1 +
> > > include/linux/kvm_host.h | 3 ++-
> > > include/linux/kvm_types.h | 1 +
> > > virt/kvm/kvm_main.c | 26 +++++++++++++++++++++++---
> > > 6 files changed, 34 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > > index c63e263312a4..e4aafa10efa1 100644
> > > --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > > +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > > @@ -1122,8 +1122,10 @@ long kvmppc_hv_get_dirty_log_hpt(struct kvm *kvm,
> > > * since we always put huge-page HPTEs in the rmap chain
> > > * corresponding to their page base address.
> > > */
> > > - if (npages)
> > > + if (npages) {
> > > set_dirty_bits(map, i, npages);
> > > + kvm->stat.generic.dirty_pages += npages;
> > > + }
> > > ++rmapp;
> > > }
> > > preempt_enable();
> > > @@ -1178,8 +1180,10 @@ void kvmppc_unpin_guest_page(struct kvm *kvm, void *va, unsigned long gpa,
> > > gfn = gpa >> PAGE_SHIFT;
> > > srcu_idx = srcu_read_lock(&kvm->srcu);
> > > memslot = gfn_to_memslot(kvm, gfn);
> > > - if (memslot && memslot->dirty_bitmap)
> > > + if (memslot && memslot->dirty_bitmap) {
> > > set_bit_le(gfn - memslot->base_gfn, memslot->dirty_bitmap);
> > > + ++kvm->stat.generic.dirty_pages;
> > > + }
> > > srcu_read_unlock(&kvm->srcu, srcu_idx);
> > > }
> > >
> > > diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > > index b5905ae4377c..3a6cb3854a44 100644
> > > --- a/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > > +++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > > @@ -1150,6 +1150,7 @@ long kvmppc_hv_get_dirty_log_radix(struct kvm *kvm,
> > > j = i + 1;
> > > if (npages) {
> > > set_dirty_bits(map, i, npages);
> > > + kvm->stat.generic.dirty_pages += npages;
> > > j = i + npages;
> > > }
> > > }
> > > diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > > index 632b2545072b..16806bc473fa 100644
> > > --- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > > +++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > > @@ -109,6 +109,7 @@ void kvmppc_update_dirty_map(const struct kvm_memory_slot *memslot,
> > > npages = (psize + PAGE_SIZE - 1) / PAGE_SIZE;
> > > gfn -= memslot->base_gfn;
> > > set_dirty_bits_atomic(memslot->dirty_bitmap, gfn, npages);
> > > + kvm->stat.generic.dirty_pages += npages;
> > > }
> > > EXPORT_SYMBOL_GPL(kvmppc_update_dirty_map);
> > >
> > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > > index f50bfcf225f0..1e8e66fb915b 100644
> > > --- a/include/linux/kvm_host.h
> > > +++ b/include/linux/kvm_host.h
> > > @@ -1421,7 +1421,8 @@ struct _kvm_stats_desc {
> > > KVM_STATS_BASE_POW10, -9)
> > >
> > > #define KVM_GENERIC_VM_STATS() \
> > > - STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush)
> > > + STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush), \
> > > + STATS_DESC_COUNTER(VM_GENERIC, dirty_pages)
> > >
> > > #define KVM_GENERIC_VCPU_STATS() \
> > > STATS_DESC_COUNTER(VCPU_GENERIC, halt_successful_poll), \
> > > diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
> > > index ed6a985c5680..6c05df00aebf 100644
> > > --- a/include/linux/kvm_types.h
> > > +++ b/include/linux/kvm_types.h
> > > @@ -78,6 +78,7 @@ struct kvm_mmu_memory_cache {
> > >
> > > struct kvm_vm_stat_generic {
> > > u64 remote_tlb_flush;
> > > + u64 dirty_pages;
> > > };
> > >
> > > struct kvm_vcpu_stat_generic {
> > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > > index a438a7a3774a..93f0ca2ea326 100644
> > > --- a/virt/kvm/kvm_main.c
> > > +++ b/virt/kvm/kvm_main.c
> > > @@ -1228,6 +1228,19 @@ static int kvm_alloc_dirty_bitmap(struct kvm_memory_slot *memslot)
> > > return 0;
> > > }
> > >
> > > +static inline unsigned long hweight_dirty_bitmap(
> > > + struct kvm_memory_slot *memslot)
> > > +{
> > > + unsigned long i;
> > > + unsigned long count = 0;
> > > + unsigned long n = kvm_dirty_bitmap_bytes(memslot);
> > > +
> > > + for (i = 0; i < n / sizeof(long); ++i)
> > > + count += hweight_long(memslot->dirty_bitmap[i]);
> > > +
> > > + return count;
> > > +}
> > > +
> > > /*
> > > * Delete a memslot by decrementing the number of used slots and shifting all
> > > * other entries in the array forward one spot.
> > > @@ -1612,6 +1625,7 @@ static int kvm_delete_memslot(struct kvm *kvm,
> > > if (r)
> > > return r;
> > >
> > > + kvm->stat.generic.dirty_pages -= hweight_dirty_bitmap(old);
> > > kvm_free_memslot(kvm, old);
> > > return 0;
> > > }
> > > @@ -1733,8 +1747,10 @@ int __kvm_set_memory_region(struct kvm *kvm,
> > > if (r)
> > > goto out_bitmap;
> > >
> > > - if (old.dirty_bitmap && !new.dirty_bitmap)
> > > + if (old.dirty_bitmap && !new.dirty_bitmap) {
> > > + kvm->stat.generic.dirty_pages -= hweight_dirty_bitmap(&old);
> > > kvm_destroy_dirty_bitmap(&old);
> > > + }
> > > return 0;
> > >
> > > out_bitmap:
> > > @@ -1895,6 +1911,7 @@ static int kvm_get_dirty_log_protect(struct kvm *kvm, struct kvm_dirty_log *log)
> > > offset = i * BITS_PER_LONG;
> > > kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot,
> > > offset, mask);
> > > + kvm->stat.generic.dirty_pages -= hweight_long(mask);
> > > }
> > > KVM_MMU_UNLOCK(kvm);
> > > }
> > > @@ -2012,6 +2029,7 @@ static int kvm_clear_dirty_log_protect(struct kvm *kvm,
> > > flush = true;
> > > kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot,
> > > offset, mask);
> > > + kvm->stat.generic.dirty_pages -= hweight_long(mask);
> > > }
> > > }
> > > KVM_MMU_UNLOCK(kvm);
> > > @@ -3062,11 +3080,13 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
> > > unsigned long rel_gfn = gfn - memslot->base_gfn;
> > > u32 slot = (memslot->as_id << 16) | memslot->id;
> > >
> > > - if (kvm->dirty_ring_size)
> > > + if (kvm->dirty_ring_size) {
> > > kvm_dirty_ring_push(kvm_dirty_ring_get(kvm),
> > > slot, rel_gfn);
> > > - else
> > > + } else {
> > > set_bit_le(rel_gfn, memslot->dirty_bitmap);
> > > + ++kvm->stat.generic.dirty_pages;
> >
> > Couple of problems here:
> >
> > - Calls to mark_page_dirty_in_slot aren't serialized by the mmu_lock,
> > so these updates will race.
> > - The page might already be dirty in the bitmap, so you're potentially
> > double counting here
> >
> > You can fix both of these problems by changing set_bit_le to a
> > test_and_set_bit_le (might not be the function name -- but you get the
> > idea) and conditionally incrementing dirty_pages.
> >
> >
> Since we need to consider the dirty ring situation and it is not
> trivial to check if the page is already in the dirty ring, I'll not
> change set_bit_le. Anyway, we are going to define the stat
> "dirty_pages" as dirtied pages in the life cycle of a VM, we only care
> about its growth rate for your use case.
To be clear, I don't have a use case anymore. It was years ago that I
came up with the idea of doing a kind of light memory dirty tracking
to predict how expensive pre-copy migration was going to be. However,
I didn't follow through on the idea; so if this never saw the light of
day in an upstream kernel, I'd be no worse for wear.
With that said, the use case I imagined did require knowing how many
pages were in the dirty bitmap at any moment. The number of pages
dirtied across the lifetime of the VM wouldn't have served that
purpose. So unless you have a use case for this new stat that's been
thought through end-to-end, I would suggest just dropping this patch
here (and from Google's kernel fork as well).
> I'll send out another version of the patch and we'll see if it is good enough.
> > > + }
> > > }
> > > }
> > > EXPORT_SYMBOL_GPL(mark_page_dirty_in_slot);
> > >
> > > base-commit: d0732b0f8884d9cc0eca0082bbaef043f3fef7fb
> > > --
> > > 2.32.0.605.g8dce9f2422-goog
> > >
>
> Thanks,
> Jing
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: stats: Add VM dirty_pages stats for the number of dirty pages
2021-08-11 23:19 ` Peter Feiner
@ 2021-08-11 23:25 ` Jing Zhang
-1 siblings, 0 replies; 22+ messages in thread
From: Jing Zhang @ 2021-08-11 23:25 UTC (permalink / raw)
To: Peter Feiner
Cc: KVM, KVMPPC, Paolo Bonzini, Peter Shier, Oliver Upton,
David Matlack, Ben Gardon
On Wed, Aug 11, 2021 at 4:19 PM Peter Feiner <pfeiner@google.com> wrote:
>
> On Wed, Aug 11, 2021 at 3:53 PM Jing Zhang <jingzhangos@google.com> wrote:
> >
> > Hi Peter,
> >
> > On Tue, Aug 10, 2021 at 3:45 PM Peter Feiner <pfeiner@google.com> wrote:
> > >
> > > On Tue, Aug 10, 2021 at 3:32 PM Jing Zhang <jingzhangos@google.com> wrote:
> > > >
> > > > Add a generic VM stats dirty_pages to record the number of dirty pages
> > > > reflected in dirty_bitmap at the moment.
> > > >
> > > > Original-by: Peter Feiner <pfeiner@google.com>
> > > > Signed-off-by: Jing Zhang <jingzhangos@google.com>
> > > > ---
> > > > arch/powerpc/kvm/book3s_64_mmu_hv.c | 8 ++++++--
> > > > arch/powerpc/kvm/book3s_64_mmu_radix.c | 1 +
> > > > arch/powerpc/kvm/book3s_hv_rm_mmu.c | 1 +
> > > > include/linux/kvm_host.h | 3 ++-
> > > > include/linux/kvm_types.h | 1 +
> > > > virt/kvm/kvm_main.c | 26 +++++++++++++++++++++++---
> > > > 6 files changed, 34 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > > > index c63e263312a4..e4aafa10efa1 100644
> > > > --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > > > +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > > > @@ -1122,8 +1122,10 @@ long kvmppc_hv_get_dirty_log_hpt(struct kvm *kvm,
> > > > * since we always put huge-page HPTEs in the rmap chain
> > > > * corresponding to their page base address.
> > > > */
> > > > - if (npages)
> > > > + if (npages) {
> > > > set_dirty_bits(map, i, npages);
> > > > + kvm->stat.generic.dirty_pages += npages;
> > > > + }
> > > > ++rmapp;
> > > > }
> > > > preempt_enable();
> > > > @@ -1178,8 +1180,10 @@ void kvmppc_unpin_guest_page(struct kvm *kvm, void *va, unsigned long gpa,
> > > > gfn = gpa >> PAGE_SHIFT;
> > > > srcu_idx = srcu_read_lock(&kvm->srcu);
> > > > memslot = gfn_to_memslot(kvm, gfn);
> > > > - if (memslot && memslot->dirty_bitmap)
> > > > + if (memslot && memslot->dirty_bitmap) {
> > > > set_bit_le(gfn - memslot->base_gfn, memslot->dirty_bitmap);
> > > > + ++kvm->stat.generic.dirty_pages;
> > > > + }
> > > > srcu_read_unlock(&kvm->srcu, srcu_idx);
> > > > }
> > > >
> > > > diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > > > index b5905ae4377c..3a6cb3854a44 100644
> > > > --- a/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > > > +++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > > > @@ -1150,6 +1150,7 @@ long kvmppc_hv_get_dirty_log_radix(struct kvm *kvm,
> > > > j = i + 1;
> > > > if (npages) {
> > > > set_dirty_bits(map, i, npages);
> > > > + kvm->stat.generic.dirty_pages += npages;
> > > > j = i + npages;
> > > > }
> > > > }
> > > > diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > > > index 632b2545072b..16806bc473fa 100644
> > > > --- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > > > +++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > > > @@ -109,6 +109,7 @@ void kvmppc_update_dirty_map(const struct kvm_memory_slot *memslot,
> > > > npages = (psize + PAGE_SIZE - 1) / PAGE_SIZE;
> > > > gfn -= memslot->base_gfn;
> > > > set_dirty_bits_atomic(memslot->dirty_bitmap, gfn, npages);
> > > > + kvm->stat.generic.dirty_pages += npages;
> > > > }
> > > > EXPORT_SYMBOL_GPL(kvmppc_update_dirty_map);
> > > >
> > > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > > > index f50bfcf225f0..1e8e66fb915b 100644
> > > > --- a/include/linux/kvm_host.h
> > > > +++ b/include/linux/kvm_host.h
> > > > @@ -1421,7 +1421,8 @@ struct _kvm_stats_desc {
> > > > KVM_STATS_BASE_POW10, -9)
> > > >
> > > > #define KVM_GENERIC_VM_STATS() \
> > > > - STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush)
> > > > + STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush), \
> > > > + STATS_DESC_COUNTER(VM_GENERIC, dirty_pages)
> > > >
> > > > #define KVM_GENERIC_VCPU_STATS() \
> > > > STATS_DESC_COUNTER(VCPU_GENERIC, halt_successful_poll), \
> > > > diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
> > > > index ed6a985c5680..6c05df00aebf 100644
> > > > --- a/include/linux/kvm_types.h
> > > > +++ b/include/linux/kvm_types.h
> > > > @@ -78,6 +78,7 @@ struct kvm_mmu_memory_cache {
> > > >
> > > > struct kvm_vm_stat_generic {
> > > > u64 remote_tlb_flush;
> > > > + u64 dirty_pages;
> > > > };
> > > >
> > > > struct kvm_vcpu_stat_generic {
> > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > > > index a438a7a3774a..93f0ca2ea326 100644
> > > > --- a/virt/kvm/kvm_main.c
> > > > +++ b/virt/kvm/kvm_main.c
> > > > @@ -1228,6 +1228,19 @@ static int kvm_alloc_dirty_bitmap(struct kvm_memory_slot *memslot)
> > > > return 0;
> > > > }
> > > >
> > > > +static inline unsigned long hweight_dirty_bitmap(
> > > > + struct kvm_memory_slot *memslot)
> > > > +{
> > > > + unsigned long i;
> > > > + unsigned long count = 0;
> > > > + unsigned long n = kvm_dirty_bitmap_bytes(memslot);
> > > > +
> > > > + for (i = 0; i < n / sizeof(long); ++i)
> > > > + count += hweight_long(memslot->dirty_bitmap[i]);
> > > > +
> > > > + return count;
> > > > +}
> > > > +
> > > > /*
> > > > * Delete a memslot by decrementing the number of used slots and shifting all
> > > > * other entries in the array forward one spot.
> > > > @@ -1612,6 +1625,7 @@ static int kvm_delete_memslot(struct kvm *kvm,
> > > > if (r)
> > > > return r;
> > > >
> > > > + kvm->stat.generic.dirty_pages -= hweight_dirty_bitmap(old);
> > > > kvm_free_memslot(kvm, old);
> > > > return 0;
> > > > }
> > > > @@ -1733,8 +1747,10 @@ int __kvm_set_memory_region(struct kvm *kvm,
> > > > if (r)
> > > > goto out_bitmap;
> > > >
> > > > - if (old.dirty_bitmap && !new.dirty_bitmap)
> > > > + if (old.dirty_bitmap && !new.dirty_bitmap) {
> > > > + kvm->stat.generic.dirty_pages -= hweight_dirty_bitmap(&old);
> > > > kvm_destroy_dirty_bitmap(&old);
> > > > + }
> > > > return 0;
> > > >
> > > > out_bitmap:
> > > > @@ -1895,6 +1911,7 @@ static int kvm_get_dirty_log_protect(struct kvm *kvm, struct kvm_dirty_log *log)
> > > > offset = i * BITS_PER_LONG;
> > > > kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot,
> > > > offset, mask);
> > > > + kvm->stat.generic.dirty_pages -= hweight_long(mask);
> > > > }
> > > > KVM_MMU_UNLOCK(kvm);
> > > > }
> > > > @@ -2012,6 +2029,7 @@ static int kvm_clear_dirty_log_protect(struct kvm *kvm,
> > > > flush = true;
> > > > kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot,
> > > > offset, mask);
> > > > + kvm->stat.generic.dirty_pages -= hweight_long(mask);
> > > > }
> > > > }
> > > > KVM_MMU_UNLOCK(kvm);
> > > > @@ -3062,11 +3080,13 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
> > > > unsigned long rel_gfn = gfn - memslot->base_gfn;
> > > > u32 slot = (memslot->as_id << 16) | memslot->id;
> > > >
> > > > - if (kvm->dirty_ring_size)
> > > > + if (kvm->dirty_ring_size) {
> > > > kvm_dirty_ring_push(kvm_dirty_ring_get(kvm),
> > > > slot, rel_gfn);
> > > > - else
> > > > + } else {
> > > > set_bit_le(rel_gfn, memslot->dirty_bitmap);
> > > > + ++kvm->stat.generic.dirty_pages;
> > >
> > > Couple of problems here:
> > >
> > > - Calls to mark_page_dirty_in_slot aren't serialized by the mmu_lock,
> > > so these updates will race.
> > > - The page might already be dirty in the bitmap, so you're potentially
> > > double counting here
> > >
> > > You can fix both of these problems by changing set_bit_le to a
> > > test_and_set_bit_le (might not be the function name -- but you get the
> > > idea) and conditionally incrementing dirty_pages.
> > >
> > >
> > Since we need to consider the dirty ring situation and it is not
> > trivial to check if the page is already in the dirty ring, I'll not
> > change set_bit_le. Anyway, we are going to define the stat
> > "dirty_pages" as dirtied pages in the life cycle of a VM, we only care
> > about its growth rate for your use case.
>
> To be clear, I don't have a use case anymore. It was years ago that I
> came up with the idea of doing a kind of light memory dirty tracking
> to predict how expensive pre-copy migration was going to be. However,
> I didn't follow through on the idea; so if this never saw the light of
> day in an upstream kernel, I'd be no worse for wear.
>
> With that said, the use case I imagined did require knowing how many
> pages were in the dirty bitmap at any moment. The number of pages
> dirtied across the lifetime of the VM wouldn't have served that
> purpose. So unless you have a use case for this new stat that's been
> thought through end-to-end, I would suggest just dropping this patch
> here (and from Google's kernel fork as well).
Thanks for the clarification, Peter.
I will send out another patch anyway according to Oliver's idea. We'll
see how we can proceed from there.
>
>
>
> > I'll send out another version of the patch and we'll see if it is good enough.
> > > > + }
> > > > }
> > > > }
> > > > EXPORT_SYMBOL_GPL(mark_page_dirty_in_slot);
> > > >
> > > > base-commit: d0732b0f8884d9cc0eca0082bbaef043f3fef7fb
> > > > --
> > > > 2.32.0.605.g8dce9f2422-goog
> > > >
> >
> > Thanks,
> > Jing
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: stats: Add VM dirty_pages stats for the number of dirty pages
@ 2021-08-11 23:25 ` Jing Zhang
0 siblings, 0 replies; 22+ messages in thread
From: Jing Zhang @ 2021-08-11 23:25 UTC (permalink / raw)
To: Peter Feiner
Cc: KVM, KVMPPC, Paolo Bonzini, Peter Shier, Oliver Upton,
David Matlack, Ben Gardon
On Wed, Aug 11, 2021 at 4:19 PM Peter Feiner <pfeiner@google.com> wrote:
>
> On Wed, Aug 11, 2021 at 3:53 PM Jing Zhang <jingzhangos@google.com> wrote:
> >
> > Hi Peter,
> >
> > On Tue, Aug 10, 2021 at 3:45 PM Peter Feiner <pfeiner@google.com> wrote:
> > >
> > > On Tue, Aug 10, 2021 at 3:32 PM Jing Zhang <jingzhangos@google.com> wrote:
> > > >
> > > > Add a generic VM stats dirty_pages to record the number of dirty pages
> > > > reflected in dirty_bitmap at the moment.
> > > >
> > > > Original-by: Peter Feiner <pfeiner@google.com>
> > > > Signed-off-by: Jing Zhang <jingzhangos@google.com>
> > > > ---
> > > > arch/powerpc/kvm/book3s_64_mmu_hv.c | 8 ++++++--
> > > > arch/powerpc/kvm/book3s_64_mmu_radix.c | 1 +
> > > > arch/powerpc/kvm/book3s_hv_rm_mmu.c | 1 +
> > > > include/linux/kvm_host.h | 3 ++-
> > > > include/linux/kvm_types.h | 1 +
> > > > virt/kvm/kvm_main.c | 26 +++++++++++++++++++++++---
> > > > 6 files changed, 34 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > > > index c63e263312a4..e4aafa10efa1 100644
> > > > --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > > > +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > > > @@ -1122,8 +1122,10 @@ long kvmppc_hv_get_dirty_log_hpt(struct kvm *kvm,
> > > > * since we always put huge-page HPTEs in the rmap chain
> > > > * corresponding to their page base address.
> > > > */
> > > > - if (npages)
> > > > + if (npages) {
> > > > set_dirty_bits(map, i, npages);
> > > > + kvm->stat.generic.dirty_pages += npages;
> > > > + }
> > > > ++rmapp;
> > > > }
> > > > preempt_enable();
> > > > @@ -1178,8 +1180,10 @@ void kvmppc_unpin_guest_page(struct kvm *kvm, void *va, unsigned long gpa,
> > > > gfn = gpa >> PAGE_SHIFT;
> > > > srcu_idx = srcu_read_lock(&kvm->srcu);
> > > > memslot = gfn_to_memslot(kvm, gfn);
> > > > - if (memslot && memslot->dirty_bitmap)
> > > > + if (memslot && memslot->dirty_bitmap) {
> > > > set_bit_le(gfn - memslot->base_gfn, memslot->dirty_bitmap);
> > > > + ++kvm->stat.generic.dirty_pages;
> > > > + }
> > > > srcu_read_unlock(&kvm->srcu, srcu_idx);
> > > > }
> > > >
> > > > diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > > > index b5905ae4377c..3a6cb3854a44 100644
> > > > --- a/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > > > +++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > > > @@ -1150,6 +1150,7 @@ long kvmppc_hv_get_dirty_log_radix(struct kvm *kvm,
> > > > j = i + 1;
> > > > if (npages) {
> > > > set_dirty_bits(map, i, npages);
> > > > + kvm->stat.generic.dirty_pages += npages;
> > > > j = i + npages;
> > > > }
> > > > }
> > > > diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > > > index 632b2545072b..16806bc473fa 100644
> > > > --- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > > > +++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > > > @@ -109,6 +109,7 @@ void kvmppc_update_dirty_map(const struct kvm_memory_slot *memslot,
> > > > npages = (psize + PAGE_SIZE - 1) / PAGE_SIZE;
> > > > gfn -= memslot->base_gfn;
> > > > set_dirty_bits_atomic(memslot->dirty_bitmap, gfn, npages);
> > > > + kvm->stat.generic.dirty_pages += npages;
> > > > }
> > > > EXPORT_SYMBOL_GPL(kvmppc_update_dirty_map);
> > > >
> > > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > > > index f50bfcf225f0..1e8e66fb915b 100644
> > > > --- a/include/linux/kvm_host.h
> > > > +++ b/include/linux/kvm_host.h
> > > > @@ -1421,7 +1421,8 @@ struct _kvm_stats_desc {
> > > > KVM_STATS_BASE_POW10, -9)
> > > >
> > > > #define KVM_GENERIC_VM_STATS() \
> > > > - STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush)
> > > > + STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush), \
> > > > + STATS_DESC_COUNTER(VM_GENERIC, dirty_pages)
> > > >
> > > > #define KVM_GENERIC_VCPU_STATS() \
> > > > STATS_DESC_COUNTER(VCPU_GENERIC, halt_successful_poll), \
> > > > diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
> > > > index ed6a985c5680..6c05df00aebf 100644
> > > > --- a/include/linux/kvm_types.h
> > > > +++ b/include/linux/kvm_types.h
> > > > @@ -78,6 +78,7 @@ struct kvm_mmu_memory_cache {
> > > >
> > > > struct kvm_vm_stat_generic {
> > > > u64 remote_tlb_flush;
> > > > + u64 dirty_pages;
> > > > };
> > > >
> > > > struct kvm_vcpu_stat_generic {
> > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > > > index a438a7a3774a..93f0ca2ea326 100644
> > > > --- a/virt/kvm/kvm_main.c
> > > > +++ b/virt/kvm/kvm_main.c
> > > > @@ -1228,6 +1228,19 @@ static int kvm_alloc_dirty_bitmap(struct kvm_memory_slot *memslot)
> > > > return 0;
> > > > }
> > > >
> > > > +static inline unsigned long hweight_dirty_bitmap(
> > > > + struct kvm_memory_slot *memslot)
> > > > +{
> > > > + unsigned long i;
> > > > + unsigned long count = 0;
> > > > + unsigned long n = kvm_dirty_bitmap_bytes(memslot);
> > > > +
> > > > + for (i = 0; i < n / sizeof(long); ++i)
> > > > + count += hweight_long(memslot->dirty_bitmap[i]);
> > > > +
> > > > + return count;
> > > > +}
> > > > +
> > > > /*
> > > > * Delete a memslot by decrementing the number of used slots and shifting all
> > > > * other entries in the array forward one spot.
> > > > @@ -1612,6 +1625,7 @@ static int kvm_delete_memslot(struct kvm *kvm,
> > > > if (r)
> > > > return r;
> > > >
> > > > + kvm->stat.generic.dirty_pages -= hweight_dirty_bitmap(old);
> > > > kvm_free_memslot(kvm, old);
> > > > return 0;
> > > > }
> > > > @@ -1733,8 +1747,10 @@ int __kvm_set_memory_region(struct kvm *kvm,
> > > > if (r)
> > > > goto out_bitmap;
> > > >
> > > > - if (old.dirty_bitmap && !new.dirty_bitmap)
> > > > + if (old.dirty_bitmap && !new.dirty_bitmap) {
> > > > + kvm->stat.generic.dirty_pages -= hweight_dirty_bitmap(&old);
> > > > kvm_destroy_dirty_bitmap(&old);
> > > > + }
> > > > return 0;
> > > >
> > > > out_bitmap:
> > > > @@ -1895,6 +1911,7 @@ static int kvm_get_dirty_log_protect(struct kvm *kvm, struct kvm_dirty_log *log)
> > > > offset = i * BITS_PER_LONG;
> > > > kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot,
> > > > offset, mask);
> > > > + kvm->stat.generic.dirty_pages -= hweight_long(mask);
> > > > }
> > > > KVM_MMU_UNLOCK(kvm);
> > > > }
> > > > @@ -2012,6 +2029,7 @@ static int kvm_clear_dirty_log_protect(struct kvm *kvm,
> > > > flush = true;
> > > > kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot,
> > > > offset, mask);
> > > > + kvm->stat.generic.dirty_pages -= hweight_long(mask);
> > > > }
> > > > }
> > > > KVM_MMU_UNLOCK(kvm);
> > > > @@ -3062,11 +3080,13 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
> > > > unsigned long rel_gfn = gfn - memslot->base_gfn;
> > > > u32 slot = (memslot->as_id << 16) | memslot->id;
> > > >
> > > > - if (kvm->dirty_ring_size)
> > > > + if (kvm->dirty_ring_size) {
> > > > kvm_dirty_ring_push(kvm_dirty_ring_get(kvm),
> > > > slot, rel_gfn);
> > > > - else
> > > > + } else {
> > > > set_bit_le(rel_gfn, memslot->dirty_bitmap);
> > > > + ++kvm->stat.generic.dirty_pages;
> > >
> > > Couple of problems here:
> > >
> > > - Calls to mark_page_dirty_in_slot aren't serialized by the mmu_lock,
> > > so these updates will race.
> > > - The page might already be dirty in the bitmap, so you're potentially
> > > double counting here
> > >
> > > You can fix both of these problems by changing set_bit_le to a
> > > test_and_set_bit_le (might not be the function name -- but you get the
> > > idea) and conditionally incrementing dirty_pages.
> > >
> > >
> > Since we need to consider the dirty ring situation and it is not
> > trivial to check if the page is already in the dirty ring, I'll not
> > change set_bit_le. Anyway, we are going to define the stat
> > "dirty_pages" as dirtied pages in the life cycle of a VM, we only care
> > about its growth rate for your use case.
>
> To be clear, I don't have a use case anymore. It was years ago that I
> came up with the idea of doing a kind of light memory dirty tracking
> to predict how expensive pre-copy migration was going to be. However,
> I didn't follow through on the idea; so if this never saw the light of
> day in an upstream kernel, I'd be no worse for wear.
>
> With that said, the use case I imagined did require knowing how many
> pages were in the dirty bitmap at any moment. The number of pages
> dirtied across the lifetime of the VM wouldn't have served that
> purpose. So unless you have a use case for this new stat that's been
> thought through end-to-end, I would suggest just dropping this patch
> here (and from Google's kernel fork as well).
Thanks for the clarification, Peter.
I will send out another patch anyway according to Oliver's idea. We'll
see how we can proceed from there.
>
>
>
> > I'll send out another version of the patch and we'll see if it is good enough.
> > > > + }
> > > > }
> > > > }
> > > > EXPORT_SYMBOL_GPL(mark_page_dirty_in_slot);
> > > >
> > > > base-commit: d0732b0f8884d9cc0eca0082bbaef043f3fef7fb
> > > > --
> > > > 2.32.0.605.g8dce9f2422-goog
> > > >
> >
> > Thanks,
> > Jing
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2021-08-11 23:26 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-10 22:32 [PATCH] KVM: stats: Add VM dirty_pages stats for the number of dirty pages Jing Zhang
2021-08-10 22:44 ` Peter Feiner
2021-08-10 22:44 ` Peter Feiner
2021-08-10 22:51 ` Jing Zhang
2021-08-10 22:51 ` Jing Zhang
2021-08-11 22:52 ` Jing Zhang
2021-08-11 22:52 ` Jing Zhang
2021-08-11 23:19 ` Peter Feiner
2021-08-11 23:19 ` Peter Feiner
2021-08-11 23:25 ` Jing Zhang
2021-08-11 23:25 ` Jing Zhang
2021-08-10 22:56 ` Oliver Upton
2021-08-10 22:56 ` Oliver Upton
2021-08-10 23:15 ` Peter Feiner
2021-08-10 23:15 ` Peter Feiner
2021-08-10 23:26 ` Oliver Upton
2021-08-10 23:26 ` Oliver Upton
2021-08-11 10:20 ` Paolo Bonzini
2021-08-11 10:20 ` Paolo Bonzini
2021-08-11 20:14 ` Jing Zhang
2021-08-11 20:14 ` Jing Zhang
2021-08-11 12:26 ` kernel test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.