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

[Wanpeng Li]

Hi Paolo,
2018-03-27 3:43 GMT+08:00 Eduardo Habkost <address@hidden>:
> 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.

What's your opinion for these two comments from Eduardo?

Regards,
Wanpeng Li