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

[Markus Armbruster <armbru@redhat.com>]

Peter Xu <peterx@redhat.com> writes:

> 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

[...]

>> > +# 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).

Good point.

> 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 ..."?

Going with

      # 2. Dirty bitmap mode captures writes to memory (for example by
      #    temporarily revoking write access to all pages) and counting page
      #    faults.  Information about modified pages is collected into a
      #    bitmap, where each bit corresponds to one guest page.  This mode
      #    requires that KVM accelerator property "dirty-ring-size" is *not*
      #    set.

if you don't mind.

[...]

>> 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>

Thanks!

[...]