Re: [PATCH v12 2/5] migration/dirtyrate: refactor dirty page rate calculation

[Hyman Huang]

在 2022/2/8 16:18, Peter Xu 写道:
> On Mon, Jan 24, 2022 at 10:10:37PM +0800, huangy81@chinatelecom.cn wrote:
> > diff --git a/cpus-common.c b/cpus-common.c
> > index 6e73d3e..63159d6 100644
> > --- a/cpus-common.c
> > +++ b/cpus-common.c
> > @@ -73,6 +73,7 @@ static int cpu_get_free_index(void)
> >   }
> >   
> >   CPUTailQ cpus = QTAILQ_HEAD_INITIALIZER(cpus);
> > +unsigned int cpu_list_generation_id;
> >   
> >   void cpu_list_add(CPUState *cpu)
> >   {
> > @@ -84,6 +85,7 @@ void cpu_list_add(CPUState *cpu)
> >           assert(!cpu_index_auto_assigned);
> >       }
> >       QTAILQ_INSERT_TAIL_RCU(&cpus, cpu, node);
> > +    cpu_list_generation_id++;
> >   }
> >   
> >   void cpu_list_remove(CPUState *cpu)
> > @@ -96,6 +98,7 @@ void cpu_list_remove(CPUState *cpu)
> >   
> >       QTAILQ_REMOVE_RCU(&cpus, cpu, node);
> >       cpu->cpu_index = UNASSIGNED_CPU_INDEX;
> > +    cpu_list_generation_id++;
> >   }
>
> Could you move the cpu list gen id changes into a separate patch?
Yes, of course
> >   
> >   CPUState *qemu_get_cpu(int index)
> > diff --git a/include/sysemu/dirtyrate.h b/include/sysemu/dirtyrate.h
> > new file mode 100644
> > index 0000000..ea4785f
> > --- /dev/null
> > +++ b/include/sysemu/dirtyrate.h
> > @@ -0,0 +1,31 @@
> > +/*
> > + * dirty page rate helper functions
> > + *
> > + * Copyright (c) 2022 CHINA TELECOM CO.,LTD.
> > + *
> > + * Authors:
> > + *  Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#ifndef QEMU_DIRTYRATE_H
> > +#define QEMU_DIRTYRATE_H
> > +
> > +extern unsigned int cpu_list_generation_id;
>
> How about exporting a function cpu_list_generation_id_get() from the cpu code,
> rather than referencing it directly?
Ok, this will be done along with "cpu list gen id changes" in a separate 
patch
> > +int64_t vcpu_calculate_dirtyrate(int64_t calc_time_ms,
> > +                                 int64_t init_time_ms,
> > +                                 VcpuStat *stat,
> > +                                 unsigned int flag,
> > +                                 bool one_shot)
> > +{
> > +    DirtyPageRecord *records;
> > +    int64_t duration;
> > +    int64_t dirtyrate;
> > +    int i = 0;
> > +    unsigned int gen_id;
> > +
> > +retry:
> > +    cpu_list_lock();
> > +    gen_id = cpu_list_generation_id;
> > +    records = vcpu_dirty_stat_alloc(stat);
> > +    vcpu_dirty_stat_collect(stat, records, true);
> > +
> > +    duration = dirty_stat_wait(calc_time_ms, init_time_ms);
> > +    cpu_list_unlock();
>
> Should release the lock before sleep (dirty_stat_wait)?
Good point, since we have introduced the cpu_list_generation_id and make 
sure the we can handle the plug/unplug scenario, we can make the cpu 
plug/unplug as fast as it can. :)
> > +
> > +    global_dirty_log_sync(flag, one_shot);
> > +
> > +    cpu_list_lock();
> > +    if (gen_id != cpu_list_generation_id) {
> > +        g_free(records);
> > +        g_free(stat->rates);
> > +        cpu_list_unlock();
> > +        goto retry;
> > +    }
> > +    vcpu_dirty_stat_collect(stat, records, false);
> > +    cpu_list_unlock();
> > +
> > +    for (i = 0; i < stat->nvcpu; i++) {
> > +        dirtyrate = do_calculate_dirtyrate(records[i], duration);
> > +
> > +        stat->rates[i].id = i;
> > +        stat->rates[i].dirty_rate = dirtyrate;
> > +
> > +        trace_dirtyrate_do_calculate_vcpu(i, dirtyrate);
> > +    }
> > +
> > +    g_free(records);
> > +
> > +    return duration;
> > +}
>
> Thanks,

--
Best regard

Hyman Huang(黄勇)