Re: [PATCH v10 1/3] migration/dirtyrate: implement vCPU dirtyrate calculation periodically

[Peter Xu]

On Thu, Dec 30, 2021 at 01:01:09PM +0800, Hyman Huang wrote:
> > > +/**
> > > + * dirtylimit_calc_state_finalize
> > > + *
> > > + * finalize dirty page rate calculation state.
> > > + */
> > > +void dirtylimit_calc_state_finalize(void);
> > > +#endif
> > 
> > Since dirtylimit and dirtyrate looks so alike, not sure it's easier to just
> > reuse dirtyrate.h; after all you reused dirtyrate.c.
> I'm working on this, and i find it's fine to just reuse dirtyrate.h, but the
> origin dirtyrate.h didn't export any function to other qemu module, so we
> should add a new file include/sysemu/dirtyrate.h to export the dirty page
> rage caluculation util function, how do you think about this?

I see what you meant, yeah if it's exported outside migration/ then looks fine.

> 
> I'm doing the code review and i find that it is more reasonable to implement
> the dirtylimit just in a standalone file such as softmmu/dirtylimit.c, since
> the implementation of dirtylimit in v10 has nothing to do with throttle algo
> in softmmu/cpu-throttle.c. If we merge the two implemenation into one file,
> it is weired. Is this ok?

Sure.

> > At least it can be changed into something like:
> > 
> >    dirtylimit_calc_func(DirtyRateVcpu *stat)
> >    {
> >        // never race against cpu hotplug/unplug
> >        cpu_list_lock();
> > 
> >        // this should include allocate buffers and record initial values 
> > for all cores
> >        record = vcpu_dirty_stat_alloc();
> >        // do the sleep
> >        duration = vcpu_dirty_stat_wait(records)
> > 
> >        // the "difference"..
> >        global_dirty_log_sync();
> > 
> >        // collect end timestamps, calculates
> >        vcpu_dirty_stat_collect(stat, records);
> >        vcpu_dirty_stat_free(stat);
> > 
> >        cpu_list_unlock();
> > 
> >        return stat;
> >    }
> > 
> > It may miss something but just to show what I meant..
> I think this work is refactor and i do this in a standalone commit before
> the dirtylimit commits. Is this ok?

I think that's more than welcomed if you can split the patch into smaller ones;
they'll be even easier to be reviewed.

Thanks,

-- 
Peter Xu