Re: [PATCH v3 1/5] KVM: x86/pmu: Prevent the PMU from counting disallowed events

[Sean Christopherson <seanjc@google.com>]

On Wed, Mar 08, 2023, Like Xu wrote:
> On 8/3/2023 12:01 am, Sean Christopherson wrote:
> > On Tue, Mar 07, 2023, Aaron Lewis wrote:
> > > On Tue, Mar 7, 2023 at 7:19 AM Like Xu <like.xu.linux@gmail.com> wrote:
> > > > > ---
> > > > >    arch/x86/kvm/pmu.c | 13 ++++++++-----
> > > > >    1 file changed, 8 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> > > > > index 612e6c70ce2e..9914a9027c60 100644
> > > > > --- a/arch/x86/kvm/pmu.c
> > > > > +++ b/arch/x86/kvm/pmu.c
> > > > > @@ -400,6 +400,12 @@ static bool check_pmu_event_filter(struct kvm_pmc *pmc)
> > > > >        return is_fixed_event_allowed(filter, pmc->idx);
> > > > >    }
> > > > > 
> > > > > +static bool event_is_allowed(struct kvm_pmc *pmc)
> > > > 
> > > > Nit, an inline event_is_allowed() here might be better.
> > > 
> > > I purposely didn't inline this because Sean generally discourages its
> > > use and has commented in several reviews to not use 'inline' and
> > > instead leave it up to the compiler to decide, unless using
> > > __always_inline.
> > 
> > Ya.
> 
> I think we all respect mainatiner's personal preferences for sure. However,
> I'm not sure how to define Sean's "generally discourage", nor does my
> binary bi-directional verifier-bot (losing control of these details at the code
> level can be frustrating, especially for people who care about performance
> gains but can't use the fresh new tool chain for some supply chain policy
> reasons),

I'm not buying that argument.  Modern compilers are much smarter than humans when
it comes to performance optimizations and will do the right thing 99% of the time.
There are exceptions, e.g. coercing the compiler into generating arithmetic instead
of conditional branches, but those are few and far between.

If you care about performance to the point where a CALL+RET (which is not at all
expensive on modern CPUs) and _maybe_ a few arithmetic ops are concerning, _and_
your toolchain is so awful that I can't do a good job of optimizing straightforward
code like this, then you have much bigger problems.

If someone can provide data to show that forcing a particularly function to be
inlined meaningful changes runtime performance, then I'll happily take a patch.

> and we don't have someone like Sean or other kernel worlds to eliminate all
> inline in the kernel world.

Huh?  I'm not saying "inline is bad", I'm saying modern compilers are plenty smart
enough to inline (or not) when appropriate in the overwhelming majority of cases,
and that outside of select edge cases and truly performance critical paths, the
days when humans can handcode better code than the compiler are long gone.  For
functions that should result in literally one or two instructions, I'm fine with
tagging them inline even though I think it's unnecessary.  But this proposed
helper is not that case.

> > > I think the sentiment is either use the strong hint or don't use it at all.
> > > This seems like an example of where the compiler can decide, and a strong
> > > hint isn't needed.
> > 
> > Not quite.  __always_inline is not a hint, it's a command.  The kernel *requires*
> > functions tagged with __always_inline to be (surprise!) always inlined, even when
> > building with features that cause the compiler to generate non-inlined functions
> > for even the most trivial helpers, e.g. KASAN can cause a literal nop function to
> > be non-inlined.  __alway_inlined is used to ensure like no-instrumentation regions
> > and __init sections are preserved when invoking common helpers.
> 
> So, do you think "__always_inline event_is_allowed()" in the highly recurring
> path reprogram_counter() is a better move ? I'd say yes, and am not willing
> to risk paying for a function call overhead since any advancement in this
> direction is encouraged.

Absolutely not.  __always_inline is for situations where the code _must_ be inlined,
or as above, where someone can prove with data that (a) modern compilers aren't
smart enough to do the right thing and (b) that inlining provides meaningful
performance benefits.