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

On Fri, Aug 13, 2021 at 5:19 PM Sean Christopherson <[email protected]> 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 <[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.
Thanks Sean. Looks like it is not a bad idea to drop this change. Will do that.

Jing