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@xxxxxxxxx> 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.
