| Message ID | 1562915730-9490-1-git-send-email-wanpengli@tencent.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND] KVM: Boosting vCPUs that are delivering interrupts | expand |
On Fri, 12 Jul 2019 at 15:15, Wanpeng Li <kernellwp@gmail.com> wrote: > > From: Wanpeng Li <wanpengli@tencent.com> > > Inspired by commit 9cac38dd5d (KVM/s390: Set preempted flag during vcpu wakeup > and interrupt delivery), except the lock holder, we want to also boost vCPUs > that are delivering interrupts. Actually most smp_call_function_many calls are > synchronous ipi calls, the ipi target vCPUs are also good yield candidates. > This patch sets preempted flag during wakeup and interrupt delivery time. > I forgot to mention that I disable pv tlb shootdown during testing, function call interrupts are not easy to be triggered directly by userspace workloads, in addition, distros' guest kernel w/o pv tlb shootdown support can also get benefit in both tlb shootdown and function call interrupts scenarios. > Testing on 80 HT 2 socket Xeon Skylake server, with 80 vCPUs VM 80GB RAM: > ebizzy -M > > vanilla boosting improved > 1VM 23000 21232 -9% > 2VM 2800 8000 180% > 3VM 1800 3100 72% > > Testing on my Haswell desktop 8 HT, with 8 vCPUs VM 8GB RAM, two VMs, > one running ebizzy -M, the other running 'stress --cpu 2': > > w/ boosting + w/o pv sched yield(vanilla) > > vanilla boosting improved > 1570 4000 55% > > w/ boosting + w/ pv sched yield(vanilla) > > vanilla boosting improved > 1844 5157 79% > > w/o boosting, perf top in VM: > > 72.33% [kernel] [k] smp_call_function_many > 4.22% [kernel] [k] call_function_i > 3.71% [kernel] [k] async_page_fault > > w/ boosting, perf top in VM: > > 38.43% [kernel] [k] smp_call_function_many > 6.31% [kernel] [k] async_page_fault > 6.13% libc-2.23.so [.] __memcpy_avx_unaligned > 4.88% [kernel] [k] call_function_interrupt > > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Radim Krčmář <rkrcmar@redhat.com> > Cc: Christian Borntraeger <borntraeger@de.ibm.com> > Signed-off-by: Wanpeng Li <wanpengli@tencent.com> > --- > virt/kvm/kvm_main.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index b4ab59d..2c46705 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -2404,8 +2404,10 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu) > int me; > int cpu = vcpu->cpu; > > - if (kvm_vcpu_wake_up(vcpu)) > + if (kvm_vcpu_wake_up(vcpu)) { > + vcpu->preempted = true; > return; > + } > > me = get_cpu(); > if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu)) > -- > 2.7.4 >
Cc arm guy's latest email On Fri, 12 Jul 2019 at 15:15, Wanpeng Li <kernellwp@gmail.com> wrote: > > From: Wanpeng Li <wanpengli@tencent.com> > > Inspired by commit 9cac38dd5d (KVM/s390: Set preempted flag during vcpu wakeup > and interrupt delivery), except the lock holder, we want to also boost vCPUs > that are delivering interrupts. Actually most smp_call_function_many calls are > synchronous ipi calls, the ipi target vCPUs are also good yield candidates. > This patch sets preempted flag during wakeup and interrupt delivery time. > > Testing on 80 HT 2 socket Xeon Skylake server, with 80 vCPUs VM 80GB RAM: > ebizzy -M > > vanilla boosting improved > 1VM 23000 21232 -9% > 2VM 2800 8000 180% > 3VM 1800 3100 72% > > Testing on my Haswell desktop 8 HT, with 8 vCPUs VM 8GB RAM, two VMs, > one running ebizzy -M, the other running 'stress --cpu 2': > > w/ boosting + w/o pv sched yield(vanilla) > > vanilla boosting improved > 1570 4000 55% > > w/ boosting + w/ pv sched yield(vanilla) > > vanilla boosting improved > 1844 5157 79% > > w/o boosting, perf top in VM: > > 72.33% [kernel] [k] smp_call_function_many > 4.22% [kernel] [k] call_function_i > 3.71% [kernel] [k] async_page_fault > > w/ boosting, perf top in VM: > > 38.43% [kernel] [k] smp_call_function_many > 6.31% [kernel] [k] async_page_fault > 6.13% libc-2.23.so [.] __memcpy_avx_unaligned > 4.88% [kernel] [k] call_function_interrupt > > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Radim Krčmář <rkrcmar@redhat.com> > Cc: Christian Borntraeger <borntraeger@de.ibm.com> > Signed-off-by: Wanpeng Li <wanpengli@tencent.com> > --- > virt/kvm/kvm_main.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index b4ab59d..2c46705 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -2404,8 +2404,10 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu) > int me; > int cpu = vcpu->cpu; > > - if (kvm_vcpu_wake_up(vcpu)) > + if (kvm_vcpu_wake_up(vcpu)) { > + vcpu->preempted = true; > return; > + } > > me = get_cpu(); > if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu)) > -- > 2.7.4 >
On 12/07/19 09:15, Wanpeng Li wrote: > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index b4ab59d..2c46705 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -2404,8 +2404,10 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu) > int me; > int cpu = vcpu->cpu; > > - if (kvm_vcpu_wake_up(vcpu)) > + if (kvm_vcpu_wake_up(vcpu)) { > + vcpu->preempted = true; > return; > + } > > me = get_cpu(); > if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu)) > Who is resetting vcpu->preempted to false in this case? This also applies to s390 in fact. Paolo
On 18.07.19 09:59, Paolo Bonzini wrote: > On 12/07/19 09:15, Wanpeng Li wrote: >> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c >> index b4ab59d..2c46705 100644 >> --- a/virt/kvm/kvm_main.c >> +++ b/virt/kvm/kvm_main.c >> @@ -2404,8 +2404,10 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu) >> int me; >> int cpu = vcpu->cpu; >> >> - if (kvm_vcpu_wake_up(vcpu)) >> + if (kvm_vcpu_wake_up(vcpu)) { >> + vcpu->preempted = true; >> return; >> + } >> >> me = get_cpu(); >> if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu)) >> > > Who is resetting vcpu->preempted to false in this case? This also > applies to s390 in fact. Isnt that done by the sched_in handler?
On 18/07/19 10:15, Christian Borntraeger wrote: > > > On 18.07.19 09:59, Paolo Bonzini wrote: >> On 12/07/19 09:15, Wanpeng Li wrote: >>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c >>> index b4ab59d..2c46705 100644 >>> --- a/virt/kvm/kvm_main.c >>> +++ b/virt/kvm/kvm_main.c >>> @@ -2404,8 +2404,10 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu) >>> int me; >>> int cpu = vcpu->cpu; >>> >>> - if (kvm_vcpu_wake_up(vcpu)) >>> + if (kvm_vcpu_wake_up(vcpu)) { >>> + vcpu->preempted = true; >>> return; >>> + } >>> >>> me = get_cpu(); >>> if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu)) >>> >> >> Who is resetting vcpu->preempted to false in this case? This also >> applies to s390 in fact. > > Isnt that done by the sched_in handler? I am a bit confused because, if it is done by the sched_in later, I don't understand why the sched_out handler hasn't set vcpu->preempted already. The s390 commit message is not very clear, but it talks about "a former sleeping cpu" that "gave up the cpu voluntarily". Does "voluntarily" that mean it is in kvm_vcpu_block? But then at least for x86 it would be after vcpu_load so the preempt notifiers have been registered, and for s390 too (kvm_arch_vcpu_ioctl_run -> __vcpu_run -> vcpu_post_run -> kvm_handle_sie_intercept etc.). Paolo
On Thu, 18 Jul 2019 at 16:34, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 18/07/19 10:15, Christian Borntraeger wrote: > > > > > > On 18.07.19 09:59, Paolo Bonzini wrote: > >> On 12/07/19 09:15, Wanpeng Li wrote: > >>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > >>> index b4ab59d..2c46705 100644 > >>> --- a/virt/kvm/kvm_main.c > >>> +++ b/virt/kvm/kvm_main.c > >>> @@ -2404,8 +2404,10 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu) > >>> int me; > >>> int cpu = vcpu->cpu; > >>> > >>> - if (kvm_vcpu_wake_up(vcpu)) > >>> + if (kvm_vcpu_wake_up(vcpu)) { > >>> + vcpu->preempted = true; > >>> return; > >>> + } > >>> > >>> me = get_cpu(); > >>> if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu)) > >>> > >> > >> Who is resetting vcpu->preempted to false in this case? This also > >> applies to s390 in fact. > > > > Isnt that done by the sched_in handler? > > I am a bit confused because, if it is done by the sched_in later, I > don't understand why the sched_out handler hasn't set vcpu->preempted > already. > > The s390 commit message is not very clear, but it talks about "a former > sleeping cpu" that "gave up the cpu voluntarily". Does "voluntarily" > that mean it is in kvm_vcpu_block? But then at least for x86 it would see the prepare_to_swait_exlusive() in kvm_vcpu_block(), the task will be set in TASK_INTERRUPTIBLE state, kvm_sched_out will set vcpu->preempted to true iff current->state == TASK_RUNNING. Regards, Wanpeng Li
On 18/07/19 10:43, Wanpeng Li wrote: >>> Isnt that done by the sched_in handler? >> >> I am a bit confused because, if it is done by the sched_in later, I >> don't understand why the sched_out handler hasn't set vcpu->preempted >> already. >> >> The s390 commit message is not very clear, but it talks about "a former >> sleeping cpu" that "gave up the cpu voluntarily". Does "voluntarily" >> that mean it is in kvm_vcpu_block? But then at least for x86 it would > > see the prepare_to_swait_exlusive() in kvm_vcpu_block(), the task will > be set in TASK_INTERRUPTIBLE state, kvm_sched_out will set > vcpu->preempted to true iff current->state == TASK_RUNNING. Ok, I was totally blind to that "if" around vcpu->preempted = true, it's obvious now. I think we need two flags then, for example vcpu->preempted and vcpu->ready: - kvm_sched_out sets both of them to true iff current->state == TASK_RUNNING - kvm_vcpu_kick sets vcpu->ready to true - kvm_sched_in clears both of them This way, vmx_vcpu_pi_load can keep looking at preempted only (it handles voluntary preemption in pi_pre_block/pi_post_block). Also, kvm_s390_vcpu_wakeup can be changed to use kvm_vcpu_wake_up, which is nice. Paolo
On Thu, 18 Jul 2019 at 17:07, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 18/07/19 10:43, Wanpeng Li wrote: > >>> Isnt that done by the sched_in handler? > >> > >> I am a bit confused because, if it is done by the sched_in later, I > >> don't understand why the sched_out handler hasn't set vcpu->preempted > >> already. > >> > >> The s390 commit message is not very clear, but it talks about "a former > >> sleeping cpu" that "gave up the cpu voluntarily". Does "voluntarily" > >> that mean it is in kvm_vcpu_block? But then at least for x86 it would > > > > see the prepare_to_swait_exlusive() in kvm_vcpu_block(), the task will > > be set in TASK_INTERRUPTIBLE state, kvm_sched_out will set > > vcpu->preempted to true iff current->state == TASK_RUNNING. > > Ok, I was totally blind to that "if" around vcpu->preempted = true, it's > obvious now. > > I think we need two flags then, for example vcpu->preempted and vcpu->ready: > > - kvm_sched_out sets both of them to true iff current->state == TASK_RUNNING > > - kvm_vcpu_kick sets vcpu->ready to true > > - kvm_sched_in clears both of them > > This way, vmx_vcpu_pi_load can keep looking at preempted only (it > handles voluntary preemption in pi_pre_block/pi_post_block). > > Also, kvm_s390_vcpu_wakeup can be changed to use kvm_vcpu_wake_up, which > is nice. Will do. :) Wanpeng
On 18/07/19 11:29, Wanpeng Li wrote: > On Thu, 18 Jul 2019 at 17:07, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> On 18/07/19 10:43, Wanpeng Li wrote: >>>>> Isnt that done by the sched_in handler? >>>> >>>> I am a bit confused because, if it is done by the sched_in later, I >>>> don't understand why the sched_out handler hasn't set vcpu->preempted >>>> already. >>>> >>>> The s390 commit message is not very clear, but it talks about "a former >>>> sleeping cpu" that "gave up the cpu voluntarily". Does "voluntarily" >>>> that mean it is in kvm_vcpu_block? But then at least for x86 it would >>> >>> see the prepare_to_swait_exlusive() in kvm_vcpu_block(), the task will >>> be set in TASK_INTERRUPTIBLE state, kvm_sched_out will set >>> vcpu->preempted to true iff current->state == TASK_RUNNING. >> >> Ok, I was totally blind to that "if" around vcpu->preempted = true, it's >> obvious now. >> >> I think we need two flags then, for example vcpu->preempted and vcpu->ready: >> >> - kvm_sched_out sets both of them to true iff current->state == TASK_RUNNING >> >> - kvm_vcpu_kick sets vcpu->ready to true >> >> - kvm_sched_in clears both of them ... and also kvm_vcpu_on_spin should check vcpu->ready. vcpu->preempted remains only for use by vmx_vcpu_pi_put. Later we could think of removing vcpu->preempted. For example, kvm_arch_sched_out and kvm_x86_ops->sched_out could get the code currently in vmx_vcpu_pi_put (testing curent->state == TASK_RUNNING instead of vcpu->preempted). But for now there's no need and I'm not sure it's an improvement at all. Paolo >> This way, vmx_vcpu_pi_load can keep looking at preempted only (it >> handles voluntary preemption in pi_pre_block/pi_post_block).
On Thu, 18 Jul 2019 at 17:39, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 18/07/19 11:29, Wanpeng Li wrote: > > On Thu, 18 Jul 2019 at 17:07, Paolo Bonzini <pbonzini@redhat.com> wrote: > >> > >> On 18/07/19 10:43, Wanpeng Li wrote: > >>>>> Isnt that done by the sched_in handler? > >>>> > >>>> I am a bit confused because, if it is done by the sched_in later, I > >>>> don't understand why the sched_out handler hasn't set vcpu->preempted > >>>> already. > >>>> > >>>> The s390 commit message is not very clear, but it talks about "a former > >>>> sleeping cpu" that "gave up the cpu voluntarily". Does "voluntarily" > >>>> that mean it is in kvm_vcpu_block? But then at least for x86 it would > >>> > >>> see the prepare_to_swait_exlusive() in kvm_vcpu_block(), the task will > >>> be set in TASK_INTERRUPTIBLE state, kvm_sched_out will set > >>> vcpu->preempted to true iff current->state == TASK_RUNNING. > >> > >> Ok, I was totally blind to that "if" around vcpu->preempted = true, it's > >> obvious now. > >> > >> I think we need two flags then, for example vcpu->preempted and vcpu->ready: > >> > >> - kvm_sched_out sets both of them to true iff current->state == TASK_RUNNING > >> > >> - kvm_vcpu_kick sets vcpu->ready to true > >> > >> - kvm_sched_in clears both of them > > ... and also kvm_vcpu_on_spin should check vcpu->ready. vcpu->preempted > remains only for use by vmx_vcpu_pi_put. Done in v2, please have a look. :) Regards, Wanpeng Li
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index b4ab59d..2c46705 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -2404,8 +2404,10 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu) int me; int cpu = vcpu->cpu; - if (kvm_vcpu_wake_up(vcpu)) + if (kvm_vcpu_wake_up(vcpu)) { + vcpu->preempted = true; return; + } me = get_cpu(); if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu))