| Message ID | 1521211002-4529-1-git-send-email-wanpengli@tencent.com |
|---|---|
| State | New |
| Headers | show |
| Series | i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS | expand |
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 1521211002-4529-1-git-send-email-wanpengli@tencent.com
Subject: [Qemu-devel] [PATCH] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
* [new tag] patchew/1521211002-4529-1-git-send-email-wanpengli@tencent.com -> patchew/1521211002-4529-1-git-send-email-wanpengli@tencent.com
t [tag update] patchew/20180315034507.6341-1-famz@redhat.com -> patchew/20180315034507.6341-1-famz@redhat.com
Switched to a new branch 'test'
9ababe35bc i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS
=== OUTPUT BEGIN ===
Checking PATCH 1/1: i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS...
WARNING: line over 80 characters
#52: FILE: target/i386/kvm.c:1003:
+ int disable_exits = kvm_check_extension(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS);
ERROR: line over 90 characters
#58: FILE: target/i386/kvm.c:1009:
+ if (kvm_vm_enable_cap(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS, 0, disable_exits)) {
total: 1 errors, 1 warnings, 36 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
2018-03-16 22:41 GMT+08:00 <no-reply@patchew.org>: > Hi, > > This series seems to have some coding style problems. See output below for > more information: > > Type: series > Message-id: 1521211002-4529-1-git-send-email-wanpengli@tencent.com > Subject: [Qemu-devel] [PATCH] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS > > === TEST SCRIPT BEGIN === > #!/bin/bash > > BASE=base > n=1 > total=$(git log --oneline $BASE.. | wc -l) > failed=0 > > git config --local diff.renamelimit 0 > git config --local diff.renames True > git config --local diff.algorithm histogram > > commits="$(git log --format=%H --reverse $BASE..)" > for c in $commits; do > echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." > if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then > failed=1 > echo > fi > n=$((n+1)) > done > > exit $failed > === TEST SCRIPT END === > > Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 > From https://github.com/patchew-project/qemu > * [new tag] patchew/1521211002-4529-1-git-send-email-wanpengli@tencent.com -> patchew/1521211002-4529-1-git-send-email-wanpengli@tencent.com > t [tag update] patchew/20180315034507.6341-1-famz@redhat.com -> patchew/20180315034507.6341-1-famz@redhat.com > Switched to a new branch 'test' > 9ababe35bc i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS > > === OUTPUT BEGIN === > Checking PATCH 1/1: i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS... > WARNING: line over 80 characters > #52: FILE: target/i386/kvm.c:1003: > + int disable_exits = kvm_check_extension(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS); > > ERROR: line over 90 characters > #58: FILE: target/i386/kvm.c:1009: > + if (kvm_vm_enable_cap(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS, 0, disable_exits)) { > > total: 1 errors, 1 warnings, 36 lines checked I think it's fine, otherwise the format looks mess, anyway, please let me know if they still need to be less than 80 characters here. Regards, Wanpeng Li > > Your patch has style problems, please review. If any of these errors > are false positives report them to the maintainer, see > CHECKPATCH in MAINTAINERS. > > === OUTPUT END === > > Test command exited with code: 1 > > > --- > Email generated automatically by Patchew [http://patchew.org/]. > Please send your feedback to patchew-devel@freelists.org
On 16/03/2018 15:58, Wanpeng Li wrote: >> WARNING: line over 80 characters >> #52: FILE: target/i386/kvm.c:1003: >> + int disable_exits = kvm_check_extension(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS); >> >> ERROR: line over 90 characters >> #58: FILE: target/i386/kvm.c:1009: >> + if (kvm_vm_enable_cap(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS, 0, disable_exits)) { >> >> total: 1 errors, 1 warnings, 36 lines checked > I think it's fine, otherwise the format looks mess, anyway, please let > me know if they still need to be less than 80 characters here. Yes, it's okay. Paolo
On Fri, Mar 16, 2018 at 07:36:42AM -0700, Wanpeng Li wrote: > From: Wanpeng Li <wanpengli@tencent.com> > > This patch adds support for KVM_CAP_X86_DISABLE_EXITS. Provides userspace with > per-VM capability(KVM_CAP_X86_DISABLE_EXITS) to not intercept MWAIT/HLT/PAUSE > in order that to improve latency in some workloads. > > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Radim Krčmář <rkrcmar@redhat.com> > Cc: Eduardo Habkost <ehabkost@redhat.com> > Signed-off-by: Wanpeng Li <wanpengli@tencent.com> Thanks. Patch looks good (except for comment below), but I would like to see QEMU documentation mentioning what exactly are the practical consequences of setting "+kvm-hint-dedicated" (especially what could happen if people enable the flag without properly configuring vCPU pinning). [...] > + if (env->features[FEAT_KVM_HINTS] & KVM_HINTS_DEDICATED) { > + int disable_exits = kvm_check_extension(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS); > + if (disable_exits) { > + disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT | > + KVM_X86_DISABLE_EXITS_HLT | > + KVM_X86_DISABLE_EXITS_PAUSE); > + } Documentation/virtual/kvm/api.txt says that KVM_FEATURE_PV_UNHALT shouldn't be enabled if disabling HLT exits. This needs to be handled by QEMU. Probably the simplest solution is to not allow kvm-hint-dedicated to be enabled if kvm-pv-unhalt is. This should be mentioned in QEMU documentation, also, especially considering that we might enable kvm-pv-unhalt by default in future QEMU versions. > + if (kvm_vm_enable_cap(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS, 0, disable_exits)) { > + error_report("kvm: DISABLE EXITS not supported"); > + } > + } > + > qemu_add_vm_change_state_handler(cpu_update_state, env); > > c = cpuid_find_entry(&cpuid_data.cpuid, 1, 0); > -- > 2.7.4 > >
2018-03-24 4:18 GMT+08:00 Eduardo Habkost <ehabkost@redhat.com>: > On Fri, Mar 16, 2018 at 07:36:42AM -0700, Wanpeng Li wrote: >> From: Wanpeng Li <wanpengli@tencent.com> >> >> This patch adds support for KVM_CAP_X86_DISABLE_EXITS. Provides userspace with >> per-VM capability(KVM_CAP_X86_DISABLE_EXITS) to not intercept MWAIT/HLT/PAUSE >> in order that to improve latency in some workloads. >> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Cc: Radim Krčmář <rkrcmar@redhat.com> >> Cc: Eduardo Habkost <ehabkost@redhat.com> >> Signed-off-by: Wanpeng Li <wanpengli@tencent.com> > > > Thanks. > > Patch looks good (except for comment below), but I would like to > see QEMU documentation mentioning what exactly are the practical > consequences of setting "+kvm-hint-dedicated" (especially what > could happen if people enable the flag without properly > configuring vCPU pinning). > > > [...] >> + if (env->features[FEAT_KVM_HINTS] & KVM_HINTS_DEDICATED) { >> + int disable_exits = kvm_check_extension(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS); >> + if (disable_exits) { >> + disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT | >> + KVM_X86_DISABLE_EXITS_HLT | >> + KVM_X86_DISABLE_EXITS_PAUSE); >> + } > > Documentation/virtual/kvm/api.txt says that KVM_FEATURE_PV_UNHALT > shouldn't be enabled if disabling HLT exits. This needs to be > handled by QEMU. This is handled by KVM(in kvm_update_cpuid()) currently to avoid kvm userspace doing something crazy. https://git.kernel.org/pub/scm/virt/kvm/kvm.git/commit/?h=queue&id=caa057a2cad647fb368a12c8e6c410ac4c28e063 > > Probably the simplest solution is to not allow kvm-hint-dedicated > to be enabled if kvm-pv-unhalt is. This should be mentioned in > QEMU documentation, also, especially considering that we might > enable kvm-pv-unhalt by default in future QEMU versions. As Locking guy Waiman mentioned before: > Generally speaking, unfair lock performs well for VMs with a small number of vCPUs. Native qspinlock may perform better than pvqspinlock if there is vCPU pinning and there is no vCPU over-commitment. I think +kvm-hint-dedicated, -kvm-pv-unhalt is more suitable for vCPU pinning and there is no vCPU over-commitment, on the contrary, -kvm-hint-dedicated, +kvm-pv-unhalt is more prefer. Regards, Wanpeng Li
On Sun, Mar 25, 2018 at 11:33:01AM +0800, Wanpeng Li wrote: > 2018-03-24 4:18 GMT+08:00 Eduardo Habkost <ehabkost@redhat.com>: > > On Fri, Mar 16, 2018 at 07:36:42AM -0700, Wanpeng Li wrote: > >> From: Wanpeng Li <wanpengli@tencent.com> > >> > >> This patch adds support for KVM_CAP_X86_DISABLE_EXITS. Provides userspace with > >> per-VM capability(KVM_CAP_X86_DISABLE_EXITS) to not intercept MWAIT/HLT/PAUSE > >> in order that to improve latency in some workloads. > >> > >> Cc: Paolo Bonzini <pbonzini@redhat.com> > >> Cc: Radim Krčmář <rkrcmar@redhat.com> > >> Cc: Eduardo Habkost <ehabkost@redhat.com> > >> Signed-off-by: Wanpeng Li <wanpengli@tencent.com> > > > > > > Thanks. > > > > Patch looks good (except for comment below), but I would like to > > see QEMU documentation mentioning what exactly are the practical > > consequences of setting "+kvm-hint-dedicated" (especially what > > could happen if people enable the flag without properly > > configuring vCPU pinning). > > > > > > [...] > >> + if (env->features[FEAT_KVM_HINTS] & KVM_HINTS_DEDICATED) { > >> + int disable_exits = kvm_check_extension(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS); > >> + if (disable_exits) { > >> + disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT | > >> + KVM_X86_DISABLE_EXITS_HLT | > >> + KVM_X86_DISABLE_EXITS_PAUSE); > >> + } > > > > Documentation/virtual/kvm/api.txt says that KVM_FEATURE_PV_UNHALT > > shouldn't be enabled if disabling HLT exits. This needs to be > > handled by QEMU. > > This is handled by KVM(in kvm_update_cpuid()) currently to avoid kvm > userspace doing something crazy. > https://git.kernel.org/pub/scm/virt/kvm/kvm.git/commit/?h=queue&id=caa057a2cad647fb368a12c8e6c410ac4c28e063 This seems to disable kvm-pv-unhalt silently if KVM_X86_DISABLE_EXITS_HLT is enabled. We shouldn't do that if the user explicitly requested +kvm-pv-unhalt in the command-line. > > > > > Probably the simplest solution is to not allow kvm-hint-dedicated > > to be enabled if kvm-pv-unhalt is. This should be mentioned in > > QEMU documentation, also, especially considering that we might > > enable kvm-pv-unhalt by default in future QEMU versions. > > As Locking guy Waiman mentioned before: > > Generally speaking, unfair lock performs well for VMs with a small number of vCPUs. Native qspinlock may perform better than pvqspinlock if there is vCPU pinning and there is no vCPU over-commitment. > I think +kvm-hint-dedicated, -kvm-pv-unhalt is more suitable for vCPU > pinning and there is no vCPU over-commitment, on the contrary, > -kvm-hint-dedicated, +kvm-pv-unhalt is more prefer. Disabling kvm-pv-unhalt by default if only "-cpu ...,+kvm-hint-dedicated" is used makes sense. But we still need the system to not silently ignore options if "-cpu ...,+kvm-pv-unhalt,+kvm-hint-dedicated" is specified.
On Fri, Mar 16, 2018 at 07:36:42AM -0700, Wanpeng Li wrote: > From: Wanpeng Li <wanpengli@tencent.com> > > This patch adds support for KVM_CAP_X86_DISABLE_EXITS. Provides userspace with > per-VM capability(KVM_CAP_X86_DISABLE_EXITS) to not intercept MWAIT/HLT/PAUSE > in order that to improve latency in some workloads. > > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Radim Krčmář <rkrcmar@redhat.com> > Cc: Eduardo Habkost <ehabkost@redhat.com> > Signed-off-by: Wanpeng Li <wanpengli@tencent.com> > --- > linux-headers/linux/kvm.h | 6 +++++- > target/i386/kvm.c | 12 ++++++++++++ > 2 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h > index a167be8..857df15 100644 > --- a/linux-headers/linux/kvm.h > +++ b/linux-headers/linux/kvm.h > @@ -925,7 +925,7 @@ struct kvm_ppc_resize_hpt { > #define KVM_CAP_S390_GS 140 > #define KVM_CAP_S390_AIS 141 > #define KVM_CAP_SPAPR_TCE_VFIO 142 > -#define KVM_CAP_X86_GUEST_MWAIT 143 > +#define KVM_CAP_X86_DISABLE_EXITS 143 > #define KVM_CAP_ARM_USER_IRQ 144 > #define KVM_CAP_S390_CMMA_MIGRATION 145 > #define KVM_CAP_PPC_FWNMI 146 > @@ -1508,6 +1508,10 @@ struct kvm_assigned_msix_entry { > #define KVM_X2APIC_API_USE_32BIT_IDS (1ULL << 0) > #define KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK (1ULL << 1) > > +#define KVM_X86_DISABLE_EXITS_MWAIT (1 << 0) > +#define KVM_X86_DISABLE_EXITS_HLT (1 << 1) > +#define KVM_X86_DISABLE_EXITS_PAUSE (1 << 2) > + > /* Available with KVM_CAP_ARM_USER_IRQ */ > > /* Bits for run->s.regs.device_irq_level */ > diff --git a/target/i386/kvm.c b/target/i386/kvm.c > index d23fff1..95ed9eb 100644 > --- a/target/i386/kvm.c > +++ b/target/i386/kvm.c > @@ -999,6 +999,18 @@ int kvm_arch_init_vcpu(CPUState *cs) > } > } > > + if (env->features[FEAT_KVM_HINTS] & KVM_HINTS_DEDICATED) { > + int disable_exits = kvm_check_extension(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS); > + if (disable_exits) { > + disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT | > + KVM_X86_DISABLE_EXITS_HLT | > + KVM_X86_DISABLE_EXITS_PAUSE); > + } > + if (kvm_vm_enable_cap(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS, 0, disable_exits)) { > + error_report("kvm: DISABLE EXITS not supported"); > + } > + } > + > qemu_add_vm_change_state_handler(cpu_update_state, env); > > c = cpuid_find_entry(&cpuid_data.cpuid, 1, 0); Why not a bit per capability? I can see how someone might want to disable mwait exists but not the rest of them. > -- > 2.7.4
On Tue, Mar 27, 2018 at 10:42:56PM +0300, Michael S. Tsirkin wrote: > On Fri, Mar 16, 2018 at 07:36:42AM -0700, Wanpeng Li wrote: > > From: Wanpeng Li <wanpengli@tencent.com> > > > > This patch adds support for KVM_CAP_X86_DISABLE_EXITS. Provides userspace with > > per-VM capability(KVM_CAP_X86_DISABLE_EXITS) to not intercept MWAIT/HLT/PAUSE > > in order that to improve latency in some workloads. > > [...] > > diff --git a/target/i386/kvm.c b/target/i386/kvm.c > > index d23fff1..95ed9eb 100644 > > --- a/target/i386/kvm.c > > +++ b/target/i386/kvm.c > > @@ -999,6 +999,18 @@ int kvm_arch_init_vcpu(CPUState *cs) > > } > > } > > > > + if (env->features[FEAT_KVM_HINTS] & KVM_HINTS_DEDICATED) { > > + int disable_exits = kvm_check_extension(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS); > > + if (disable_exits) { > > + disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT | > > + KVM_X86_DISABLE_EXITS_HLT | > > + KVM_X86_DISABLE_EXITS_PAUSE); > > + } > > + if (kvm_vm_enable_cap(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS, 0, disable_exits)) { > > + error_report("kvm: DISABLE EXITS not supported"); > > + } > > + } > > + > > qemu_add_vm_change_state_handler(cpu_update_state, env); > > > > c = cpuid_find_entry(&cpuid_data.cpuid, 1, 0); > > Why not a bit per capability? > I can see how someone might want to disable mwait exists > but not the rest of them. kvm-hint-dedicated=on should be used only if the physical CPU is dedicated to the VCPU task. Are there any advantages of getting vmexits for HLT and PAUSE if no other task is going to use the CPU?
On Tue, Mar 27, 2018 at 06:36:46PM -0300, Eduardo Habkost wrote: > On Tue, Mar 27, 2018 at 10:42:56PM +0300, Michael S. Tsirkin wrote: > > On Fri, Mar 16, 2018 at 07:36:42AM -0700, Wanpeng Li wrote: > > > From: Wanpeng Li <wanpengli@tencent.com> > > > > > > This patch adds support for KVM_CAP_X86_DISABLE_EXITS. Provides userspace with > > > per-VM capability(KVM_CAP_X86_DISABLE_EXITS) to not intercept MWAIT/HLT/PAUSE > > > in order that to improve latency in some workloads. > > > > [...] > > > diff --git a/target/i386/kvm.c b/target/i386/kvm.c > > > index d23fff1..95ed9eb 100644 > > > --- a/target/i386/kvm.c > > > +++ b/target/i386/kvm.c > > > @@ -999,6 +999,18 @@ int kvm_arch_init_vcpu(CPUState *cs) > > > } > > > } > > > > > > + if (env->features[FEAT_KVM_HINTS] & KVM_HINTS_DEDICATED) { > > > + int disable_exits = kvm_check_extension(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS); > > > + if (disable_exits) { > > > + disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT | > > > + KVM_X86_DISABLE_EXITS_HLT | > > > + KVM_X86_DISABLE_EXITS_PAUSE); > > > + } > > > + if (kvm_vm_enable_cap(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS, 0, disable_exits)) { > > > + error_report("kvm: DISABLE EXITS not supported"); > > > + } > > > + } > > > + > > > qemu_add_vm_change_state_handler(cpu_update_state, env); > > > > > > c = cpuid_find_entry(&cpuid_data.cpuid, 1, 0); > > > > Why not a bit per capability? > > I can see how someone might want to disable mwait exists > > but not the rest of them. > > kvm-hint-dedicated=on should be used only if the physical CPU is > dedicated to the VCPU task. Are there any advantages of getting > vmexits for HLT and PAUSE if no other task is going to use the > CPU? No but there are advantages to using mwait even without a dedicated host CPU (VCPUs can wake up each other without exiting to hypervisor). Which is my point - there should be a separate flag to disable mwait exiting only. > -- > Eduardo
On Wed, Mar 28, 2018 at 03:06:23AM +0300, Michael S. Tsirkin wrote: > On Tue, Mar 27, 2018 at 06:36:46PM -0300, Eduardo Habkost wrote: > > On Tue, Mar 27, 2018 at 10:42:56PM +0300, Michael S. Tsirkin wrote: > > > On Fri, Mar 16, 2018 at 07:36:42AM -0700, Wanpeng Li wrote: > > > > From: Wanpeng Li <wanpengli@tencent.com> > > > > > > > > This patch adds support for KVM_CAP_X86_DISABLE_EXITS. Provides userspace with > > > > per-VM capability(KVM_CAP_X86_DISABLE_EXITS) to not intercept MWAIT/HLT/PAUSE > > > > in order that to improve latency in some workloads. > > > > > > [...] > > > > diff --git a/target/i386/kvm.c b/target/i386/kvm.c > > > > index d23fff1..95ed9eb 100644 > > > > --- a/target/i386/kvm.c > > > > +++ b/target/i386/kvm.c > > > > @@ -999,6 +999,18 @@ int kvm_arch_init_vcpu(CPUState *cs) > > > > } > > > > } > > > > > > > > + if (env->features[FEAT_KVM_HINTS] & KVM_HINTS_DEDICATED) { > > > > + int disable_exits = kvm_check_extension(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS); > > > > + if (disable_exits) { > > > > + disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT | > > > > + KVM_X86_DISABLE_EXITS_HLT | > > > > + KVM_X86_DISABLE_EXITS_PAUSE); > > > > + } > > > > + if (kvm_vm_enable_cap(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS, 0, disable_exits)) { > > > > + error_report("kvm: DISABLE EXITS not supported"); > > > > + } > > > > + } > > > > + > > > > qemu_add_vm_change_state_handler(cpu_update_state, env); > > > > > > > > c = cpuid_find_entry(&cpuid_data.cpuid, 1, 0); > > > > > > Why not a bit per capability? > > > I can see how someone might want to disable mwait exists > > > but not the rest of them. > > > > kvm-hint-dedicated=on should be used only if the physical CPU is > > dedicated to the VCPU task. Are there any advantages of getting > > vmexits for HLT and PAUSE if no other task is going to use the > > CPU? > > No but there are advantages to using mwait even without a dedicated host > CPU (VCPUs can wake up each other without exiting to hypervisor). Are there any downsides? What needs to be taken into account when deciding if mwait exits can be safely disabled? > > Which is my point - there should be a separate flag to disable mwait > exiting only. Adding new command-line option is possible, but not necessary for the dedicated-CPU use case. This means this patch is already useful without adding new flags.
On Wed, Mar 28, 2018 at 03:31:23PM -0300, Eduardo Habkost wrote: > On Wed, Mar 28, 2018 at 03:06:23AM +0300, Michael S. Tsirkin wrote: > > On Tue, Mar 27, 2018 at 06:36:46PM -0300, Eduardo Habkost wrote: > > > On Tue, Mar 27, 2018 at 10:42:56PM +0300, Michael S. Tsirkin wrote: > > > > On Fri, Mar 16, 2018 at 07:36:42AM -0700, Wanpeng Li wrote: > > > > > From: Wanpeng Li <wanpengli@tencent.com> > > > > > > > > > > This patch adds support for KVM_CAP_X86_DISABLE_EXITS. Provides userspace with > > > > > per-VM capability(KVM_CAP_X86_DISABLE_EXITS) to not intercept MWAIT/HLT/PAUSE > > > > > in order that to improve latency in some workloads. > > > > > > > > [...] > > > > > diff --git a/target/i386/kvm.c b/target/i386/kvm.c > > > > > index d23fff1..95ed9eb 100644 > > > > > --- a/target/i386/kvm.c > > > > > +++ b/target/i386/kvm.c > > > > > @@ -999,6 +999,18 @@ int kvm_arch_init_vcpu(CPUState *cs) > > > > > } > > > > > } > > > > > > > > > > + if (env->features[FEAT_KVM_HINTS] & KVM_HINTS_DEDICATED) { > > > > > + int disable_exits = kvm_check_extension(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS); > > > > > + if (disable_exits) { > > > > > + disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT | > > > > > + KVM_X86_DISABLE_EXITS_HLT | > > > > > + KVM_X86_DISABLE_EXITS_PAUSE); > > > > > + } > > > > > + if (kvm_vm_enable_cap(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS, 0, disable_exits)) { > > > > > + error_report("kvm: DISABLE EXITS not supported"); > > > > > + } > > > > > + } > > > > > + > > > > > qemu_add_vm_change_state_handler(cpu_update_state, env); > > > > > > > > > > c = cpuid_find_entry(&cpuid_data.cpuid, 1, 0); > > > > > > > > Why not a bit per capability? > > > > I can see how someone might want to disable mwait exists > > > > but not the rest of them. > > > > > > kvm-hint-dedicated=on should be used only if the physical CPU is > > > dedicated to the VCPU task. Are there any advantages of getting > > > vmexits for HLT and PAUSE if no other task is going to use the > > > CPU? > > > > No but there are advantages to using mwait even without a dedicated host > > CPU (VCPUs can wake up each other without exiting to hypervisor). > > Are there any downsides? What needs to be taken into account > when deciding if mwait exits can be safely disabled? Exit might take longer as it might need to wake up CPU from a deep C state. So one needs to know which C states are enabled on host and whether any tasks on the same CPU are latency sensitive. > > > > > Which is my point - there should be a separate flag to disable mwait > > exiting only. > > Adding new command-line option is possible, but not necessary for > the dedicated-CPU use case. This means this patch is already > useful without adding new flags. True. > -- > Eduardo
Hi Paolo, 2018-03-27 3:43 GMT+08:00 Eduardo Habkost <ehabkost@redhat.com>: > On Sun, Mar 25, 2018 at 11:33:01AM +0800, Wanpeng Li wrote: >> 2018-03-24 4:18 GMT+08:00 Eduardo Habkost <ehabkost@redhat.com>: >> > On Fri, Mar 16, 2018 at 07:36:42AM -0700, Wanpeng Li wrote: >> >> From: Wanpeng Li <wanpengli@tencent.com> >> >> >> >> This patch adds support for KVM_CAP_X86_DISABLE_EXITS. Provides userspace with >> >> per-VM capability(KVM_CAP_X86_DISABLE_EXITS) to not intercept MWAIT/HLT/PAUSE >> >> in order that to improve latency in some workloads. >> >> >> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> >> Cc: Radim Krčmář <rkrcmar@redhat.com> >> >> Cc: Eduardo Habkost <ehabkost@redhat.com> >> >> Signed-off-by: Wanpeng Li <wanpengli@tencent.com> >> > >> > >> > Thanks. >> > >> > Patch looks good (except for comment below), but I would like to >> > see QEMU documentation mentioning what exactly are the practical >> > consequences of setting "+kvm-hint-dedicated" (especially what >> > could happen if people enable the flag without properly >> > configuring vCPU pinning). >> > >> > >> > [...] >> >> + if (env->features[FEAT_KVM_HINTS] & KVM_HINTS_DEDICATED) { >> >> + int disable_exits = kvm_check_extension(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS); >> >> + if (disable_exits) { >> >> + disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT | >> >> + KVM_X86_DISABLE_EXITS_HLT | >> >> + KVM_X86_DISABLE_EXITS_PAUSE); >> >> + } >> > >> > Documentation/virtual/kvm/api.txt says that KVM_FEATURE_PV_UNHALT >> > shouldn't be enabled if disabling HLT exits. This needs to be >> > handled by QEMU. >> >> This is handled by KVM(in kvm_update_cpuid()) currently to avoid kvm >> userspace doing something crazy. >> https://git.kernel.org/pub/scm/virt/kvm/kvm.git/commit/?h=queue&id=caa057a2cad647fb368a12c8e6c410ac4c28e063 > > This seems to disable kvm-pv-unhalt silently if > KVM_X86_DISABLE_EXITS_HLT is enabled. We shouldn't do that if > the user explicitly requested +kvm-pv-unhalt in the command-line. > >> >> > >> > Probably the simplest solution is to not allow kvm-hint-dedicated >> > to be enabled if kvm-pv-unhalt is. This should be mentioned in >> > QEMU documentation, also, especially considering that we might >> > enable kvm-pv-unhalt by default in future QEMU versions. >> >> As Locking guy Waiman mentioned before: >> > Generally speaking, unfair lock performs well for VMs with a small number of vCPUs. Native qspinlock may perform better than pvqspinlock if there is vCPU pinning and there is no vCPU over-commitment. >> I think +kvm-hint-dedicated, -kvm-pv-unhalt is more suitable for vCPU >> pinning and there is no vCPU over-commitment, on the contrary, >> -kvm-hint-dedicated, +kvm-pv-unhalt is more prefer. > > Disabling kvm-pv-unhalt by default if only "-cpu > ...,+kvm-hint-dedicated" is used makes sense. But we still need > the system to not silently ignore options if > "-cpu ...,+kvm-pv-unhalt,+kvm-hint-dedicated" is specified. What's your opinion for these two comments from Eduardo? Regards, Wanpeng Li
diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h index a167be8..857df15 100644 --- a/linux-headers/linux/kvm.h +++ b/linux-headers/linux/kvm.h @@ -925,7 +925,7 @@ struct kvm_ppc_resize_hpt { #define KVM_CAP_S390_GS 140 #define KVM_CAP_S390_AIS 141 #define KVM_CAP_SPAPR_TCE_VFIO 142 -#define KVM_CAP_X86_GUEST_MWAIT 143 +#define KVM_CAP_X86_DISABLE_EXITS 143 #define KVM_CAP_ARM_USER_IRQ 144 #define KVM_CAP_S390_CMMA_MIGRATION 145 #define KVM_CAP_PPC_FWNMI 146 @@ -1508,6 +1508,10 @@ struct kvm_assigned_msix_entry { #define KVM_X2APIC_API_USE_32BIT_IDS (1ULL << 0) #define KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK (1ULL << 1) +#define KVM_X86_DISABLE_EXITS_MWAIT (1 << 0) +#define KVM_X86_DISABLE_EXITS_HLT (1 << 1) +#define KVM_X86_DISABLE_EXITS_PAUSE (1 << 2) + /* Available with KVM_CAP_ARM_USER_IRQ */ /* Bits for run->s.regs.device_irq_level */ diff --git a/target/i386/kvm.c b/target/i386/kvm.c index d23fff1..95ed9eb 100644 --- a/target/i386/kvm.c +++ b/target/i386/kvm.c @@ -999,6 +999,18 @@ int kvm_arch_init_vcpu(CPUState *cs) } } + if (env->features[FEAT_KVM_HINTS] & KVM_HINTS_DEDICATED) { + int disable_exits = kvm_check_extension(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS); + if (disable_exits) { + disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT | + KVM_X86_DISABLE_EXITS_HLT | + KVM_X86_DISABLE_EXITS_PAUSE); + } + if (kvm_vm_enable_cap(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS, 0, disable_exits)) { + error_report("kvm: DISABLE EXITS not supported"); + } + } + qemu_add_vm_change_state_handler(cpu_update_state, env); c = cpuid_find_entry(&cpuid_data.cpuid, 1, 0);