Date:   Tue, 23 Nov 2021 16:18:34 +0800
From:   Aili Yao <yaoaili126@gmail.com>
To:     Sean Christopherson <seanjc@google.com>
Cc:     pbonzini@redhat.com, vkuznets@redhat.com, wanpengli@tencent.com,
        jmattson@google.com, joro@8bytes.org, tglx@linutronix.de,
        mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com,
        x86@kernel.org, hpa@zytor.com, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, yaoaili@kingsoft.com
Subject: Re: [PATCH] KVM: LAPIC: Per vCPU control over
 kvm_can_post_timer_interrupt
Message-ID: <20211123161834.30714698@gmail.com>
In-Reply-To: <YZvrvmRnuDc1e+gi@google.com>
References: <20211122095619.000060d2@gmail.com>
        <YZvrvmRnuDc1e+gi@google.com>
Organization: ksyun
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Mon, 22 Nov 2021 19:13:02 +0000
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. 

I checked this, It seems we will set vcpu->cpu to a valid value when we
create vcpu( kvm_vm_ioctl_create_vcpu()), only after that we can
configure lapic through vcpu fd and start the timer, this may not be one
real problem.

Currently, the patch seems work as expected in my test, maybe one
possible candidate for the issue listed above.

Thanks

> 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?
> 
> >  }
> >  
> >  bool kvm_can_use_hv_timer(struct kvm_vcpu *vcpu)
> > -- 
> > 2.25.1
> >