| Message ID | 20230215121231.43436-1-lirongqing@baidu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/kvm: refine condition for checking vCPU preempted | expand |
+Paolo, Wanpeng, and Vitaly In the future, use get_maintainers.pl to build To: and Cc: so that the right folks see the patch. Not everyone habitually scours the KVM list. :-) On Wed, Feb 15, 2023, lirongqing@baidu.com wrote: > From: Li RongQing <lirongqing@baidu.com> > > Check whether vcpu is preempted or not when HLT is trapped or there > is not realtime hint. Please explain _why_ there's no need to check for preemption in this setup. What may be obvious to you isn't necessarily obvious to reviewers or readers. > In other words, it is unnecessary to check preemption if HLT is not > intercepted and guest has realtime hint > > Signed-off-by: Li RongQing <lirongqing@baidu.com> > --- > arch/x86/kernel/kvm.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c > index 1cceac5..1a2744d 100644 > --- a/arch/x86/kernel/kvm.c > +++ b/arch/x86/kernel/kvm.c > @@ -820,8 +820,10 @@ static void __init kvm_guest_init(void) > has_steal_clock = 1; > static_call_update(pv_steal_clock, kvm_steal_clock); > > - pv_ops.lock.vcpu_is_preempted = > - PV_CALLEE_SAVE(__kvm_vcpu_is_preempted); > + if (kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) || Rather than have the guest rely on host KVM behavior clearing PV_UNHALT when HLT is passed through), would it make sense to add something like KVM_HINTS_HLT_PASSTHROUGH to more explicitly tell the guest that HLT isn't intercepted? > + !kvm_para_has_hint(KVM_HINTS_REALTIME)) Misaligned indentation (one too many spaces). > + pv_ops.lock.vcpu_is_preempted = > + PV_CALLEE_SAVE(__kvm_vcpu_is_preempted); > } > > if (kvm_para_has_feature(KVM_FEATURE_PV_EOI)) > -- > 2.9.4 >
> -----Original Message----- > From: Sean Christopherson <seanjc@google.com> > Sent: Wednesday, March 15, 2023 8:16 AM > To: Li,Rongqing <lirongqing@baidu.com> > Cc: kvm@vger.kernel.org; x86@kernel.org; Paolo Bonzini > <pbonzini@redhat.com>; Wanpeng Li <wanpengli@tencent.com>; Vitaly > Kuznetsov <vkuznets@redhat.com> > Subject: Re: [PATCH] x86/kvm: refine condition for checking vCPU preempted > > +Paolo, Wanpeng, and Vitaly > > In the future, use get_maintainers.pl to build To: and Cc: so that the right folks > see the patch. Not everyone habitually scours the KVM list. :-) > Ok > On Wed, Feb 15, 2023, lirongqing@baidu.com wrote: > > From: Li RongQing <lirongqing@baidu.com> > > > > Check whether vcpu is preempted or not when HLT is trapped or there is > > not realtime hint. > > Please explain _why_ there's no need to check for preemption in this setup. > What may be obvious to you isn't necessarily obvious to reviewers or readers. > I will rewrite the changelog > > In other words, it is unnecessary to check preemption if HLT is not > > intercepted and guest has realtime hint > > > > Signed-off-by: Li RongQing <lirongqing@baidu.com> > > --- > > arch/x86/kernel/kvm.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c index > > 1cceac5..1a2744d 100644 > > --- a/arch/x86/kernel/kvm.c > > +++ b/arch/x86/kernel/kvm.c > > @@ -820,8 +820,10 @@ static void __init kvm_guest_init(void) > > has_steal_clock = 1; > > static_call_update(pv_steal_clock, kvm_steal_clock); > > > > - pv_ops.lock.vcpu_is_preempted = > > - PV_CALLEE_SAVE(__kvm_vcpu_is_preempted); > > + if (kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) || > > Rather than have the guest rely on host KVM behavior clearing PV_UNHALT > when HLT is passed through), would it make sense to add something like > KVM_HINTS_HLT_PASSTHROUGH to more explicitly tell the guest that HLT isn't > intercepted? KVM_HINTS_HLT_PASSTHROUGH is more obvious, but it need both kvm and guest support Thanks -Li > > > + !kvm_para_has_hint(KVM_HINTS_REALTIME)) > > Misaligned indentation (one too many spaces). > > > + pv_ops.lock.vcpu_is_preempted = > > + PV_CALLEE_SAVE(__kvm_vcpu_is_preempted); > > } > > > > if (kvm_para_has_feature(KVM_FEATURE_PV_EOI)) > > -- > > 2.9.4 > >
On Wed, Mar 15, 2023, Li,Rongqing wrote: > > Rather than have the guest rely on host KVM behavior clearing PV_UNHALT > > when HLT is passed through), would it make sense to add something like > > KVM_HINTS_HLT_PASSTHROUGH to more explicitly tell the guest that HLT isn't > > intercepted? > > KVM_HINTS_HLT_PASSTHROUGH is more obvious, but it need both kvm and guest support Yeah, that's the downside. But modifying KVM and/or the userspace VMM shouldn't be difficult, i.e the only meaningful cost is the rollout of a new kernel/VMM. On the other hand, establishing the heuristic that !PV_UNHALT == HLT_PASSTHROUGH could have to subtle issues in the future. It safe-ish in the context of this patch as userspace is unlikely to set KVM_HINTS_REALTIME, hide PV_UNHALT, and _not_ passthrough HLT. But without the REALTIME side of things, !PV_UNHALT == HLT_PASSTHROUGH is much less likely to hold true.
> -----Original Message----- > From: Sean Christopherson <seanjc@google.com> > Sent: Wednesday, March 15, 2023 11:27 PM > To: Li,Rongqing <lirongqing@baidu.com> > Cc: kvm@vger.kernel.org; x86@kernel.org; Paolo Bonzini > <pbonzini@redhat.com>; Wanpeng Li <wanpengli@tencent.com>; Vitaly > Kuznetsov <vkuznets@redhat.com> > Subject: Re: [PATCH] x86/kvm: refine condition for checking vCPU preempted > > On Wed, Mar 15, 2023, Li,Rongqing wrote: > > > Rather than have the guest rely on host KVM behavior clearing > > > PV_UNHALT when HLT is passed through), would it make sense to add > > > something like KVM_HINTS_HLT_PASSTHROUGH to more explicitly tell the > > > guest that HLT isn't intercepted? > > > > KVM_HINTS_HLT_PASSTHROUGH is more obvious, but it need both kvm and > > guest support > > Yeah, that's the downside. But modifying KVM and/or the userspace VMM > shouldn't be difficult, i.e the only meaningful cost is the rollout of a new > kernel/VMM. > > On the other hand, establishing the heuristic that !PV_UNHALT == > HLT_PASSTHROUGH could have to subtle issues in the future. It safe-ish in the > context of this patch as userspace is unlikely to set KVM_HINTS_REALTIME, hide > PV_UNHALT, and _not_ passthrough HLT. But without the REALTIME side of > things, !PV_UNHALT == HLT_PASSTHROUGH is much less likely to hold true. Ok, could you submit these codes Thanks -Li
On Thu, Mar 16, 2023, Li,Rongqing wrote: > > From: Sean Christopherson <seanjc@google.com> > > On Wed, Mar 15, 2023, Li,Rongqing wrote: > > > > Rather than have the guest rely on host KVM behavior clearing > > > > PV_UNHALT when HLT is passed through), would it make sense to add > > > > something like KVM_HINTS_HLT_PASSTHROUGH to more explicitly tell the > > > > guest that HLT isn't intercepted? > > > > > > KVM_HINTS_HLT_PASSTHROUGH is more obvious, but it need both kvm and > > > guest support > > > > Yeah, that's the downside. But modifying KVM and/or the userspace VMM > > shouldn't be difficult, i.e the only meaningful cost is the rollout of a new > > kernel/VMM. > > > > On the other hand, establishing the heuristic that !PV_UNHALT == > > HLT_PASSTHROUGH could have to subtle issues in the future. It safe-ish in the > > context of this patch as userspace is unlikely to set KVM_HINTS_REALTIME, hide > > PV_UNHALT, and _not_ passthrough HLT. But without the REALTIME side of > > things, !PV_UNHALT == HLT_PASSTHROUGH is much less likely to hold true. > > Ok, could you submit these codes I'd like to hear from others first, especially Paolo and/or Wanpeng.
> -----Original Message----- > From: Sean Christopherson <seanjc@google.com> > Sent: Friday, March 17, 2023 1:01 AM > To: Li,Rongqing <lirongqing@baidu.com> > Cc: kvm@vger.kernel.org; x86@kernel.org; Paolo Bonzini > <pbonzini@redhat.com>; Wanpeng Li <wanpengli@tencent.com>; Vitaly > Kuznetsov <vkuznets@redhat.com> > Subject: Re: [PATCH] x86/kvm: refine condition for checking vCPU preempted > > On Thu, Mar 16, 2023, Li,Rongqing wrote: > > > From: Sean Christopherson <seanjc@google.com> On Wed, Mar 15, 2023, > > > Li,Rongqing wrote: > > > > > Rather than have the guest rely on host KVM behavior clearing > > > > > PV_UNHALT when HLT is passed through), would it make sense to > > > > > add something like KVM_HINTS_HLT_PASSTHROUGH to more explicitly > > > > > tell the guest that HLT isn't intercepted? > > > > > > > > KVM_HINTS_HLT_PASSTHROUGH is more obvious, but it need both kvm > > > > and guest support > > > > > > Yeah, that's the downside. But modifying KVM and/or the userspace > > > VMM shouldn't be difficult, i.e the only meaningful cost is the > > > rollout of a new kernel/VMM. > > > > > > On the other hand, establishing the heuristic that !PV_UNHALT == > > > HLT_PASSTHROUGH could have to subtle issues in the future. It > > > safe-ish in the context of this patch as userspace is unlikely to > > > set KVM_HINTS_REALTIME, hide PV_UNHALT, and _not_ passthrough HLT. > > > But without the REALTIME side of things, !PV_UNHALT == > HLT_PASSTHROUGH is much less likely to hold true. > > > > Ok, could you submit these codes > > I'd like to hear from others first, especially Paolo and/or Wanpeng. I see no progress How about to adopt this patch at first, it can give small performance for existing KVM and setup Then you continue to modify the kernel/VMM to give better support for KVM/guest -Li
On Thu, Mar 30, 2023, Li,Rongqing wrote: > > From: Sean Christopherson <seanjc@google.com> > > On Thu, Mar 16, 2023, Li,Rongqing wrote: > > > > From: Sean Christopherson <seanjc@google.com> On Wed, Mar 15, 2023, > > > > Li,Rongqing wrote: > > > > > > Rather than have the guest rely on host KVM behavior clearing > > > > > > PV_UNHALT when HLT is passed through), would it make sense to > > > > > > add something like KVM_HINTS_HLT_PASSTHROUGH to more explicitly > > > > > > tell the guest that HLT isn't intercepted? > > > > > > > > > > KVM_HINTS_HLT_PASSTHROUGH is more obvious, but it need both kvm > > > > > and guest support > > > > > > > > Yeah, that's the downside. But modifying KVM and/or the userspace > > > > VMM shouldn't be difficult, i.e the only meaningful cost is the > > > > rollout of a new kernel/VMM. > > > > > > > > On the other hand, establishing the heuristic that !PV_UNHALT == > > > > HLT_PASSTHROUGH could have to subtle issues in the future. It > > > > safe-ish in the context of this patch as userspace is unlikely to > > > > set KVM_HINTS_REALTIME, hide PV_UNHALT, and _not_ passthrough HLT. > > > > But without the REALTIME side of things, !PV_UNHALT == > > HLT_PASSTHROUGH is much less likely to hold true. > > > > > > Ok, could you submit these codes > > > > I'd like to hear from others first, especially Paolo and/or Wanpeng. > > I see no progress > How about to adopt this patch at first, it can give small performance for existing KVM and setup > Then you continue to modify the kernel/VMM to give better support for KVM/guest Guest-side KVM stuff is not my maintenance domain, i.e. this needs Paolo's attention no matter what.
> -----Original Message----- > From: Sean Christopherson <seanjc@google.com> > Sent: Friday, March 31, 2023 3:26 AM > To: Li,Rongqing <lirongqing@baidu.com> > Cc: kvm@vger.kernel.org; x86@kernel.org; Paolo Bonzini > <pbonzini@redhat.com>; Wanpeng Li <wanpengli@tencent.com>; Vitaly > Kuznetsov <vkuznets@redhat.com> > Subject: Re: [PATCH] x86/kvm: refine condition for checking vCPU preempted > > Guest-side KVM stuff is not my maintenance domain, i.e. this needs Paolo's > attention no matter what. Ok, I will rewrite the changelog and modify the code indentation as your suggestion, and resend this patch Thanks -Li
diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c index 1cceac5..1a2744d 100644 --- a/arch/x86/kernel/kvm.c +++ b/arch/x86/kernel/kvm.c @@ -820,8 +820,10 @@ static void __init kvm_guest_init(void) has_steal_clock = 1; static_call_update(pv_steal_clock, kvm_steal_clock); - pv_ops.lock.vcpu_is_preempted = - PV_CALLEE_SAVE(__kvm_vcpu_is_preempted); + if (kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) || + !kvm_para_has_hint(KVM_HINTS_REALTIME)) + pv_ops.lock.vcpu_is_preempted = + PV_CALLEE_SAVE(__kvm_vcpu_is_preempted); } if (kvm_para_has_feature(KVM_FEATURE_PV_EOI))