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), and we don't have someone like Sean or other kernel worlds to
eliminate all inline in the kernel world.
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.
