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

[Peter Xu]

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.

-- 
Peter Xu