Re: [PATCH v2 2/4] migration/calc-dirty-rate: detailed stats in sampling mode

[gudkov.andrei]

On Thu, May 11, 2023 at 08:14:29AM +0200, Markus Armbruster wrote:
> Andrei Gudkov via <qemu-devel@nongnu.org> writes:
> 
> > Collect number of dirty pages for progresseively increasing time
> > periods starting with 125ms up to number of seconds specified with
> > calc-dirty-rate. Report through qmp and hmp: 1) vector of dirty page
> > measurements, 2) page size, 3) total number of VM pages, 4) number
> > of sampled pages.
> >
> > Signed-off-by: Andrei Gudkov <gudkov.andrei@huawei.com>
> 
> [...]
> 
> > diff --git a/qapi/migration.json b/qapi/migration.json
> > index 2c35b7b9cf..f818f51e0e 100644
> > --- a/qapi/migration.json
> > +++ b/qapi/migration.json
> > @@ -1805,6 +1805,22 @@
>    ##
>    # @DirtyRateInfo:
>    #
>    # Information about current dirty page rate of vm.
>    #
>    # @dirty-rate: an estimate of the dirty page rate of the VM in units
>    #     of MB/s, present only when estimating the rate has completed.
>    #
>    # @status: status containing dirtyrate query status includes
>    #     'unstarted' or 'measuring' or 'measured'
> 
> Not this patch's fault, but here goes anyway:
> 
> 0. "dirtyrate" isn't a word.  Spell it "dirty rate".  Many more
>    instances elsewhere.
> 
> 1. "status containing status"... what has the poor English language done
>    to us that we torture it so?
> 
> 2. "includes 'unstarted' or 'measuring' or 'measured' is confusing and
>    entirely redundant with the type.  @status doesn't "include" these,
>    these are the possible values, and all of them.
> 
> Suggest:
> 
>      @status: dirty rate measuring status
> 
> I do understand how difficult writing good English is for non-native
> speakers.  This is mainly a failure of patch review.
> 
>    #
>    # @start-time: start time in units of second for calculation
>    #
>    # @calc-time: time in units of second for sample dirty pages
>    #
>    # @sample-pages: page count per GB for sample dirty pages the default
>    #     value is 512 (since 6.1)
>    #
>    # @mode: mode containing method of calculate dirtyrate includes
>    #     'page-sampling' and 'dirty-ring' (Since 6.2)
> 
> Still not this patch's fault:
> 
> 1. "mode containing method": more torture :)
> 
> 2. "includes 'page-sampling' and 'dirty-ring'" is confusing.
> 
>    When it was added in commit 0e21bf24608, it was confusing and
>    redundant like the text for @status above.
> 
>    Then commit 826b8bc80cb added value 'dirty-bitmap' without updating
>    the member doc here.
> 
> Suggest:
> 
>      @mode: dirty rate measuring mode
> 
>    #
>    # @vcpu-dirty-rate: dirtyrate for each vcpu if dirty-ring mode
>    #     specified (Since 6.2)
>    #
> > +# @page-size: page size in bytes (since 8.1)
> > +#
> > +# @n-total-pages: [page-sampling] total number of VM pages (since 8.1)
> > +#
> > +# @n-sampled-pages: [page-sampling] number of sampled VM pages (since 8.1)
> > +#
> > +# @periods: [page-sampling] array of time periods expressed in milliseconds
> > +#           for which dirty-sample measurements were collected (since 8.1)
> > +#
> > +# @n-dirty-pages: [page-sampling] number of pages among all sampled pages
> > +#                 that were observed as changed during respective time 
> > period.
> > +#                 i-th element of this array corresponds to the i-th 
> > element
> > +#                 of the @periods array, i.e. @n-dirty-pages[i] is the 
> > number
> > +#                 of dirtied pages during period of @periods[i] 
> > milliseconds
> > +#                 after the initiation of calc-dirty-rate (since 8.1)
> > +#
> 
> Changed doc comment conventions landed yesterday (merge commit
> 568992e3440).  Please format like this:
> 
>    # @page-size: page size in bytes (since 8.1)
>    #
>    # @n-total-pages: [page-sampling] total number of VM pages (since 8.1)
>    #
>    # @n-sampled-pages: [page-sampling] number of sampled VM pages (since
>    #     8.1)
>    #
>    # @n-zero-pages: [page-sampling] number of observed all-zero pages
>    #     among all sampled pages (since 8.1)
>    #
>    # @periods: [page-sampling] array of time periods expressed in
>    #     milliseconds for which dirty-sample measurements were collected
>    #     (since 8.1)
>    #
>    # @n-dirty-pages: [page-sampling] number of pages among all sampled
>    #     pages that were observed as changed during respective time
>    #     period.  i-th element of this array corresponds to the i-th
>    #     element of the @periods array, i.e. @n-dirty-pages[i] is the
>    #     number of dirtied pages during period of @periods[i]
>    #     milliseconds after the initiation of calc-dirty-rate (since 8.1)
> 
> The meaning of "[page-sampling]" is unclear.  What are you trying to
> express?

There are two different measurements modes: page-sampling and dirty-ring.
They gather different sets of metrics. All the metrics above are
available only in page-sampling mode.

> 
> For better or worse, we try to avoid abbreviations in QMP.  The "n-"
> prefix is one.  What does it stand for?

This is the number of respective objects. I can replace it with something like
"sampled-pages-count", "zero-pages-count", etc at the cost of longer names.

> 
> It's quite unclear how all these numbers relate to each other.  What's
> the difference between @n-sampled-pages and @sample-pages?  I think
> we're missing an overview of the dirty rate measurement feature.

This looks like an easy thing to do. I will add some docs to
@calc-dirty-rate related to page-sampling mode.

> 
> >  # Since: 5.2
> >  ##
> >  { 'struct': 'DirtyRateInfo',
> > @@ -1814,7 +1830,13 @@
> >             'calc-time': 'int64',
> >             'sample-pages': 'uint64',
> >             'mode': 'DirtyRateMeasureMode',
> > -           '*vcpu-dirty-rate': [ 'DirtyRateVcpu' ] } }
> > +           '*vcpu-dirty-rate': [ 'DirtyRateVcpu' ],
> > +           'page-size': 'int64',
> 
> Shouldn't this be of type 'size'?
> 
> > +           '*n-total-pages': 'int64',
> > +           '*n-sampled-pages': 'int64',
> > +           '*periods': ['int64'],
> > +           '*n-dirty-pages': ['int64'] } }
> 
> 'uint64', like @sample-pages?
> 
> > +
> >  
> >  ##
> >  # @calc-dirty-rate: