Messages in this thread | | | Subject | Re: [PATCH v8 1/4] KVM: X86: Add vCPU running/preempted state | From | Paolo Bonzini <> | Date | Wed, 13 Dec 2017 13:31:16 +0100 |
| |
On 13/12/2017 12:45, David Hildenbrand wrote: > On 13.12.2017 12:38, Wanpeng Li wrote: >> 2017-12-13 18:20 GMT+08:00 David Hildenbrand <david@redhat.com>: >>> On 13.12.2017 02:33, Wanpeng Li wrote: >>>> From: Wanpeng Li <wanpeng.li@hotmail.com> >>>> >>>> This patch reuses the preempted field in kvm_steal_time, and will export >>>> the vcpu running/pre-empted information to the guest from host. This will >>>> enable guest to intelligently send ipi to running vcpus and set flag for >>>> pre-empted vcpus. This will prevent waiting for vcpus that are not running. >>>> >>>> Cc: Paolo Bonzini <pbonzini@redhat.com> >>>> Cc: Radim Krčmář <rkrcmar@redhat.com> >>>> Cc: Peter Zijlstra <peterz@infradead.org> >>>> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com> >>>> --- >>>> arch/x86/include/uapi/asm/kvm_para.h | 3 +++ >>>> arch/x86/kernel/kvm.c | 2 +- >>>> arch/x86/kvm/x86.c | 4 ++-- >>>> 3 files changed, 6 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h >>>> index 09cc064..763b692 100644 >>>> --- a/arch/x86/include/uapi/asm/kvm_para.h >>>> +++ b/arch/x86/include/uapi/asm/kvm_para.h >>>> @@ -51,6 +51,9 @@ struct kvm_steal_time { >>>> __u32 pad[11]; >>>> }; >>>> >>>> +#define KVM_VCPU_NOT_PREEMPTED (0 << 0) >>>> +#define KVM_VCPU_PREEMPTED (1 << 0) >>> >>> Is it really helpful to have two flags? >>> >>> Just use KVM_VCPU_PREEMPTED and clear that one in record_steal_time() >> >> I think it is fine since there is a third flag introduced in patch >> 2/4, it is more clear currently. >> >> Regards, >> Wanpeng Li >> > > Having two flags representing the same thing is not clear to me.
I agree that KVM_VCPU_NOT_PREEMPTED is not particularly necessary, but it is not correct to clear KVM_VCPU_PREEMPTED; instead, the entire field must be cleared to zero.
Also, this patch is not justified very well by the commit message. A better wording would be:
The next patch will add another bit to the preempted field in kvm_steal_time. Define a constant for bit 0 (the only one that is currently used).
| |