Re: [Patch v4 07/13] perf/x86: Add constraint for guest perf metrics event

[Peter Zijlstra <peterz@infradead.org>]

On Tue, Oct 03, 2023 at 08:23:26AM -0700, Sean Christopherson wrote:
> On Tue, Oct 03, 2023, Peter Zijlstra wrote:
> > On Mon, Oct 02, 2023 at 05:56:28PM -0700, Sean Christopherson wrote:

> > > Well drat, that there would have saved a wee bit of frustration.  Better late
> > > than never though, that's for sure.
> > > 
> > > Just to double confirm: keeping guest PMU state loaded until the vCPU is scheduled
> > > out or KVM exits to userspace, would mean that host perf events won't be active
> > > for potentially large swaths of non-KVM code.  Any function calls or event/exception
> > > handlers that occur within the context of ioctl(KVM_RUN) would run with host
> > > perf events disabled.
> > 
> > Hurmph, that sounds sub-optimal, earlier you said <1500 cycles, this all
> > sounds like a ton more.
> > 
> > /me frobs around the kvm code some...
> > 
> > Are we talking about exit_fastpath loop in vcpu_enter_guest() ? That
> > seems to run with IRQs disabled, so at most you can trigger a #PF or
> > something, which will then trip an exception fixup because you can't run
> > #PF with IRQs disabled etc..
> >
> > That seems fine. That is, a theoretical kvm_x86_handle_enter_irqoff()
> > coupled with the existing kvm_x86_handle_exit_irqoff() seems like
> > reasonable solution from where I'm sitting. That also more or less
> > matches the FPU state save/restore AFAICT.
> > 
> > Or are you talking about the whole of vcpu_run() ? That seems like a
> > massive amount of code, and doesn't look like anything I'd call a
> > fast-path. Also, much of that loop has preemption enabled...
> 
> The whole of vcpu_run().  And yes, much of it runs with preemption enabled.  KVM
> uses preempt notifiers to context switch state if the vCPU task is scheduled
> out/in, we'd use those hooks to swap PMU state.
> 
> Jumping back to the exception analogy, not all exits are equal.  For "simple" exits
> that KVM can handle internally, the roundtrip is <1500.   The exit_fastpath loop is
> roughly half that.
> 
> But for exits that are more complex, e.g. if the guest hits the equivalent of a
> page fault, the cost of handling the page fault can vary significantly.  It might
> be <1500, but it might also be 10x that if handling the page fault requires faulting
> in a new page in the host.
> 
> We don't want to get too aggressive with moving stuff into the exit_fastpath loop,
> because doing too much work with IRQs disabled can cause latency problems for the
> host.  This isn't much of a concern for slice-of-hardware setups, but would be
> quite problematic for other use cases.
> 
> And except for obviously slow paths (from the guest's perspective), extra latency
> on any exit can be problematic.  E.g. even if we got to the point where KVM handles
> 99% of exits the fastpath (may or may not be feasible), a not-fastpath exit at an
> inopportune time could throw off the guest's profiling results, introduce unacceptable
> jitter, etc.

I'm confused... the PMU must not be running after vm-exit. It must not
be able to profile the host. So what jitter are you talking about?

Even if we persist the MSR contents, the PMU itself must be disabled on
vm-exit and enabled on vm-enter. If not by hardware then by software
poking at the global ctrl msr.

I also don't buy the latency argument, we already do full and complete
PMU rewrites with IRQs disabled in the context switch path. And as
mentioned elsewhere, the whole AMX thing has an 8k copy stuck in the FPU
save/restore.

I would much prefer we keep the PMU swizzle inside the IRQ disabled
region of vcpu_enter_guest(). That's already a ton better than you have
today.