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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
> On 06/26/2015 02:07 PM, Dr. David Alan Gilbert wrote:
> >* Jason J. Herne (jjherne@linux.vnet.ibm.com) 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.
> >
> >I'm worried about atomicity and threads and all those fun things.
> >
> >I think all the starting/stopping/setting the throttling level is done in the
> >migration thread; I think the timers run in the main/io thread?
> >So you really need to be careful with at least:
> >     throttle_timer_stop - which may have a minor effect
> >     throttle_timer  - I worry about the way cpu_timer_active checks the pointer
> >                       yet it's freed when the timer goes off.   It's probably
> >                       not too bad because it never dereferences it.
> >
> >So, probably need some atomic's in there (cc'ing paolo)
> >
> >Dave
> >
> 
> I think we're ok with respect to throttle_timer. As you mentioned, we rely
> on the
> value only to know if throttling is active or not.
> 
> I'm not seeing any other race conditions or serialization issues. But that
> doesn't
> mean the code is perfect either :)
> 
> >>Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>
> >>Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> >>---
> >>  cpus.c            | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  include/qom/cpu.h | 38 ++++++++++++++++++++++++++++
> >>  2 files changed, 114 insertions(+)
> >>
> >>diff --git a/cpus.c b/cpus.c
> >>index de6469f..f57cf4f 100644
> >>--- a/cpus.c
> >>+++ b/cpus.c
> >>@@ -68,6 +68,16 @@ static CPUState *next_cpu;
> >>  int64_t max_delay;
> >>  int64_t max_advance;
> >>
> >>+/* vcpu throttling controls */
> >>+static QEMUTimer *throttle_timer;
> >>+static bool throttle_timer_stop;
> >>+static int throttle_percentage;
> >
> >Unsigned?
> >
> 
> Yep. Can do.
> 
> >>+static float throttle_ratio;
> >>+
> >>+#define CPU_THROTTLE_PCT_MIN 1
> >>+#define CPU_THROTTLE_PCT_MAX 99
> >>+#define CPU_THROTTLE_TIMESLICE 10
> >>+
> >>  bool cpu_is_stopped(CPUState *cpu)
> >>  {
> >>      return cpu->stopped || !runstate_is_running();
> >>@@ -919,6 +929,72 @@ static void qemu_kvm_wait_io_event(CPUState *cpu)
> >>      qemu_wait_io_event_common(cpu);
> >>  }
> >>
> >>+static void cpu_throttle_thread(void *opaque)
> >>+{
> >>+    long sleeptime_ms = (long)(throttle_ratio * CPU_THROTTLE_TIMESLICE);
> >>+
> >>+    qemu_mutex_unlock_iothread();
> >>+    g_usleep(sleeptime_ms * 1000); /* Convert ms to us for usleep call */
> >>+    qemu_mutex_lock_iothread();
> >>+
> >>+    timer_mod(throttle_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
> >>+                                   CPU_THROTTLE_TIMESLICE);
> >>+}
> >>+
> >>+static void cpu_throttle_timer_pop(void *opaque)
> >>+{
> >>+    CPUState *cpu;
> >>+
> >>+    /* Stop the timer if needed */
> >>+    if (throttle_timer_stop) {
> >>+        timer_del(throttle_timer);
> >>+        timer_free(throttle_timer);
> >>+        throttle_timer = NULL;
> >>+        return;
> >>+    }
> >>+
> >>+    CPU_FOREACH(cpu) {
> >>+        async_run_on_cpu(cpu, cpu_throttle_thread, NULL);
> >>+    }
> >>+}
> >
> >Why pop? I pop stacks, balloons and bubbles.
> >
> 
> Hmmm... timer pops are a very common term in System Z land :). But then
> again we do have a ton of odd terminology around here. Do you have a
> better suggestion?  cpu_throttle_timer_expire? cpu_throttle_timer_tick?

My preference would be _tick.

Dave

> 
> >>+
> >>+void cpu_throttle_set(int new_throttle_pct)
> >>+{
> >>+    double pct;
> >>+
> >>+    /* Ensure throttle percentage is within valid range */
> >>+    new_throttle_pct = MIN(new_throttle_pct, CPU_THROTTLE_PCT_MAX);
> >>+    throttle_percentage = MAX(new_throttle_pct, CPU_THROTTLE_PCT_MIN);
> >>+
> >>+    pct = (double)throttle_percentage/100;
> >>+    throttle_ratio = pct / (1 - pct);
> >>+
> >>+    if (!cpu_throttle_active()) {
> >>+        throttle_timer_stop = false;
> >>+        throttle_timer = timer_new_ms(QEMU_CLOCK_REALTIME,
> >>+                                           cpu_throttle_timer_pop, NULL);
> >>+        timer_mod(throttle_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
> >>+                                       CPU_THROTTLE_TIMESLICE);
> >>+    }
> >>+}
> >>+
> >>+void cpu_throttle_stop(void)
> >>+{
> >>+    if (cpu_throttle_active()) {
> >>+        throttle_timer_stop = true;
> >>+    }
> >>+}
> >>+
> >>+bool cpu_throttle_active(void)
> >>+{
> >>+    return (throttle_timer != NULL);
> >>+}
> >>+
> >>+int cpu_throttle_get_percentage(void)
> >>+{
> >>+    return throttle_percentage;
> >>+}
> >>+
> >>  static void *qemu_kvm_cpu_thread_fn(void *arg)
> >>  {
> >>      CPUState *cpu = arg;
> >>diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> >>index 39f0f19..56eb964 100644
> >>--- a/include/qom/cpu.h
> >>+++ b/include/qom/cpu.h
> >>@@ -553,6 +553,44 @@ CPUState *qemu_get_cpu(int index);
> >>   */
> >>  bool cpu_exists(int64_t id);
> >>
> >>+/**
> >>+ * cpu_throttle_set:
> >>+ * @new_throttle_pct: Percent of sleep time to running time.
> >>+ *                    Valid range is 1 to 99.
> >>+ *
> >>+ * Throttles all vcpus by forcing them to sleep for the given percentage of
> >>+ * time. A throttle_percentage of 50 corresponds to a 50% duty cycle roughly.
> >>+ * (example: 10ms sleep for every 10ms awake).
> >>+ *
> >>+ * cpu_throttle_set can be called as needed to adjust new_throttle_pct.
> >>+ * Once the throttling starts, it will remain in effect until cpu_throttle_stop
> >>+ * is called.
> >>+ */
> >>+void cpu_throttle_set(int new_throttle_pct);
> >>+
> >>+/**
> >>+ * cpu_throttle_stop:
> >>+ *
> >>+ * Stops the vcpu throttling started by cpu_throttle_set.
> >>+ */
> >>+void cpu_throttle_stop(void);
> >>+
> >>+/**
> >>+ * cpu_throttle_active:
> >>+ *
> >>+ * Returns %true if the vcpus are currently being throttled, %false otherwise.
> >>+ */
> >>+bool cpu_throttle_active(void);
> >>+
> >>+/**
> >>+ * cpu_throttle_get_percentage:
> >>+ *
> >>+ * Returns the vcpu throttle percentage. See cpu_throttle_set for details.
> >>+ *
> >>+ * Returns The throttle percentage in range 1 to 99.
> >>+ */
> >>+int cpu_throttle_get_percentage(void);
> >>+
> >>  #ifndef CONFIG_USER_ONLY
> >>
> >>  typedef void (*CPUInterruptHandler)(CPUState *, int);
> >>--
> >>1.9.1
> >>
> >--
> >Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
> 
> -- 
> -- Jason J. Herne (jjherne@linux.vnet.ibm.com)
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK