Re: [Qemu-devel] [PATCH] migration: Improve accuracy of vCPU throttling with per-vCPU timers

[Peter Xu]

On Tue, Jun 18, 2019 at 12:25:43PM +0000, Cosmin Marin wrote:
>       Hi Peter,
> 
>       thanks for reviewing the patch. Indeed, I agree that it's almost 
> impossible to determine which solution it's better from the scalability 
> perspective. However, I feel that using per-vCPU timers is the only way for 
> ensuring correctness of the throttling ratio.

The thing is that your patch actually contains two changes:

1. use N timers instead of one.

2. remove throttle_thread_scheduled check, so we do the throttle
   always

Here what I'm worried is that _maybe_ the 2nd item is the one that
really helped.

Note that there is a side effect that we might queue more than one
work on one specific cpu if we queue it too fast, but it does not
block us from trying it out to identify which item (1 or 2 or both)
really helped here.  Then if we think that (queuing too much) is an
issue then we can discuss on how to fix it since current patch will
have this problem as well.

> 
>       It's a bit unclear to me how the throttling ratio inconsistency can be 
> fixed by using a single timer even avoiding the conditional timer re-arming.  
> Could you provide more details about the use of a single timer ?
  
It'll be simply a patch only contains the changes for item (2) above.
To be explicit, it is:

-------------------------------------------------------------------
diff --git a/cpus.c b/cpus.c
index dde3b7b981..bb59675b98 100644
--- a/cpus.c
+++ b/cpus.c
@@ -792,7 +792,6 @@ static void cpu_throttle_thread(CPUState *cpu, 
run_on_cpu_data opaque)
     qemu_mutex_unlock_iothread();
     g_usleep(sleeptime_ns / 1000); /* Convert ns to us for usleep call */
     qemu_mutex_lock_iothread();
-    atomic_set(&cpu->throttle_thread_scheduled, 0);
 }

 static void cpu_throttle_timer_tick(void *opaque)
@@ -805,10 +804,7 @@ static void cpu_throttle_timer_tick(void *opaque)
         return;
     }
     CPU_FOREACH(cpu) {
-        if (!atomic_xchg(&cpu->throttle_thread_scheduled, 1)) {
-            async_run_on_cpu(cpu, cpu_throttle_thread,
-                             RUN_ON_CPU_NULL);
-        }
+        async_run_on_cpu(cpu, cpu_throttle_thread, RUN_ON_CPU_NULL);
     }

     pct = (double)cpu_throttle_get_percentage()/100;
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 5ee0046b62..0bd34fbb70 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -439,11 +439,6 @@ struct CPUState {
     /* shared by kvm, hax and hvf */
     bool vcpu_dirty;

-    /* Used to keep track of an outstanding cpu throttle thread for migration
-     * autoconverge
-     */
-    bool throttle_thread_scheduled;
-
     bool ignore_memory_transaction_failures;

     struct hax_vcpu_state *hax_vcpu;
-------------------------------------------------------------------

Regards,

-- 
Peter Xu