Re: [PATCH v14 3/7] migration/dirtyrate: Refactor dirty page rate calculation

[Hyman Huang]

在 2022/2/14 15:57, Peter Xu 写道:
> Mostly good, one trivial nit below:
>
> On Fri, Feb 11, 2022 at 12:17:37AM +0800, huangy81@chinatelecom.cn wrote:
> > +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_get();
> > +    records = vcpu_dirty_stat_alloc(stat);
> > +    vcpu_dirty_stat_collect(stat, records, true);
> > +    cpu_list_unlock();
> > +
> > +    duration = dirty_stat_wait(calc_time_ms, init_time_ms);
>
> Is it a must to pass in init_time_ms rather than always sleep in
> dirty_stat_wait()?  Could we simply drop it?
Indeed, the parameter 'init_time_ms' seems kind of weird :(, we 
introduce 'init_time_ms' just becasue the calculate_dirtyrate_dirty_ring 
will call the function, see the following block:


 static void calculate_dirtyrate_dirty_ring(struct DirtyRateConfig config)
 {
-    CPUState *cpu;
-    int64_t msec = 0;
     int64_t start_time;
+    int64_t duration;
     uint64_t dirtyrate = 0;
     uint64_t dirtyrate_sum = 0;
-    DirtyPageRecord *dirty_pages;
-    int nvcpu = 0;
     int i = 0;

-    CPU_FOREACH(cpu) {
-        nvcpu++;
-    }
-
-    dirty_pages = malloc(sizeof(*dirty_pages) * nvcpu);
-
-    DirtyStat.dirty_ring.nvcpu = nvcpu;
-    DirtyStat.dirty_ring.rates = malloc(sizeof(DirtyRateVcpu) * nvcpu);
-
-    dirtyrate_global_dirty_log_start();
-
-    CPU_FOREACH(cpu) {
-        record_dirtypages(dirty_pages, cpu, true);
-    }
+    /* start log sync */
+    global_dirty_log_change(GLOBAL_DIRTY_DIRTY_RATE, true);

     start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
     DirtyStat.start_time = start_time / 1000;

The reason why we introduce the 'init_time_ms' is wanting to store the
start_time info and display.

Dropping this parameter is fine from my point view if we ignore the 
duration error result from the delay between caller and callee of
'vcpu_calculate_dirtyrate’

-    msec = config.sample_period_seconds * 1000;
-    msec = set_sample_page_period(msec, start_time);
-    DirtyStat.calc_time = msec / 1000;
+    /* calculate vcpu dirtyrate */
+    duration = vcpu_calculate_dirtyrate(config.sample_period_seconds * 
1000,
+                                        start_time,
+                                        &DirtyStat.dirty_ring,
+                                        GLOBAL_DIRTY_DIRTY_RATE,
+                                        true);

-    dirtyrate_global_dirty_log_stop();
-
-    CPU_FOREACH(cpu) {
-        record_dirtypages(dirty_pages, cpu, false);
-    }
+    DirtyStat.calc_time = duration / 1000;




--
Best regard

Hyman Huang(黄勇)