| Message ID | 20210813195433.2555924-1-jingzhangos@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] KVM: stats: Add VM stat for the cumulative number of dirtied pages | expand |
On Fri, Aug 13, 2021, Jing Zhang wrote: > A per VCPU stat dirtied_pages is added to record the number of dirtied > pages in the life cycle of a VM. > The growth rate of this stat is a good indicator during the process of > live migrations. The exact number of dirty pages at the moment doesn't > matter. That's why we define dirtied_pages as a cumulative counter instead > of an instantaneous one. > > Original-by: Peter Feiner <pfeiner@google.com> > Suggested-by: Oliver Upton <oupton@google.com> > Reviewed-by: Oliver Upton <oupton@google.com> > Signed-off-by: Jing Zhang <jingzhangos@google.com> > --- > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 3e67c93ca403..8c673198cc83 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -3075,6 +3075,8 @@ void mark_page_dirty_in_slot(struct kvm *kvm, > struct kvm_memory_slot *memslot, > gfn_t gfn) > { > + struct kvm_vcpu *vcpu = kvm_get_running_vcpu(); > + > if (memslot && kvm_slot_dirty_track_enabled(memslot)) { > unsigned long rel_gfn = gfn - memslot->base_gfn; > u32 slot = (memslot->as_id << 16) | memslot->id; > @@ -3084,6 +3086,9 @@ void mark_page_dirty_in_slot(struct kvm *kvm, > slot, rel_gfn); > else > set_bit_le(rel_gfn, memslot->dirty_bitmap); > + > + if (vcpu) > + ++vcpu->stat.generic.dirtied_pages; I agree with Peter that this is a solution looking for a problem, and the stat is going to be confusing because it's only active if dirty logging is enabled. For Oliver's debug use case, it will require userspace to coordinate reaping the dirty bitmap/ring with the stats, otherwise there's no baseline, e.g. the number of dirtied pages will scale with how frequently userspace is clearing dirty bits. At that point, userspace can do the whole thing itself, e.g. with a dirty ring it's trivial to do "dirtied_pages += ring->dirty_index - ring->reset_index". The traditional bitmap will be slower, but without additional userspace enabling the dirty logging dependency means this is mostly limited to live migration being in-progress. In that case, something in userspace needs to actually be processing the dirty pages, it should be easy for that something to keep a running count.
On Fri, Aug 13, 2021 at 5:19 PM Sean Christopherson <seanjc@google.com> wrote: > > On Fri, Aug 13, 2021, Jing Zhang wrote: > > A per VCPU stat dirtied_pages is added to record the number of dirtied > > pages in the life cycle of a VM. > > The growth rate of this stat is a good indicator during the process of > > live migrations. The exact number of dirty pages at the moment doesn't > > matter. That's why we define dirtied_pages as a cumulative counter instead > > of an instantaneous one. > > > > Original-by: Peter Feiner <pfeiner@google.com> > > Suggested-by: Oliver Upton <oupton@google.com> > > Reviewed-by: Oliver Upton <oupton@google.com> > > Signed-off-by: Jing Zhang <jingzhangos@google.com> > > --- > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > index 3e67c93ca403..8c673198cc83 100644 > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c > > @@ -3075,6 +3075,8 @@ void mark_page_dirty_in_slot(struct kvm *kvm, > > struct kvm_memory_slot *memslot, > > gfn_t gfn) > > { > > + struct kvm_vcpu *vcpu = kvm_get_running_vcpu(); > > + > > if (memslot && kvm_slot_dirty_track_enabled(memslot)) { > > unsigned long rel_gfn = gfn - memslot->base_gfn; > > u32 slot = (memslot->as_id << 16) | memslot->id; > > @@ -3084,6 +3086,9 @@ void mark_page_dirty_in_slot(struct kvm *kvm, > > slot, rel_gfn); > > else > > set_bit_le(rel_gfn, memslot->dirty_bitmap); > > + > > + if (vcpu) > > + ++vcpu->stat.generic.dirtied_pages; > > I agree with Peter that this is a solution looking for a problem, and the stat is > going to be confusing because it's only active if dirty logging is enabled. > > For Oliver's debug use case, it will require userspace to coordinate reaping the > dirty bitmap/ring with the stats, otherwise there's no baseline, e.g. the number > of dirtied pages will scale with how frequently userspace is clearing dirty bits. > > At that point, userspace can do the whole thing itself, e.g. with a dirty ring > it's trivial to do "dirtied_pages += ring->dirty_index - ring->reset_index". > The traditional bitmap will be slower, but without additional userspace enabling > the dirty logging dependency means this is mostly limited to live migration being > in-progress. In that case, something in userspace needs to actually be processing > the dirty pages, it should be easy for that something to keep a running count. Thanks Sean. Looks like it is not a bad idea to drop this change. Will do that. Jing
diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c index c63e263312a4..e06cd9c9a278 100644 --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c @@ -1112,6 +1112,7 @@ long kvmppc_hv_get_dirty_log_hpt(struct kvm *kvm, { unsigned long i; unsigned long *rmapp; + struct kvm_vcpu *vcpu = kvm_get_running_vcpu(); preempt_disable(); rmapp = memslot->arch.rmap; @@ -1122,8 +1123,11 @@ 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); + if (vcpu) + vcpu->stat.generic.dirtied_pages += npages; + } ++rmapp; } preempt_enable(); @@ -1164,6 +1168,7 @@ void *kvmppc_pin_guest_page(struct kvm *kvm, unsigned long gpa, void kvmppc_unpin_guest_page(struct kvm *kvm, void *va, unsigned long gpa, bool dirty) { + struct kvm_vcpu *vcpu = kvm_get_running_vcpu(); struct page *page = virt_to_page(va); struct kvm_memory_slot *memslot; unsigned long gfn; @@ -1178,8 +1183,11 @@ 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); + if (vcpu) + ++vcpu->stat.generic.dirtied_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..a07f143ff5ee 100644 --- a/arch/powerpc/kvm/book3s_64_mmu_radix.c +++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c @@ -1134,6 +1134,7 @@ static int kvm_radix_test_clear_dirty(struct kvm *kvm, long kvmppc_hv_get_dirty_log_radix(struct kvm *kvm, struct kvm_memory_slot *memslot, unsigned long *map) { + struct kvm_vcpu *vcpu = kvm_get_running_vcpu(); unsigned long i, j; int npages; @@ -1150,6 +1151,8 @@ long kvmppc_hv_get_dirty_log_radix(struct kvm *kvm, j = i + 1; if (npages) { set_dirty_bits(map, i, npages); + if (vcpu) + vcpu->stat.generic.dirtied_pages += npages; j = i + npages; } } diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index d447b21cdd73..c73d8bf74f9f 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -1459,7 +1459,8 @@ struct _kvm_stats_desc { STATS_DESC_LOGHIST_TIME_NSEC(VCPU_GENERIC, halt_poll_fail_hist, \ HALT_POLL_HIST_COUNT), \ STATS_DESC_LOGHIST_TIME_NSEC(VCPU_GENERIC, halt_wait_hist, \ - HALT_POLL_HIST_COUNT) + HALT_POLL_HIST_COUNT), \ + STATS_DESC_COUNTER(VCPU_GENERIC, dirtied_pages) extern struct dentry *kvm_debugfs_dir; diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h index de7fb5f364d8..949549f55187 100644 --- a/include/linux/kvm_types.h +++ b/include/linux/kvm_types.h @@ -93,6 +93,7 @@ struct kvm_vcpu_stat_generic { u64 halt_poll_success_hist[HALT_POLL_HIST_COUNT]; u64 halt_poll_fail_hist[HALT_POLL_HIST_COUNT]; u64 halt_wait_hist[HALT_POLL_HIST_COUNT]; + u64 dirtied_pages; }; #define KVM_STATS_NAME_SIZE 48 diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 3e67c93ca403..8c673198cc83 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -3075,6 +3075,8 @@ void mark_page_dirty_in_slot(struct kvm *kvm, struct kvm_memory_slot *memslot, gfn_t gfn) { + struct kvm_vcpu *vcpu = kvm_get_running_vcpu(); + if (memslot && kvm_slot_dirty_track_enabled(memslot)) { unsigned long rel_gfn = gfn - memslot->base_gfn; u32 slot = (memslot->as_id << 16) | memslot->id; @@ -3084,6 +3086,9 @@ void mark_page_dirty_in_slot(struct kvm *kvm, slot, rel_gfn); else set_bit_le(rel_gfn, memslot->dirty_bitmap); + + if (vcpu) + ++vcpu->stat.generic.dirtied_pages; } } EXPORT_SYMBOL_GPL(mark_page_dirty_in_slot);