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

[Hyman]

在 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);
> > +}