Re: [Qemu-devel] [PATCH v7 1/5] cpu: Provide vcpu throttling interface

[Juan Quintela]

"Jason J. Herne" <address@hidden> wrote:
> Provide a method to throttle guest cpu execution. CPUState is augmented with
> timeout controls and throttle start/stop functions. To throttle the guest cpu
> the caller simply has to call the throttle set function and provide a 
> percentage
> of throttle time.
>
> Signed-off-by: Jason J. Herne <address@hidden>
> Reviewed-by: Matthew Rosato <address@hidden>
> ---
>  cpus.c            | 78 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/qom/cpu.h | 42 ++++++++++++++++++++++++++++++
>  2 files changed, 120 insertions(+)
>
> diff --git a/cpus.c b/cpus.c
> index de6469f..b5ff9c9 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -68,6 +68,14 @@ static CPUState *next_cpu;
>  int64_t max_delay;
>  int64_t max_advance;
>  
> +/* vcpu throttling controls */
> +static QEMUTimer *throttle_timer;
> +static unsigned int throttle_percentage;
> +
> +#define CPU_THROTTLE_PCT_MIN 1
> +#define CPU_THROTTLE_PCT_MAX 99
> +#define CPU_THROTTLE_TIMESLICE_NS 10000000
> +
>  bool cpu_is_stopped(CPUState *cpu)
>  {
>      return cpu->stopped || !runstate_is_running();
> @@ -486,10 +494,80 @@ static const VMStateDescription vmstate_timers = {
>      }
>  };
>  
> +static void cpu_throttle_thread(void *opaque)
> +{
> +    CPUState *cpu = opaque;
> +    double pct;
> +    double throttle_ratio;
> +    long sleeptime_ns;
> +
> +    if (!cpu_throttle_get_percentage()) {

cpu_throotle_active()?


> +        return;
> +    }
> +
> +    pct = (double)cpu_throttle_get_percentage()/100;
> +    throttle_ratio = pct / (1 - pct);
> +    sleeptime_ns = (long)(throttle_ratio * CPU_THROTTLE_TIMESLICE_NS);
> +
> +    qemu_mutex_unlock_iothread();
> +    atomic_set(&cpu->throttle_thread_scheduled, 0);
> +    g_usleep(sleeptime_ns / 1000); /* Convert ns to us for usleep call */
> +    qemu_mutex_lock_iothread();


Why is this thread safe?

qemu_mutex_lock_iothread() is protecting (at least) cpu_work_first on
each cpu.  How can we be sure that _nothing_ will change that while we
are waiting?   A fast look through the tree don't show anything that
runs here that drops the lock.  I am missing something?

> +}
> +
> +static void cpu_throttle_timer_tick(void *opaque)
> +{
> +    CPUState *cpu;
> +    double pct;
> +
> +    /* Stop the timer if needed */
> +    if (!cpu_throttle_get_percentage()) {

cpu_throotle_active()?

agree with rest of the changes.

Later, Juan.