* Jason J. Herne (jjhe...@linux.vnet.ibm.com) wrote:
> On 06/03/2015 03:56 AM, Juan Quintela wrote:
> >"Jason J. Herne" <jjhe...@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 start function and provide a
> >>ratio of
> >>sleep time to normal execution time.
> >>
> >>Signed-off-by: Jason J. Herne <jjhe...@linux.vnet.ibm.com>
> >>Reviewed-by: Matthew Rosato <mjros...@linux.vnet.ibm.com>
> >
> >
> >
> >Hi
> >
> >sorry that I replied to your previous submission, I had "half done" the
> >mail from yesterday, I still think that all applies.
> >
> >
> >>---
> >> cpus.c | 62
> >> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> include/qom/cpu.h | 46 +++++++++++++++++++++++++++++++++++++++++
> >> 2 files changed, 108 insertions(+)
> >>
> >>diff --git a/cpus.c b/cpus.c
> >>index de6469f..7568357 100644
> >>--- a/cpus.c
> >>+++ b/cpus.c
> >>@@ -64,6 +64,9 @@
> >>
> >> #endif /* CONFIG_LINUX */
> >>
> >>+/* Number of ms between cpu throttle operations */
> >>+#define CPU_THROTTLE_TIMESLICE 10
> >
> >
> >We are checking for throotling on each cpu each 10ms.
> >But on patch 2 we can see that we only change the throotling each
> >time that we call migration_bitmap_sync(), that only happens each round
> >through all the pages. Normally auto-converge only matters for machines
> >with lots of memory, so this is going to happen each more than 10ms (we
> >change it each 4 passes). You changed it to each 2 passes, and you add
> >it a 0.2. I think that I would preffer to just have it each single
> >pass, but add a 0.1 each pass? simpler and end result would be the same?
> >
> >
>
> Well, we certainly could make it run every pass but I think it would get
> a little too aggressive then. The reason is, we do not increment the
> throttle
> rate by adding 0.2 each time. We increment it by multiplying the current
> rate
> by 2. So by doing that every pass we are doubling the exponential growth
> rate. I will admit the numbers I chose are hardly scientific... I chose them
> because they seemed to provide a decent balance of "throttling aggression"
> in
> my workloads.
That's the advantage of making them parameters.
Dave
> >This is what the old code did (new code do exactly the same, just in the
> >previous patch)
> >
> >-static void mig_sleep_cpu(void *opq)
> >-{
> >- qemu_mutex_unlock_iothread();
> >- g_usleep(30*1000);
> >- qemu_mutex_lock_iothread();
> >-}
> >
> >So, what we are doing, calling async_run_on_cpu() with this function.
> >
> >To make things short, we end here:
> >
> >static void flush_queued_work(CPUState *cpu)
> >{
> > struct qemu_work_item *wi;
> >
> > if (cpu->queued_work_first == NULL) {
> > return;
> > }
> >
> > while ((wi = cpu->queued_work_first)) {
> > cpu->queued_work_first = wi->next;
> > wi->func(wi->data);
> > wi->done = true;
> > if (wi->free) {
> > g_free(wi);
> > }
> > }
> > cpu->queued_work_last = NULL;
> > qemu_cond_broadcast(&qemu_work_cond);
> >}
> >
> >So, we are walking a list that is protected with the iothread_lock, and
> >we are dropping the lock in the middle of the walk ..... Just hoping
> >that nothing else is calling run_async_on_cpu() from a different place
> >....
> >
> >
> >Could we change this to something like:
> >
> >diff --git a/kvm-all.c b/kvm-all.c
> >index 17a3771..ae9635f 100644
> >--- a/kvm-all.c
> >+++ b/kvm-all.c
> >@@ -1787,6 +1787,7 @@ int kvm_cpu_exec(CPUState *cpu)
> > {
> > struct kvm_run *run = cpu->kvm_run;
> > int ret, run_ret;
> >+ long throotling_time;
> >
> > DPRINTF("kvm_cpu_exec()\n");
> >
> >@@ -1813,8 +1814,15 @@ int kvm_cpu_exec(CPUState *cpu)
> > */
> > qemu_cpu_kick_self();
> > }
> >+ throotling_time = cpu->throotling_time;
> >+ cpu->throotling_time = 0;
> >+ cpu->sleeping_time += throotling_time;
> > qemu_mutex_unlock_iothread();
> >
> >+
> >+ if (throotling_time) {
> >+ g_usleep(throttling_time);
> >+ }
> > run_ret = kvm_vcpu_ioctl(cpu, KVM_RUN, 0);
> >
> > qemu_mutex_lock_iothread();
> >
> >
> >
> >And we handle where we are doing now what throotling_time and
> >sleeping_time should be? No need to drop throotling_time/lock/whatever.
> >
> >It adds an if on the fast path, but we have a call to the hypervisor
> >each time, so it shouldn't be so bad, no?
> >
> >For tcp/xen we should seach for a different place to put this code, but
> >you get the idea. Reason for putting it here is because this is where
> >the iothread is dropped, so we don't lost any locking, any other place
> >that we put it needs review with respect to this.
> >
> >
> >Jason, I am really, really sorry to have open this big can of worms on
> >your patch. Now you know why I was telling that "improving"
> >autoconverge was not as easy as it looked.
> >
> >With respect ot the last comment, I also think that we can include the
> >Jason patches. They are one imprevement over what we have now. It is
> >just that it needs more improvements.
> >
>
> Yes, I understand what you are suggesting here. I can certainly look into it
> if you would prefer that rather than accept the current design. The reason I
> did things using the timer and thread was because it fit into the old
> auto-converge code very nicely. Does it make sense to go forward with my
> current design (modified as per your earlier suggestions), and then follow
> up with your proposal at a later date?
>
> --
> -- Jason J. Herne (jjhe...@linux.vnet.ibm.com)
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
