[PATCH] x86/pmu: Disable inlining of measure()

[PATCH] x86/pmu: Disable inlining of measure()

[Bill Wendling]

From: Bill Wendling <isanbard@gmail.com>

Clang can be more aggressive at inlining than GCC and will fully inline
calls to measure(). This can mess with the counter overflow check. To
set up the PMC overflow, check_counter_overflow() first records the
number of instructions retired in an invocation of measure() and checks
to see that subsequent calls to measure() retire the same number of
instructions. If inlining occurs, those numbers can be different and the
overflow test fails.

  FAIL: overflow: cntr-0
  PASS: overflow: status-0
  PASS: overflow: status clear-0
  PASS: overflow: irq-0
  FAIL: overflow: cntr-1
  PASS: overflow: status-1
  PASS: overflow: status clear-1
  PASS: overflow: irq-1
  FAIL: overflow: cntr-2
  PASS: overflow: status-2
  PASS: overflow: status clear-2
  PASS: overflow: irq-2
  FAIL: overflow: cntr-3
  PASS: overflow: status-3
  PASS: overflow: status clear-3
  PASS: overflow: irq-3

Disabling inlining of measure() keeps the assumption that all calls to
measure() retire the same number of instructions.

Cc: Jim Mattson <jmattson@google.com>
Signed-off-by: Bill Wendling <morbo@google.com>
---
 x86/pmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/x86/pmu.c b/x86/pmu.c
index a46bdbf4788c..bbfd268aafa4 100644
--- a/x86/pmu.c
+++ b/x86/pmu.c
@@ -211,7 +211,7 @@ static void stop_event(pmu_counter_t *evt)
 	evt->count = rdmsr(evt->ctr);
 }
 
