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
