Re: [PATCH] qapi: better docs for calc-dirty-rate and friends

[Peter Xu <peterx@redhat.com>]

On Thu, May 25, 2023 at 03:08:35PM +0200, Markus Armbruster wrote:
> Andrei Gudkov <gudkov.andrei@huawei.com> writes:
> 
> > Rewrote calc-dirty-rate documentation. Briefly described
> > different modes of dirty page rate measurement. Added some
> > examples. Fixed obvious grammar errors.
> >
> > Signed-off-by: Andrei Gudkov <gudkov.andrei@huawei.com>
> > ---
> >  qapi/migration.json | 107 +++++++++++++++++++++++++++++++-------------
> >  1 file changed, 77 insertions(+), 30 deletions(-)
> >
> > diff --git a/qapi/migration.json b/qapi/migration.json
> > index 179af0c4d8..19b51444b5 100644
> > --- a/qapi/migration.json
> > +++ b/qapi/migration.json
> > @@ -1735,14 +1735,14 @@
> >  ##
> >  # @DirtyRateStatus:
> >  #
> > -# An enumeration of dirtyrate status.
> > +# Dirty page rate measurement status.
> >  #
> > -# @unstarted: the dirtyrate thread has not been started.
> > +# @unstarted: measuring thread has not been started yet
> >  #
> > -# @measuring: the dirtyrate thread is measuring.
> > +# @measuring: measuring thread is running
> >  #
> > -# @measured: the dirtyrate thread has measured and results are
> > -#     available.
> > +# @measured: dirty page rate is measured and the results are
> > +#     available
> >  #
> >  # Since: 5.2
> >  ##
> > @@ -1752,13 +1752,14 @@
> >  ##
> >  # @DirtyRateMeasureMode:
> >  #
> > -# An enumeration of mode of measuring dirtyrate.
> > +# Method used to measure dirty page rate.  Differences between
> > +# available methods are explained in @calc-dirty-rate.
> >  #
> > -# @page-sampling: calculate dirtyrate by sampling pages.
> > +# @page-sampling: use page sampling
> >  #
> > -# @dirty-ring: calculate dirtyrate by dirty ring.
> > +# @dirty-ring: use dirty ring
> >  #
> > -# @dirty-bitmap: calculate dirtyrate by dirty bitmap.
> > +# @dirty-bitmap: use dirty bitmap
> >  #
> >  # Since: 6.2
> >  ##
> > @@ -1768,26 +1769,25 @@
> >  ##
> >  # @DirtyRateInfo:
> >  #
> > -# Information about current dirty page rate of vm.
> > +# Information about measured dirty page rate.
> >  #
> >  # @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.
> > +#     of MiB/s. Value is present only when @status is 'measured'.
> 
> For consistency, please put two spaces between setences.
> 
> >  #
> > -# @status: status containing dirtyrate query status includes
> > -#     'unstarted' or 'measuring' or 'measured'
> > +# @status: current status of dirty page rate measurements
> >  #
> >  # @start-time: start time in units of second for calculation
> >  #
> > -# @calc-time: time in units of second for sample dirty pages
> > +# @calc-time: time period in units of second for which dirty page
> > +#     rate was measured
> 
> Maybe
> 
>    # @calc-time: time period for which dirty page rate was measured
>    #     (in seconds)
> 
> >  #
> > -# @sample-pages: page count per GB for sample dirty pages the default
> > -#     value is 512 (since 6.1)
> > +# @sample-pages: number of sampled pages per each GiB of guest
> 
> per GiB
> 
> > +#     memory.  Value is valid only in page-sampling mode (Since 6.1)
> 
> Suggest "Valid only in ..."
> 
> >  #
> > -# @mode: mode containing method of calculate dirtyrate includes
> > -#     'page-sampling' and 'dirty-ring' (Since 6.2)
> > +# @mode: mode that was used to measure dirty page rate (Since 6.2)
> >  #
> > -# @vcpu-dirty-rate: dirtyrate for each vcpu if dirty-ring mode
> > -#     specified (Since 6.2)
> > +# @vcpu-dirty-rate: dirty rate for each vCPU if dirty-ring mode
> > +#     was specified (Since 6.2)
> >  #
> >  # Since: 5.2
> >  ##
> > @@ -1803,15 +1803,50 @@
> >  ##
> >  # @calc-dirty-rate:
> >  #
> > -# start calculating dirty page rate for vm
> > -#
> > -# @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: mechanism of calculating dirtyrate includes 'page-sampling'
> > -#     and 'dirty-ring' (Since 6.1)
> > +# Starts measuring dirty page rate of the VM.  Results can be
> 
> Imperative mood: "Start measuring ..."
> 
> > +# retrieved with @query-dirty-rate after measurements are completed.
> > +#
> > +# Dirty page rate is the number of pages changed in a given time
> > +# period expressed in MiB/s.  The following methods of calculation
> > +# are available:
> > +#
> > +# 1. In page sampling mode, a random subset of pages are selected
> > +#    and hashed twice: once in the beginning of measurement time
> 
> Suggest "once at the beginning"
> 
> > +#    period, another one -- in the end.  If two hashes for some page
> 
> Suggest ", and once again at the end".
> 
> > +#    are different, the page is counted as changed.  Since this
> > +#    method relies on sampling and hashing, calculated dirty page
> > +#    rate is only the estimation of its true value.  Setting
> > +#    @sample-pages to higher value improves estimation quality but
> 
> Suggest "Increasing @sample-pages improves estimation quality at the
> cost ..."
> 
> > +#    at the cost of higher computational overhead.
> > +#
> > +# 2. Dirty bitmap mode captures writes to memory by temporarily
> > +#    revoking write access to all pages and counting page faults.