-static void measure(pmu_counter_t *evt, int count)
+static noinline void measure(pmu_counter_t *evt, int count)
 {
 	int i;
 	for (i = 0; i < count; i++)
-- 
2.36.1.255.ge46751e96f-goog

Re: [PATCH] x86/pmu: Disable inlining of measure()

[Jim Mattson]

On Wed, Jun 1, 2022 at 9:30 AM Bill Wendling <morbo@google.com> wrote:
>
> From: Bill Wendling <isanbard@gmail.com>
>
> Clang can be more aggressive at inlining than GCC and will fully inline
> calls to measure(). This can mess with the counter overflow check. To
> set up the PMC overflow, check_counter_overflow() first records the
> number of instructions retired in an invocation of measure() and checks
> to see that subsequent calls to measure() retire the same number of
> instructions. If inlining occurs, those numbers can be different and the
> overflow test fails.
>
>   FAIL: overflow: cntr-0
>   PASS: overflow: status-0
>   PASS: overflow: status clear-0
>   PASS: overflow: irq-0
>   FAIL: overflow: cntr-1
>   PASS: overflow: status-1
>   PASS: overflow: status clear-1
>   PASS: overflow: irq-1
>   FAIL: overflow: cntr-2
>   PASS: overflow: status-2
>   PASS: overflow: status clear-2
>   PASS: overflow: irq-2
>   FAIL: overflow: cntr-3
>   PASS: overflow: status-3
>   PASS: overflow: status clear-3
>   PASS: overflow: irq-3
>
> Disabling inlining of measure() keeps the assumption that all calls to
> measure() retire the same number of instructions.
>
> Cc: Jim Mattson <jmattson@google.com>
> Signed-off-by: Bill Wendling <morbo@google.com>
Reviewed-by: Jim Mattson <jmattson@google.com>

Re: [PATCH] x86/pmu: Disable inlining of measure()

[Bill Wendling]

On Wed, Jun 1, 2022 at 10:22 AM Jim Mattson <jmattson@google.com> wrote:
>
> On Wed, Jun 1, 2022 at 9:30 AM Bill Wendling <morbo@google.com> wrote:
> >
> > From: Bill Wendling <isanbard@gmail.com>
> >
> > Clang can be more aggressive at inlining than GCC and will fully inline
> > calls to measure(). This can mess with the counter overflow check. To
> > set up the PMC overflow, check_counter_overflow() first records the
> > number of instructions retired in an invocation of measure() and checks
> > to see that subsequent calls to measure() retire the same number of
> > instructions. If inlining occurs, those numbers can be different and the
> > overflow test fails.
> >
> >   FAIL: overflow: cntr-0
> >   PASS: overflow: status-0
> >   PASS: overflow: status clear-0
> >   PASS: overflow: irq-0
> >   FAIL: overflow: cntr-1
> >   PASS: overflow: status-1
> >   PASS: overflow: status clear-1
> >   PASS: overflow: irq-1
> >   FAIL: overflow: cntr-2
> >   PASS: overflow: status-2
> >   PASS: overflow: status clear-2
> >   PASS: overflow: irq-2
> >   FAIL: overflow: cntr-3
> >   PASS: overflow: status-3
> >   PASS: overflow: status clear-3
> >   PASS: overflow: irq-3
> >
> > Disabling inlining of measure() keeps the assumption that all calls to
> > measure() retire the same number of instructions.
> >
> > Cc: Jim Mattson <jmattson@google.com>
> > Signed-off-by: Bill Wendling <morbo@google.com>
> Reviewed-by: Jim Mattson <jmattson@google.com>

Bumping for visibility.

-bw

Re: [PATCH] x86/pmu: Disable inlining of measure()

[Sean Christopherson]

On Tue, Oct 25, 2022, Bill Wendling wrote:
> On Wed, Jun 1, 2022 at 10:22 AM Jim Mattson <jmattson@google.com> wrote:
> >
> > On Wed, Jun 1, 2022 at 9:30 AM Bill Wendling <morbo@google.com> wrote:
> > >
> > > From: Bill Wendling <isanbard@gmail.com>
> > >
> > > Clang can be more aggressive at inlining than GCC and will fully inline
> > > calls to measure(). This can mess with the counter overflow check. To
> > > set up the PMC overflow, check_counter_overflow() first records the
> > > number of instructions retired in an invocation of measure() and checks
> > > to see that subsequent calls to measure() retire the same number of
> > > instructions. If inlining occurs, those numbers can be different and the
> > > overflow test fails.
> > >
> > >   FAIL: overflow: cntr-0
> > >   PASS: overflow: status-0
> > >   PASS: overflow: status clear-0
> > >   PASS: overflow: irq-0
> > >   FAIL: overflow: cntr-1
> > >   PASS: overflow: status-1
> > >   PASS: overflow: status clear-1
> > >   PASS: overflow: irq-1
> > >   FAIL: overflow: cntr-2
> > >   PASS: overflow: status-2
> > >   PASS: overflow: status clear-2
> > >   PASS: overflow: irq-2
> > >   FAIL: overflow: cntr-3
> > >   PASS: overflow: status-3
> > >   PASS: overflow: status clear-3
> > >   PASS: overflow: irq-3
> > >
> > > Disabling inlining of measure() keeps the assumption that all calls to
> > > measure() retire the same number of instructions.
> > >
> > > Cc: Jim Mattson <jmattson@google.com>
> > > Signed-off-by: Bill Wendling <morbo@google.com>
> > Reviewed-by: Jim Mattson <jmattson@google.com>
> 
> Bumping for visibility.

Heh, make sure kvm-unit-tests is in the subject, i.e. [kvm-unit-tests PATCH].
This slipped by on my end because I didn't realize at a quick glance that it was
touching KVM-unit-tests and not kernel code.

[kvm-unit-tests PATCH] x86/pmu: Disable inlining of measure()

[Bill Wendling]

On Tue, Oct 25, 2022 at 4:12 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, Oct 25, 2022, Bill Wendling wrote:
> > On Wed, Jun 1, 2022 at 10:22 AM Jim Mattson <jmattson@google.com> wrote:
> > >
> > > On Wed, Jun 1, 2022 at 9:30 AM Bill Wendling <morbo@google.com> wrote:
> > > >
> > > > From: Bill Wendling <isanbard@gmail.com>
> > > >
> > > > Clang can be more aggressive at inlining than GCC and will fully inline
> > > > calls to measure(). This can mess with the counter overflow check. To
> > > > set up the PMC overflow, check_counter_overflow() first records the
> > > > number of instructions retired in an invocation of measure() and checks
> > > > to see that subsequent calls to measure() retire the same number of
> > > > instructions. If inlining occurs, those numbers can be different and the
> > > > overflow test fails.
> > > >
> > > >   FAIL: overflow: cntr-0
> > > >   PASS: overflow: status-0
> > > >   PASS: overflow: status clear-0
> > > >   PASS: overflow: irq-0
> > > >   FAIL: overflow: cntr-1
> > > >   PASS: overflow: status-1
> > > >   PASS: overflow: status clear-1
> > > >   PASS: overflow: irq-1
> > > >   FAIL: overflow: cntr-2
> > > >   PASS: overflow: status-2
> > > >   PASS: overflow: status clear-2
> > > >   PASS: overflow: irq-2
> > > >   FAIL: overflow: cntr-3
> > > >   PASS: overflow: status-3
> > > >   PASS: overflow: status clear-3
> > > >   PASS: overflow: irq-3
> > > >
> > > > Disabling inlining of measure() keeps the assumption that all calls to
> > > > measure() retire the same number of instructions.
> > > >
> > > > Cc: Jim Mattson <jmattson@google.com>
> > > > Signed-off-by: Bill Wendling <morbo@google.com>
> > > Reviewed-by: Jim Mattson <jmattson@google.com>
> >
> > Bumping for visibility.
>
> Heh, make sure kvm-unit-tests is in the subject, i.e. [kvm-unit-tests PATCH].
> This slipped by on my end because I didn't realize at a quick glance that it was
> touching KVM-unit-tests and not kernel code.

Doh! I'm not sure why I forgot that. Added. Thanks. :)

-bw

Re: [kvm-unit-tests PATCH] x86/pmu: Disable inlining of measure()

[Sean Christopherson]

On Wed, Oct 26, 2022, Bill Wendling wrote:
> On Tue, Oct 25, 2022 at 4:12 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Tue, Oct 25, 2022, Bill Wendling wrote:
> > > On Wed, Jun 1, 2022 at 10:22 AM Jim Mattson <jmattson@google.com> wrote:
> > > >
> > > > On Wed, Jun 1, 2022 at 9:30 AM Bill Wendling <morbo@google.com> wrote:
> > > > >
> > > > > From: Bill Wendling <isanbard@gmail.com>

This "From:" should not exist.  It causes your @gmail.com account be to credited
as the author, whereas your SOB suggests you intended this to be credited to your
@google.com address.

Let me know if this should indeed list morbo@google.com as the author, I can fix
it up when applying.

> > > > > Signed-off-by: Bill Wendling <morbo@google.com>
> > > > Reviewed-by: Jim Mattson <jmattson@google.com>

Re: [kvm-unit-tests PATCH] x86/pmu: Disable inlining of measure()

[Bill Wendling]

On Tue, Nov 1, 2022 at 11:53 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Wed, Oct 26, 2022, Bill Wendling wrote:
> > On Tue, Oct 25, 2022 at 4:12 PM Sean Christopherson <seanjc@google.com> wrote:
> > >
> > > On Tue, Oct 25, 2022, Bill Wendling wrote:
> > > > On Wed, Jun 1, 2022 at 10:22 AM Jim Mattson <jmattson@google.com> wrote:
> > > > >
> > > > > On Wed, Jun 1, 2022 at 9:30 AM Bill Wendling <morbo@google.com> wrote:
> > > > > >
> > > > > > From: Bill Wendling <isanbard@gmail.com>
>
> This "From:" should not exist.  It causes your @gmail.com account be to credited
> as the author, whereas your SOB suggests you intended this to be credited to your
> @google.com address.
>
> Let me know if this should indeed list morbo@google.com as the author, I can fix
> it up when applying.
>
Please use morbo@google.com. Sorry about this mixup.

-bw

> > > > > > Signed-off-by: Bill Wendling <morbo@google.com>
> > > > > Reviewed-by: Jim Mattson <jmattson@google.com>

Re: [PATCH] x86/pmu: Disable inlining of measure()

[Paolo Bonzini]

On 6/1/22 18:30, Bill Wendling wrote:
> From: Bill Wendling <isanbard@gmail.com>
> 
> Clang can be more aggressive at inlining than GCC and will fully inline
> calls to measure(). This can mess with the counter overflow check. To
> set up the PMC overflow, check_counter_overflow() first records the
> number of instructions retired in an invocation of measure() and checks
> to see that subsequent calls to measure() retire the same number of
> instructions. If inlining occurs, those numbers can be different and the
> overflow test fails.
> 
>    FAIL: overflow: cntr-0
>    PASS: overflow: status-0
>    PASS: overflow: status clear-0
>    PASS: overflow: irq-0
>    FAIL: overflow: cntr-1
>    PASS: overflow: status-1
>    PASS: overflow: status clear-1
>    PASS: overflow: irq-1
>    FAIL: overflow: cntr-2
>    PASS: overflow: status-2
>    PASS: overflow: status clear-2
>    PASS: overflow: irq-2
>    FAIL: overflow: cntr-3
>    PASS: overflow: status-3
>    PASS: overflow: status clear-3
>    PASS: overflow: irq-3
> 
> Disabling inlining of measure() keeps the assumption that all calls to
> measure() retire the same number of instructions.
> 
> Cc: Jim Mattson <jmattson@google.com>
> Signed-off-by: Bill Wendling <morbo@google.com>
> ---
>   x86/pmu.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/x86/pmu.c b/x86/pmu.c
> index a46bdbf4788c..bbfd268aafa4 100644
> --- a/x86/pmu.c
> +++ b/x86/pmu.c
> @@ -211,7 +211,7 @@ static void stop_event(pmu_counter_t *evt)
>   	evt->count = rdmsr(evt->ctr);
>   }
>   
> -static void measure(pmu_counter_t *evt, int count)
> +static noinline void measure(pmu_counter_t *evt, int count)
>   {
>   	int i;
>   	for (i = 0; i < count; i++)

Queued, thanks.

Paolo