Re: [PATCH v3] KVM: stats: Add VM stat for the cumulative number of dirtied pages

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 <[email protected]>
> Suggested-by: Oliver Upton <[email protected]>
> Reviewed-by: Oliver Upton <[email protected]>
> Signed-off-by: Jing Zhang <[email protected]>
> ---
> 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.