MIME-Version: 1.0
References: <20211122095619.000060d2@gmail.com> <YZvrvmRnuDc1e+gi@google.com>
In-Reply-To: <YZvrvmRnuDc1e+gi@google.com>
From:   Wanpeng Li <kernellwp@gmail.com>
Date:   Tue, 23 Nov 2021 10:57:38 +0800
Message-ID: <CANRm+Cx+bC8D7s1qzJYbrT+1rm46wxg6bAXD+kGYAHGnruZMXw@mail.gmail.com>
Subject: Re: [PATCH] KVM: LAPIC: Per vCPU control over kvm_can_post_timer_interrupt
To:     Sean Christopherson <seanjc@google.com>
Cc:     Aili Yao <yaoaili126@gmail.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        "the arch/x86 maintainers" <x86@kernel.org>,
        "H. Peter Anvin" <hpa@zytor.com>, kvm <kvm@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>, yaoaili@kingsoft.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, 23 Nov 2021 at 03:14, Sean Christopherson <seanjc@google.com> wrote:
>
> On Mon, Nov 22, 2021, Aili Yao wrote:
> > From: Aili Yao <yaoaili@kingsoft.com>
> >
> > When we isolate some pyhiscal cores, We may not use them for kvm guests,
> > We may use them for other purposes like DPDK, or we can make some kvm
> > guests isolated and some not, the global judgement pi_inject_timer is
> > not enough; We may make wrong decisions:
> >
> > In such a scenario, the guests without isolated cores will not be
> > permitted to use vmx preemption timer, and tscdeadline fastpath also be
> > disabled, both will lead to performance penalty.
> >
> > So check whether the vcpu->cpu is isolated, if not, don't post timer
> > interrupt.
> >
> > Signed-off-by: Aili Yao <yaoaili@kingsoft.com>
> > ---
> >  arch/x86/kvm/lapic.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index 759952dd1222..72dde5532101 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -34,6 +34,7 @@
> >  #include <asm/delay.h>
> >  #include <linux/atomic.h>
> >  #include <linux/jump_label.h>
> > +#include <linux/sched/isolation.h>
> >  #include "kvm_cache_regs.h"
> >  #include "irq.h"
> >  #include "ioapic.h"
> > @@ -113,7 +114,8 @@ static inline u32 kvm_x2apic_id(struct kvm_lapic *apic)
> >
> >  static bool kvm_can_post_timer_interrupt(struct kvm_vcpu *vcpu)
> >  {
> > -     return pi_inject_timer && kvm_vcpu_apicv_active(vcpu);
> > +     return pi_inject_timer && kvm_vcpu_apicv_active(vcpu) &&
> > +             !housekeeping_cpu(vcpu->cpu, HK_FLAG_TIMER);
>
> I don't think this is safe, vcpu->cpu will be -1 if the vCPU isn't scheduled in.
> This also doesn't play nice with the admin forcing pi_inject_timer=1.  Not saying
> there's a reasonable use case for doing that, but it's supported today and this
> would break that behavior.  It would also lead to weird behavior if a vCPU were
> migrated on/off a housekeeping vCPU.  Again, probably not a reasonable use case,
> but I don't see anything that would outright prevent that behavior.
>
> The existing behavior also feels a bit unsafe as pi_inject_timer is writable while
> KVM is running, though I supposed that's orthogonal to this discussion.
>
> Rather than check vcpu->cpu, is there an existing vCPU flag that can be queried,
> e.g. KVM_HINTS_REALTIME?

How about something like below:

From 67f605120e212384cb3d5788ba8c83f15659503b Mon Sep 17 00:00:00 2001
From: Wanpeng Li <wanpengli@tencent.com>
Date: Tue, 23 Nov 2021 10:36:10 +0800
Subject: [PATCH] KVM: LAPIC: To keep the vCPUs in non-root mode for timer-pi

From: Wanpeng Li <wanpengli@tencent.com>

As commit 0c5f81dad46 (KVM: LAPIC: Inject timer interrupt via posted interrupt)
mentioned that the host admin should well tune the guest setup, so that vCPUs
are placed on isolated pCPUs, and with several pCPUs surplus for
*busy* housekeeping.
It is better to disable mwait/hlt/pause vmexits to keep the vCPUs in non-root
mode. However, we may isolate pCPUs for other purpose like DPDK or we can make
some guests isolated and others not, Let's add the checking kvm_mwait_in_guest()
to kvm_can_post_timer_interrupt() since we can't benefit from timer
posted-interrupt
w/o keeping the vCPUs in non-root mode.

Reported-by: Aili Yao <yaoaili@kingsoft.com>
Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
 arch/x86/kvm/lapic.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 759952dd1222..8257566d44c7 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -113,14 +113,13 @@ static inline u32 kvm_x2apic_id(struct kvm_lapic *apic)

 static bool kvm_can_post_timer_interrupt(struct kvm_vcpu *vcpu)
 {
-    return pi_inject_timer && kvm_vcpu_apicv_active(vcpu);
+    return pi_inject_timer && kvm_mwait_in_guest(vcpu->kvm) &&
kvm_vcpu_apicv_active(vcpu);
 }

 bool kvm_can_use_hv_timer(struct kvm_vcpu *vcpu)
 {
     return kvm_x86_ops.set_hv_timer
-           && !(kvm_mwait_in_guest(vcpu->kvm) ||
-            kvm_can_post_timer_interrupt(vcpu));
+           && !kvm_mwait_in_guest(vcpu->kvm);
 }
 EXPORT_SYMBOL_GPL(kvm_can_use_hv_timer);