Re: [Qemu-devel] [PATCH] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS

[Eduardo Habkost]

On Sun, Mar 25, 2018 at 11:33:01AM +0800, Wanpeng Li wrote:
> 2018-03-24 4:18 GMT+08:00 Eduardo Habkost <address@hidden>:
> > On Fri, Mar 16, 2018 at 07:36:42AM -0700, Wanpeng Li wrote:
> >> From: Wanpeng Li <address@hidden>
> >>
> >> 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 <address@hidden>
> >> Cc: Radim Krčmář <address@hidden>
> >> Cc: Eduardo Habkost <address@hidden>
> >> Signed-off-by: Wanpeng Li <address@hidden>
> >
> >
> > 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.

-- 
Eduardo