Hi Jing,
On Tue, Aug 10, 2021 at 3:32 PM Jing Zhang <jingzhangos@xxxxxxxxxx> 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@xxxxxxxxxx>
> Signed-off-by: Jing Zhang <jingzhangos@xxxxxxxxxx>
> ---
> 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
>
