From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37574) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z9aGo-00043n-Mb for qemu-devel@nongnu.org; Mon, 29 Jun 2015 10:42:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z9aGk-0001oX-KM for qemu-devel@nongnu.org; Mon, 29 Jun 2015 10:42:50 -0400 Received: from e17.ny.us.ibm.com ([129.33.205.207]:59617) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z9aGk-0001o1-Dy for qemu-devel@nongnu.org; Mon, 29 Jun 2015 10:42:46 -0400 Received: from /spool/local by e17.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 29 Jun 2015 10:42:45 -0400 Received: from b01cxnp23034.gho.pok.ibm.com (b01cxnp23034.gho.pok.ibm.com [9.57.198.29]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id 6EC6738C8026 for ; Mon, 29 Jun 2015 10:42:43 -0400 (EDT) Received: from d01av04.pok.ibm.com (d01av04.pok.ibm.com [9.56.224.64]) by b01cxnp23034.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t5TEgheM57540618 for ; Mon, 29 Jun 2015 14:42:43 GMT Received: from d01av04.pok.ibm.com (localhost [127.0.0.1]) by d01av04.pok.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t5TEgg5h027275 for ; Mon, 29 Jun 2015 10:42:43 -0400 Message-ID: <55915962.5040903@linux.vnet.ibm.com> Date: Mon, 29 Jun 2015 10:42:42 -0400 From: "Jason J. Herne" MIME-Version: 1.0 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> <558DA1B8.2090006@linux.vnet.ibm.com> In-Reply-To: <558DA1B8.2090006@linux.vnet.ibm.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 1/5] cpu: Provide vcpu throttling interface Reply-To: jjherne@linux.vnet.ibm.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert" , pbonzini@redhat.com Cc: amit.shah@redhat.com, borntraeger@de.ibm.com, qemu-devel@nongnu.org, afaerber@suse.de, quintela@redhat.com On 06/26/2015 03:02 PM, Jason J. Herne 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 :) > Is there any more discussion on this before I send a v4? Paolo? >>> 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? >> > > 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? > >>> + >>> +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)