| Message ID | 20220819110939.78013-4-likexu@tencent.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/pmu: Test case optimization, fixes and additions | expand |
On Fri, Aug 19, 2022, Like Xu wrote: > From: Like Xu <likexu@tencent.com> > > The pmu test check_counter_overflow() always fails with the "./configure > --arch=i386". Explicitly state that the failures are with 32-bit binaries. E.g. I can and do run KUT in 32-bit VMs, which doesn't require the explicit --arch=i386. > The cnt.count obtained from the latter run of measure() > (based on fixed counter 0) is not equal to the expected value (based > on gp counter 0) and there is a positive error with a value of 2. > > The two extra instructions come from inline wrmsr() and inline rdmsr() > inside the global_disable() binary code block. Specifically, for each msr > access, the i386 code will have two assembly mov instructions before > rdmsr/wrmsr (mark it for fixed counter 0, bit 32), but only one assembly > mov is needed for x86_64 and gp counter 0 on i386. > > Fix the expected init cnt.count for fixed counter 0 overflow based on > the same fixed counter 0, not always using gp counter 0. You lost me here. I totally understand the problem, but I don't understand the fix. > Signed-off-by: Like Xu <likexu@tencent.com> > --- > x86/pmu.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/x86/pmu.c b/x86/pmu.c > index 45ca2c6..057fd4a 100644 > --- a/x86/pmu.c > +++ b/x86/pmu.c > @@ -315,6 +315,9 @@ static void check_counter_overflow(void) > > if (i == nr_gp_counters) { > cnt.ctr = fixed_events[0].unit_sel; > + __measure(&cnt, 0); > + count = cnt.count; > + cnt.count = 1 - count; This definitely needs a comment. Dumb question time: if the count is off by 2, why can't we just subtract 2? #ifndef __x86_64__ /* comment about extra MOV insns for RDMSR/WRMSR */ cnt.count -= 2; #endif > cnt.count &= (1ull << pmu_fixed_counter_width()) - 1; > } > > -- > 2.37.2 >
On 6/10/2022 6:18 am, Sean Christopherson wrote: > On Fri, Aug 19, 2022, Like Xu wrote: >> From: Like Xu <likexu@tencent.com> >> >> The pmu test check_counter_overflow() always fails with the "./configure >> --arch=i386". > > Explicitly state that the failures are with 32-bit binaries. E.g. I can and do > run KUT in 32-bit VMs, which doesn't require the explicit --arch=i386. True and applied. > >> The cnt.count obtained from the latter run of measure() >> (based on fixed counter 0) is not equal to the expected value (based >> on gp counter 0) and there is a positive error with a value of 2. >> >> The two extra instructions come from inline wrmsr() and inline rdmsr() >> inside the global_disable() binary code block. Specifically, for each msr >> access, the i386 code will have two assembly mov instructions before >> rdmsr/wrmsr (mark it for fixed counter 0, bit 32), but only one assembly >> mov is needed for x86_64 and gp counter 0 on i386. >> >> Fix the expected init cnt.count for fixed counter 0 overflow based on >> the same fixed counter 0, not always using gp counter 0. > > You lost me here. I totally understand the problem, but I don't understand the > fix. The sequence of instructions to count events using the #GP and #Fixed counters is different. Thus the fix is quite high level, to use the same counter (w/ same instruction sequences) to set initial value for the same counter. I may add this to the commit message. > >> Signed-off-by: Like Xu <likexu@tencent.com> >> --- >> x86/pmu.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/x86/pmu.c b/x86/pmu.c >> index 45ca2c6..057fd4a 100644 >> --- a/x86/pmu.c >> +++ b/x86/pmu.c >> @@ -315,6 +315,9 @@ static void check_counter_overflow(void) >> >> if (i == nr_gp_counters) { >> cnt.ctr = fixed_events[0].unit_sel; >> + __measure(&cnt, 0); >> + count = cnt.count; >> + cnt.count = 1 - count; > > This definitely needs a comment. > > Dumb question time: if the count is off by 2, why can't we just subtract 2? More low-level code (bringing in differences between the 32-bit and 64-bit runtimes) being added would break this. The test goal is simply to set the initial value of a counter to overflow, which is always off by 1, regardless of the involved rd/wrmsr or other execution details. > > #ifndef __x86_64__ > /* comment about extra MOV insns for RDMSR/WRMSR */ > cnt.count -= 2; > #endif > >> cnt.count &= (1ull << pmu_fixed_counter_width()) - 1; >> } >> >> -- >> 2.37.2 >>
On Mon, Oct 17, 2022, Like Xu wrote: > On 6/10/2022 6:18 am, Sean Christopherson wrote: > > > --- > > > x86/pmu.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/x86/pmu.c b/x86/pmu.c > > > index 45ca2c6..057fd4a 100644 > > > --- a/x86/pmu.c > > > +++ b/x86/pmu.c > > > @@ -315,6 +315,9 @@ static void check_counter_overflow(void) > > > if (i == nr_gp_counters) { > > > cnt.ctr = fixed_events[0].unit_sel; > > > + __measure(&cnt, 0); > > > + count = cnt.count; > > > + cnt.count = 1 - count; > > > > This definitely needs a comment. > > > > Dumb question time: if the count is off by 2, why can't we just subtract 2? > > More low-level code (bringing in differences between the 32-bit and 64-bit runtimes) > being added would break this. > > The test goal is simply to set the initial value of a counter to overflow, > which is always off by 1, regardless of the involved rd/wrmsr or other > execution details. Oooh, I see what this code is doing. But wouldn't it be better to offset from '0'? E.g. if the measured workload is a single instruction, then the measured count will be '1' and thus "1 - count" will be zero, meaning no overflow will occur. Ah, but as per the SDM, the "+1" is needed to ensure the overflow is detected immediately. Here, however, if an interrupt is to be generated after 100 event counts, the counter should be preset to minus 100 plus 1 (-100 + 1), or -99. The counter will then overflow after it counts 99 events and generate an interrupt on the next (100th) event counted. The difference of 1 for this count enables the interrupt to be generated immediately after the selected event count has been reached, instead of waiting for the overflow to be propagation through the counter. What about adding a helper to measure/compute the overflow preset value? That would provide a convenient location to document the (IMO) weird behavior that's necessary to ensure immediate event delivery. E.g. --- x86/pmu.c | 36 ++++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/x86/pmu.c b/x86/pmu.c index f891053f..a38ae3f6 100644 --- a/x86/pmu.c +++ b/x86/pmu.c @@ -325,16 +325,30 @@ static void check_counters_many(void) report(i == n, "all counters"); } -static void check_counter_overflow(void) +static uint64_t measure_for_overflow(pmu_counter_t *cnt) { - uint64_t count; - int i; - pmu_counter_t cnt = { - .ctr = gp_counter_base, - .config = EVNTSEL_OS | EVNTSEL_USR | gp_events[1].unit_sel /* instructions */, - }; __measure(&cnt, 0); - count = cnt.count; + + /* + * To generate overflow, i.e. roll over to '0', the initial count just + * needs to be preset to the negative expected count. However, as per + * Intel's SDM, the preset count needs to be incremented by 1 to ensure + * the overflow interrupt is generated immediately instead of possibly + * waiting for the overflow to propagate through the counter. + */ + assert(cnt.count > 1); + return 1 - cnt.count; +} + +static void check_counter_overflow(void) +{ + uint64_t overflow_preset; + int i; + pmu_counter_t cnt = { + .ctr = gp_counter_base, + .config = EVNTSEL_OS | EVNTSEL_USR | gp_events[1].unit_sel /* instructions */, + }; + overflow_preset = measure_for_overflow(&cnt); /* clear status before test */ if (pmu_version() > 1) { @@ -349,7 +363,7 @@ static void check_counter_overflow(void) int idx; cnt.ctr = get_gp_counter_msr(i); - cnt.count = 1 - count; + cnt.count = overflow_preset; if (gp_counter_base == MSR_IA32_PMC0) cnt.count &= (1ull << pmu_gp_counter_width()) - 1; @@ -358,9 +372,7 @@ static void check_counter_overflow(void) break; cnt.ctr = fixed_events[0].unit_sel; - __measure(&cnt, 0); - count = cnt.count; - cnt.count = 1 - count; + cnt.count = measure_for_overflow(&cnt); cnt.count &= (1ull << pmu_fixed_counter_width()) - 1; } base-commit: c3e384a2268baed99d4b59dd239c98bd6a5471eb --
diff --git a/x86/pmu.c b/x86/pmu.c index 45ca2c6..057fd4a 100644 --- a/x86/pmu.c +++ b/x86/pmu.c @@ -315,6 +315,9 @@ static void check_counter_overflow(void) if (i == nr_gp_counters) { cnt.ctr = fixed_events[0].unit_sel; + __measure(&cnt, 0); + count = cnt.count; + cnt.count = 1 - count; cnt.count &= (1ull << pmu_fixed_counter_width()) - 1; }