From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49970) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZAIbt-0002K1-PT for qemu-devel@nongnu.org; Wed, 01 Jul 2015 10:03:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZAIbq-0006dB-F6 for qemu-devel@nongnu.org; Wed, 01 Jul 2015 10:03:33 -0400 Received: from mail-wi0-x22f.google.com ([2a00:1450:400c:c05::22f]:37480) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZAIbp-0006cl-V7 for qemu-devel@nongnu.org; Wed, 01 Jul 2015 10:03:30 -0400 Received: by wicgi11 with SMTP id gi11so46533012wic.0 for ; Wed, 01 Jul 2015 07:03:29 -0700 (PDT) Sender: Paolo Bonzini References: <1435254377-13322-1-git-send-email-jjherne@linux.vnet.ibm.com> <1435254377-13322-2-git-send-email-jjherne@linux.vnet.ibm.com> <20150626180702.GK2186@work-vm> From: Paolo Bonzini Message-ID: <5593F32F.6090909@redhat.com> Date: Wed, 1 Jul 2015 16:03:27 +0200 MIME-Version: 1.0 In-Reply-To: <20150626180702.GK2186@work-vm> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v3 1/5] cpu: Provide vcpu throttling interface List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert" , "Jason J. Herne" Cc: amit.shah@redhat.com, borntraeger@de.ibm.com, qemu-devel@nongnu.org, afaerber@suse.de, quintela@redhat.com On 26/06/2015 20:07, 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. Agreed. I think the only atomic should be throttle_percentage; if zero, throttling is inactive. In particular, throttle_ratio can be computed in cpu_throttle_thread. If you have exactly one variable that is shared between the threads, everything is much simpler. There is no need to allocate and free the timer; it's very cheap and in fact we probably should convert to statically allocated timers sooner or later. So you can just create it once, for example in cpu_ticks_init. Paolo > So, probably need some atomic's in there (cc'ing paolo) > > Dave > >> Signed-off-by: Jason J. Herne >> Reviewed-by: Matthew Rosato >> --- >> 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? > >> +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. > >> + >> +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 > >