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.and...@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:
