[v6,2/2] migration/dirtyrate: implement dirty-bitmap dirtyrate calculation

From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>

introduce dirty-bitmap mode as the third method of calc-dirty-rate.
implement dirty-bitmap dirtyrate calculation, which can be used
to measuring dirtyrate in the absence of dirty-ring.

introduce "dirty_bitmap:-b" option in hmp calc_dirty_rate to
indicate dirty bitmap method should be used for calculation.

Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
---
 hmp-commands.hx       |   9 ++--
 migration/dirtyrate.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++----
 qapi/migration.json   |   6 ++-
 3 files changed, 115 insertions(+), 15 deletions(-)

On Fri, Jul 16, 2021 at 07:13:47PM +0800, huangy81@chinatelecom.cn wrote:
> +static void calculate_dirtyrate_dirty_bitmap(struct DirtyRateConfig config)
> +{
> +    int64_t msec = 0;
> +    int64_t start_time;
> +    DirtyPageRecord dirty_pages;

[1]

> +
> +    dirtyrate_global_dirty_log_start();
> +
> +    /*
> +     * 1'round of log sync may return all 1 bits with
> +     * KVM_DIRTY_LOG_INITIALLY_SET enable
> +     * skip it unconditionally and start dirty tracking
> +     * from 2'round of log sync
> +     */
> +    dirtyrate_global_dirty_log_sync();
> +
> +    /*
> +     * reset page protect manually and unconditionally.
> +     * this make sure kvm dirty log be cleared if
> +     * KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE cap is enabled.
> +     */
> +    dirtyrate_manual_reset_protect();
> +

[2]

> +    record_dirtypages_bitmap(&dirty_pages, true);

[3]

> +
> +    start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> +    DirtyStat.start_time = start_time / 1000;
> +
> +    msec = config.sample_period_seconds * 1000;
> +    msec = set_sample_page_period(msec, start_time);
> +    DirtyStat.calc_time = msec / 1000;
> +
> +    /* fetch dirty bitmap from kvm and stop dirty tracking */

I don't think it really fetched anything..  So I think we need:

       dirtyrate_global_dirty_log_sync();

It seems to be there in older versions but not in the latest two versions.

Please still remember to smoke the patches before posting, because without the
log sync we'll read nothing.

> +    dirtyrate_global_dirty_log_stop();
> +
> +    record_dirtypages_bitmap(&dirty_pages, false);

[4]

I think it's easier we take bql at [1]/[3] and release at [2]/[4], rather than
taking it for every function.  Then we can move the bql operations out of
dirtyrate_global_dirty_log_stop() in this patch.

Thanks,

> +
> +    do_calculate_dirtyrate_bitmap(dirty_pages);
> +}

在 2021/7/17 3:36, Peter Xu 写道:
> On Fri, Jul 16, 2021 at 07:13:47PM +0800, huangy81@chinatelecom.cn wrote:
>> +static void calculate_dirtyrate_dirty_bitmap(struct DirtyRateConfig config)
>> +{
>> +    int64_t msec = 0;
>> +    int64_t start_time;
>> +    DirtyPageRecord dirty_pages;
> 
> [1]
> 
>> +
>> +    dirtyrate_global_dirty_log_start();
>> +
>> +    /*
>> +     * 1'round of log sync may return all 1 bits with
>> +     * KVM_DIRTY_LOG_INITIALLY_SET enable
>> +     * skip it unconditionally and start dirty tracking
>> +     * from 2'round of log sync
>> +     */
>> +    dirtyrate_global_dirty_log_sync();
>> +
>> +    /*
>> +     * reset page protect manually and unconditionally.
>> +     * this make sure kvm dirty log be cleared if
>> +     * KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE cap is enabled.
>> +     */
>> +    dirtyrate_manual_reset_protect();
>> +
> 
> [2]
> 
>> +    record_dirtypages_bitmap(&dirty_pages, true);
> 
> [3]
> 
>> +
>> +    start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
>> +    DirtyStat.start_time = start_time / 1000;
>> +
>> +    msec = config.sample_period_seconds * 1000;
>> +    msec = set_sample_page_period(msec, start_time);
>> +    DirtyStat.calc_time = msec / 1000;
>> +
>> +    /* fetch dirty bitmap from kvm and stop dirty tracking */
> 
> I don't think it really fetched anything..  So I think we need:
> 
>         dirtyrate_global_dirty_log_sync();
> 
> It seems to be there in older versions but not in the latest two versions.
yes, latest version dropped this because dirtyrate_global_dirty_log_stop 
below already do the sync before stop dirty log, which is recommended in 
patchset of "support dirtyrate at the granualrity of vcpu" and make 
dirtyrate more accurate. the older version do not consider this. :)
> 
> Please still remember to smoke the patches before posting, because without the
> log sync we'll read nothing.
> 
>> +    dirtyrate_global_dirty_log_stop();
>> +
>> +    record_dirtypages_bitmap(&dirty_pages, false);
> 
> [4]
> 
> I think it's easier we take bql at [1]/[3] and release at [2]/[4], rather than
> taking it for every function.  Then we can move the bql operations out of
> dirtyrate_global_dirty_log_stop() in this patch.
yeah, take bql at [1] and release at [2] is reasonable.
but if we try to take bql at [3], it will sleep for calculation time in 
set_sample_page_period which is configured by user, which may be a heavy 
overhead.
how about we take bql at [1] and release at [2], ingore bql at [3]/[4] 
and let it be the same as older versoin. since we only copy 
total_dirty_pages to local var in "get_dirtyrate" thread and maybe we 
don't need bql.
> 
> Thanks,
> 
>> +
>> +    do_calculate_dirtyrate_bitmap(dirty_pages);
>> +}
>

On Sat, Jul 17, 2021 at 10:57:50AM +0800, Hyman wrote:
> 
> 
> 在 2021/7/17 3:36, Peter Xu 写道:
> > On Fri, Jul 16, 2021 at 07:13:47PM +0800, huangy81@chinatelecom.cn wrote:
> > > +static void calculate_dirtyrate_dirty_bitmap(struct DirtyRateConfig config)
> > > +{
> > > +    int64_t msec = 0;
> > > +    int64_t start_time;
> > > +    DirtyPageRecord dirty_pages;
> > 
> > [1]
> > 
> > > +
> > > +    dirtyrate_global_dirty_log_start();
> > > +
> > > +    /*
> > > +     * 1'round of log sync may return all 1 bits with
> > > +     * KVM_DIRTY_LOG_INITIALLY_SET enable
> > > +     * skip it unconditionally and start dirty tracking
> > > +     * from 2'round of log sync
> > > +     */
> > > +    dirtyrate_global_dirty_log_sync();
> > > +
> > > +    /*
> > > +     * reset page protect manually and unconditionally.
> > > +     * this make sure kvm dirty log be cleared if
> > > +     * KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE cap is enabled.
> > > +     */
> > > +    dirtyrate_manual_reset_protect();
> > > +
> > 
> > [2]
> > 
> > > +    record_dirtypages_bitmap(&dirty_pages, true);
> > 
> > [3]
> > 
> > > +
> > > +    start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> > > +    DirtyStat.start_time = start_time / 1000;
> > > +
> > > +    msec = config.sample_period_seconds * 1000;
> > > +    msec = set_sample_page_period(msec, start_time);
> > > +    DirtyStat.calc_time = msec / 1000;
> > > +
> > > +    /* fetch dirty bitmap from kvm and stop dirty tracking */
> > 
> > I don't think it really fetched anything..  So I think we need:
> > 
> >         dirtyrate_global_dirty_log_sync();
> > 
> > It seems to be there in older versions but not in the latest two versions.
> yes, latest version dropped this because dirtyrate_global_dirty_log_stop
> below already do the sync before stop dirty log, which is recommended in
> patchset of "support dirtyrate at the granualrity of vcpu" and make
> dirtyrate more accurate. the older version do not consider this. :)

Oh I see.  I was still using your old code so it does not have that bit.  It's okay.

> > 
> > Please still remember to smoke the patches before posting, because without the
> > log sync we'll read nothing.
> > 
> > > +    dirtyrate_global_dirty_log_stop();
> > > +
> > > +    record_dirtypages_bitmap(&dirty_pages, false);
> > 
> > [4]
> > 
> > I think it's easier we take bql at [1]/[3] and release at [2]/[4], rather than
> > taking it for every function.  Then we can move the bql operations out of
> > dirtyrate_global_dirty_log_stop() in this patch.
> yeah, take bql at [1] and release at [2] is reasonable.
> but if we try to take bql at [3], it will sleep for calculation time in
> set_sample_page_period which is configured by user, which may be a heavy
> overhead.
> how about we take bql at [1] and release at [2], ingore bql at [3]/[4] and
> let it be the same as older versoin. since we only copy total_dirty_pages to
> local var in "get_dirtyrate" thread and maybe we don't need bql.

Sounds good, thanks.