| Message ID | 1435254377-13322-2-git-send-email-jjherne@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
* 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 > 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? > +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
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? >> + >> +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) 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
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 <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? > >>> + >>> +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 >> >
On 25/06/2015 19:46, Jason J. Herne wrote: > +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); The timer need not run while the VM is stopped. Please use QEMU_CLOCK_VIRTUAL_RT. > +} > + Are you sure you want each CPU to set the timer? I think this should be done in cpu_throttle_timer_pop, or it could use timer_mod_anticipate. > +static void cpu_throttle_timer_pop(void *opaque) > +{ > + CPUState *cpu; > + > + /* Stop the timer if needed */ > + if (throttle_timer_stop) { > + timer_del(throttle_timer); timer_del is not needed in the timer callback. Paolo > + timer_free(throttle_timer); > + throttle_timer = NULL; > + return; > + } > + > + CPU_FOREACH(cpu) { > + async_run_on_cpu(cpu, cpu_throttle_thread, NULL); > + } > +} > +
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 <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? > >> +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 > >
On 07/01/2015 10:03 AM, Paolo Bonzini wrote: > > > 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 > Hi Paolo, I like your suggestions. I'll switch to a static timer and create it in cpu_ticks_init. So no need for a timer_free anywhere then?
On 02/07/2015 16:25, Jason J. Herne wrote: > Hi Paolo, > I like your suggestions. I'll switch to a static timer and create it in > cpu_ticks_init. > So no need for a timer_free anywhere then? Nope. :) Paolo
On 07/01/2015 10:03 AM, Paolo Bonzini wrote: > > > 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. > I've made all of the changes you have suggested except adding atomics. I'm having a bit of trouble figuring out what is needed here. Perhaps I should be using atomic_read() to read throttle_percentage? If so, I don't undertand why. Rather than trying to explain everything here I'm going to submit my V4 patches. If any atomic operations are still necessary with V4 please let me know.
On 02/07/2015 18:33, Jason J. Herne wrote: > I've made all of the changes you have suggested except adding atomics. I'm having > a bit of trouble figuring out what is needed here. Perhaps I should be using > atomic_read() to read throttle_percentage? If so, I don't undertand why. Rather > than trying to explain everything here I'm going to submit my V4 patches. > If any atomic operations are still necessary with V4 please let me know. Fair enough! Paolo
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; +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); + } +} + +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);