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, huang...@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
