| Message ID | 20230307141400.1486314-2-aaronlewis@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix "Instructions Retired" from incorrectly counting | expand |
On 2023/3/7 22:13, Aaron Lewis wrote: > When counting "Instructions Retired" (0xc0) in a guest, KVM will > occasionally increment the PMU counter regardless of if that event is > being filtered. This is because some PMU events are incremented via > kvm_pmu_trigger_event(), which doesn't know about the event filter. Add > the event filter to kvm_pmu_trigger_event(), so events that are > disallowed do not increment their counters. It would be nice to have: Reported-by: Jinrong Liang <cloudliang@tencent.com> , since he also found the same issue. > Fixes: 9cd803d496e7 ("KVM: x86: Update vPMCs when retiring instructions") > Signed-off-by: Aaron Lewis <aaronlewis@google.com> Reviewed-by: Like Xu <likexu@tencent.com> > --- > 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. > +{ > + return pmc_is_enabled(pmc) && pmc_speculative_in_use(pmc) && > + check_pmu_event_filter(pmc); > +} > + > static void reprogram_counter(struct kvm_pmc *pmc) > { > struct kvm_pmu *pmu = pmc_to_pmu(pmc); > @@ -409,10 +415,7 @@ static void reprogram_counter(struct kvm_pmc *pmc) > > pmc_pause_counter(pmc); > > - if (!pmc_speculative_in_use(pmc) || !pmc_is_enabled(pmc)) > - goto reprogram_complete; > - > - if (!check_pmu_event_filter(pmc)) > + if (!event_is_allowed(pmc)) > goto reprogram_complete; > > if (pmc->counter < pmc->prev_counter) > @@ -684,7 +687,7 @@ void kvm_pmu_trigger_event(struct kvm_vcpu *vcpu, u64 perf_hw_id) > for_each_set_bit(i, pmu->all_valid_pmc_idx, X86_PMC_IDX_MAX) { > pmc = static_call(kvm_x86_pmu_pmc_idx_to_pmc)(pmu, i); > > - if (!pmc || !pmc_is_enabled(pmc) || !pmc_speculative_in_use(pmc)) > + if (!pmc || !event_is_allowed(pmc)) > continue; > > /* Ignore checks for edge detect, pin control, invert and CMASK bits */
On Tue, Mar 7, 2023 at 7:19 AM Like Xu <like.xu.linux@gmail.com> wrote: > > On 2023/3/7 22:13, Aaron Lewis wrote: > > > When counting "Instructions Retired" (0xc0) in a guest, KVM will > > occasionally increment the PMU counter regardless of if that event is > > being filtered. This is because some PMU events are incremented via > > kvm_pmu_trigger_event(), which doesn't know about the event filter. Add > > the event filter to kvm_pmu_trigger_event(), so events that are > > disallowed do not increment their counters. > It would be nice to have: > > Reported-by: Jinrong Liang <cloudliang@tencent.com> > > , since he also found the same issue. > Sure, I can add that. > > Fixes: 9cd803d496e7 ("KVM: x86: Update vPMCs when retiring instructions") > > Signed-off-by: Aaron Lewis <aaronlewis@google.com> > > Reviewed-by: Like Xu <likexu@tencent.com> > > > --- > > 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. 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. > > > +{ > > + return pmc_is_enabled(pmc) && pmc_speculative_in_use(pmc) && > > + check_pmu_event_filter(pmc); > > +} > > + > > static void reprogram_counter(struct kvm_pmc *pmc) > > { > > struct kvm_pmu *pmu = pmc_to_pmu(pmc); > > @@ -409,10 +415,7 @@ static void reprogram_counter(struct kvm_pmc *pmc) > > > > pmc_pause_counter(pmc); > > > > - if (!pmc_speculative_in_use(pmc) || !pmc_is_enabled(pmc)) > > - goto reprogram_complete; > > - > > - if (!check_pmu_event_filter(pmc)) > > + if (!event_is_allowed(pmc)) > > goto reprogram_complete; > > > > if (pmc->counter < pmc->prev_counter) > > @@ -684,7 +687,7 @@ void kvm_pmu_trigger_event(struct kvm_vcpu *vcpu, u64 perf_hw_id) > > for_each_set_bit(i, pmu->all_valid_pmc_idx, X86_PMC_IDX_MAX) { > > pmc = static_call(kvm_x86_pmu_pmc_idx_to_pmc)(pmu, i); > > > > - if (!pmc || !pmc_is_enabled(pmc) || !pmc_speculative_in_use(pmc)) > > + if (!pmc || !event_is_allowed(pmc)) > > continue; > > > > /* Ignore checks for edge detect, pin control, invert and CMASK bits */
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 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.
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), 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.
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.
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) +{ + return pmc_is_enabled(pmc) && pmc_speculative_in_use(pmc) && + check_pmu_event_filter(pmc); +} + static void reprogram_counter(struct kvm_pmc *pmc) { struct kvm_pmu *pmu = pmc_to_pmu(pmc); @@ -409,10 +415,7 @@ static void reprogram_counter(struct kvm_pmc *pmc) pmc_pause_counter(pmc); - if (!pmc_speculative_in_use(pmc) || !pmc_is_enabled(pmc)) - goto reprogram_complete; - - if (!check_pmu_event_filter(pmc)) + if (!event_is_allowed(pmc)) goto reprogram_complete; if (pmc->counter < pmc->prev_counter) @@ -684,7 +687,7 @@ void kvm_pmu_trigger_event(struct kvm_vcpu *vcpu, u64 perf_hw_id) for_each_set_bit(i, pmu->all_valid_pmc_idx, X86_PMC_IDX_MAX) { pmc = static_call(kvm_x86_pmu_pmc_idx_to_pmc)(pmu, i); - if (!pmc || !pmc_is_enabled(pmc) || !pmc_speculative_in_use(pmc)) + if (!pmc || !event_is_allowed(pmc)) continue; /* Ignore checks for edge detect, pin control, invert and CMASK bits */
When counting "Instructions Retired" (0xc0) in a guest, KVM will occasionally increment the PMU counter regardless of if that event is being filtered. This is because some PMU events are incremented via kvm_pmu_trigger_event(), which doesn't know about the event filter. Add the event filter to kvm_pmu_trigger_event(), so events that are disallowed do not increment their counters. Fixes: 9cd803d496e7 ("KVM: x86: Update vPMCs when retiring instructions") Signed-off-by: Aaron Lewis <aaronlewis@google.com> --- arch/x86/kvm/pmu.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)