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

[Juan Quintela]

huangy81@chinatelecom.cn wrote:
> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
>
> introduce the third method GLOBAL_DIRTY_RESTRAINT of dirty
> tracking for calculate dirtyrate periodly for dirty restraint.
>
> implement thread for calculate dirtyrate periodly, which will
> be used for dirty restraint.
>
> add dirtyrestraint.h to introduce the util function for dirty
> restrain.
>
> Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>

Some comentes:

> +void dirtyrestraint_calc_start(void);
> +
> +void dirtyrestraint_calc_state_init(int max_cpus);

dirtylimit_? instead of restraint.

We have a start function, but I can't see a finish/end/stop functions.

> +#define DIRTYRESTRAINT_CALC_TIME_MS         1000    /* 1000ms */
> +
> +struct {
> +    DirtyRatesData data;
> +    int64_t period;
> +    bool enable;

Related to previous comment.  I can't see where we set enable to 1, but
nowhere were we set it back to 0, so this never finish.

> +    QemuCond ready_cond;
> +    QemuMutex ready_mtx;

This is a question of style, but when you only have a mutex and a cond
in one struct, you can use the "cond" and "mutex" names.

But as said, it is a question of style, if you preffer do it this way.

> +static inline void record_dirtypages(DirtyPageRecord *dirty_pages,
> +                                     CPUState *cpu, bool start);

You have put the code at the beggining of the file, if you put it at the
end of it, I think you can avoid this forward declaration.

> +static void dirtyrestraint_calc_func(void)
> +{
> +    CPUState *cpu;
> +    DirtyPageRecord *dirty_pages;
> +    int64_t start_time, end_time, calc_time;
> +    DirtyRateVcpu rate;
> +    int i = 0;
> +
> +    dirty_pages = g_malloc0(sizeof(*dirty_pages) *
> +        dirtyrestraint_calc_state->data.nvcpu);
> +
> +    dirtyrestraint_global_dirty_log_start();
> +
> +    CPU_FOREACH(cpu) {
> +        record_dirtypages(dirty_pages, cpu, true);
> +    }
> +
> +    start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> +    g_usleep(DIRTYRESTRAINT_CALC_TIME_MS * 1000);
> +    end_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> +    calc_time = end_time - start_time;
> +
> +    dirtyrestraint_global_dirty_log_stop();
> +
> +    CPU_FOREACH(cpu) {
> +        record_dirtypages(dirty_pages, cpu, false);
> +    }
> +
> +    for (i = 0; i < dirtyrestraint_calc_state->data.nvcpu; i++) {
> +        uint64_t increased_dirty_pages =
> +            dirty_pages[i].end_pages - dirty_pages[i].start_pages;
> +        uint64_t memory_size_MB =
> +            (increased_dirty_pages * TARGET_PAGE_SIZE) >> 20;
> +        int64_t dirtyrate = (memory_size_MB * 1000) / calc_time;
> +
> +        rate.id = i;
> +        rate.dirty_rate  = dirtyrate;
> +        dirtyrestraint_calc_state->data.rates[i] = rate;
> +
> +        trace_dirtyrate_do_calculate_vcpu(i,
> +            dirtyrestraint_calc_state->data.rates[i].dirty_rate);
> +    }
> +
> +    return;

unnecesary return;

> +}
> +
> +static void *dirtyrestraint_calc_thread(void *opaque)
> +{
> +    rcu_register_thread();
> +
> +    while (qatomic_read(&dirtyrestraint_calc_state->enable)) {
> +        dirtyrestraint_calc_func();
> +        dirtyrestraint_calc_state->ready = true;

           You really need this to be a global variable?  You can pass
           it on the opaque, no?

Later, Juan.