Just to mention that wr-protection is only one way to do this, IIUC that
depends on both arch (e.g. s390 impl KVM_GET_DIRTY_LOG totally differently
from x86) and also hardware capabilities (e.g. even on x86, PML enabled
hosts use dirty bits and PML-full vm exits rather than page faults).

But I think wr-protect may still be a good detailed showcase of it so
keeping it there looks very nice, perhaps just add "... writes to memory,
for example, by temporarily revoking ..."?

> 
> Comma before "and".
> 
> > +#    Information about modified pages is collected into bitmap,
> 
> "into a bitmap"
> 
> > +#    where each bit corresponds to one guest page.  This mode
> > +#    requires that KVM accelerator property "dirty-ring-size=N"
> 
> Suggest just "dirty-ring-size" (omit "=N").
> 
> > +#    is *not* set.
> > +#
> > +# 3. Dirty ring mode is similar to dirty bitmap mode, but the
> > +#    information about modified pages is collected into ring buffer.
> > +#    This mode tracks page modification per each vCPU separately.
> 
> Either "for each vCPU" or "per vCPU".
> 
> > +#    It requires that KVM accelerator property "dirty-ring-size=N"
> > +#    is set.
> 
> Suggest just "dirty-ring-size" (omit "=N").
> 
> > +#
> > +# @calc-time: time period in units of second for which dirty page rate
> > +#    is calculated.  Note that larger @calc-time values will typically
> > +#    result in smaller dirty page rates because page dirtying is a
> > +#    one-time event.  Once some page is counted as dirty during
> > +#    @calc-time period, further writes to this page will not increase
> > +#    dirty page rate anymore.
> 
> Please indent one more, for consistency.
> 
> > +#
> > +# @sample-pages: number of sampled pages per each GiB of guest memory.
> > +#     Default value is 512.  For 4KiB guest pages this corresponds to
> > +#     sampling ratio of 0.2%.  This argument is used only in page
> > +#     sampling mode. (Since 6.1)
> 
> Two spaces between '.' and '(', please.
> 
> > +#
> > +# @mode: mechanism for tracking dirty pages.  Default value is
> > +#    'page-sampling'.  Others are 'dirty-bitmap' and 'dirty-ring'.
> > +#    (Since 6.1)
> >  #
> >  # Since: 5.2
> >  #
> > @@ -1828,9 +1863,21 @@
> >  ##
> >  # @query-dirty-rate:
> >  #
> > -# query dirty page rate in units of MB/s for vm
> > +# Query results of the most recent invocation of @calc-dirty-rate.
> >  #
> >  # Since: 5.2
> > +#
> > +# Examples:
> > +#
> > +# 1. Measurement is in progress:
> > +#
> > +# <- {"status": "measuring", "sample-pages": 512,
> > +#     "mode": "page-sampling", "start-time": 3665220, "calc-time": 10}
> > +#
> > +# 2. Measurement has been completed:
> > +#
> > +# <- {"status": "measured", "sample-pages": 512, "dirty-rate": 108,
> > +#     "mode": "page-sampling", "start-time": 3665220, "calc-time": 10}
> >  ##
> >  { 'command': 'query-dirty-rate', 'returns': 'DirtyRateInfo' }
> 
> This is *sooo* much better than before.  Thank you!

I also agree. :)

> 
> An R-by from a migration maintainer would be nice.

Only one migration maintainer now, but we have two reviewers.

Here's one from the reviewers' list:

Acked-by: Peter Xu <peterx@redhat.com>

> 
> If you agree with my suggestions, I can apply them in my tree, saving
> you a respin.  Let me know.
> 
> Acked-by: Markus Armbruster <armbru@redhat.com>
> 

-- 
Peter Xu