Re: [PATCH v3 7/7] migration/dirtyrate: implement dirty-ring dirtyrate calculation

[Peter Xu <peterx@redhat.com>]

On Fri, Jun 11, 2021 at 10:05:22PM +0800, Hyman Huang wrote:
> > > +    switch (last_method) {
> > > +    case CALC_NONE:
> > > +    case CALC_SAMPLE_PAGES:
> > > +        if (config.per_vcpu) {
> > > +            DirtyStat.method.vcpu.nvcpu = -1;
> > > +            DirtyStat.method.vcpu.rates = NULL;
> > > +        } else {
> > > +            DirtyStat.method.vm.total_dirty_samples = 0;
> > > +            DirtyStat.method.vm.total_sample_count = 0;
> > > +            DirtyStat.method.vm.total_block_mem_MB = 0;
> > > +        }
> > > +        break;
> > > +    case CALC_DIRTY_RING:
> > > +        if (!config.per_vcpu) {
> > > +            g_free(DirtyStat.method.vcpu.rates);
> > > +            DirtyStat.method.vcpu.rates = NULL;
> > > +            DirtyStat.method.vm.total_dirty_samples = 0;
> > > +            DirtyStat.method.vm.total_sample_count = 0;
> > > +            DirtyStat.method.vm.total_block_mem_MB = 0;
> > > +        }
> > 
> > I'm a bit confused; why it's CALC_DIRTY_RING but per_vcpu not set?  Why we need
> > to care about "last_method" at all?..
> this two method share the union, the sample method use the local variable of
> SampleVMStat type, the dirty ring method should alloc rates of DirtyRateVcpu
> type every time qmp calc_dirty_rate called in case of add vcpu, once rates
> has been store dirty rate value, it can't be freed until the next time of
> executing calc_dirty_rate, because info dirty rate may access the rates
> struct, so the rates struct can only be freed before calc_dirty_rate with
> sample.

OK, then please consider separate the cleanup and init, otherwise as mode
grows, it'll be a N*N matrix, afaict..

So we can always clean everything first depending on the old mode, then update
last_method (or whatever its new name is...) to the new mode, and init the stat
for the new mode.  Then it'll always be 2*N.

Another note is that I also suggest you do all these in the main thread, not
the sampling thread.  Because if you do that in sampling thread it means main
thread can "query" at any time, then we'd better need a mutex to protect the
whole section when you update either last_method or DirtyStat.  In main thread
it should be serialized already as there's either BQL or a single QMP monitor
thread.

Oh wait... not sure whether "only one QMP iothread" will still keep true
forever.. So maybe always introduce a mutex and protect all those fields; who
knows when QMP will start concurrent execution of these "info" commands (it
looks doable.. :).

-- 
Peter Xu