| Message ID | 1408049924-18848-1-git-send-email-chris.j.arges@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Il 14/08/2014 22:58, Chris J Arges ha scritto: > In the check_counters_many function measure was only being called on the last > counter, causing the pmu test to fail. I don't understand. measure loops on all N counters and calls start_event (which in turn calls global_enable) and stop_event (which calls global_disable) on all counters. > This ensures that measure is called for > each counter in the array before calling verify_counter. Actually the point of this test is to run the loop while all the counters are active, so this patch is just masking another bug. Paolo > Signed-off-by: Chris J Arges <chris.j.arges@canonical.com> > --- > x86/pmu.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/x86/pmu.c b/x86/pmu.c > index 5c85146..3402d1e 100644 > --- a/x86/pmu.c > +++ b/x86/pmu.c > @@ -287,11 +287,11 @@ static void check_counters_many(void) > n++; > } > > - measure(cnt, n); > - > - for (i = 0; i < n; i++) > + for (i = 0; i < n; i++) { > + measure(&cnt[i], 1); > if (!verify_counter(&cnt[i])) > break; > + } > > report("all counters", i == n); > } > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/25/2014 11:45 AM, Paolo Bonzini wrote: > Il 14/08/2014 22:58, Chris J Arges ha scritto: >> In the check_counters_many function measure was only being called on the last >> counter, causing the pmu test to fail. > > I don't understand. measure loops on all N counters and calls > start_event (which in turn calls global_enable) and stop_event (which > calls global_disable) on all counters. > >> This ensures that measure is called for >> each counter in the array before calling verify_counter. > > Actually the point of this test is to run the loop while all the > counters are active, so this patch is just masking another bug. > > Paolo Paolo, Ok I see now where this patch doesn't make sense. With the latest kvm tree I get: sudo ./x86-run x86/pmu.flat -smp 1 -cpu host | grep -v PASS qemu-system-x86_64 -enable-kvm -device pc-testdev -device isa-debug-exit,iobase=0xf4,iosize=0x4 -display none -serial stdio -device pci-testdev -kernel x86/pmu.flat -smp 1 -cpu host enabling apic paging enabled cr0 = 80010011 cr3 = 7fff000 cr4 = 20 PMU version: 2 GP counters: 4 GP counter width: 48 Mask length: 7 Fixed counters: 3 Fixed counter width: 48 FAIL: all counters SUMMARY: 67 tests, 1 unexpected failures Return value from qemu: 3 I've tested this on a few Intel platforms (sandybridge/haswell), I'll look into the code more then. Thanks, --chris j arges > >> Signed-off-by: Chris J Arges <chris.j.arges@canonical.com> >> --- >> x86/pmu.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/x86/pmu.c b/x86/pmu.c >> index 5c85146..3402d1e 100644 >> --- a/x86/pmu.c >> +++ b/x86/pmu.c >> @@ -287,11 +287,11 @@ static void check_counters_many(void) >> n++; >> } >> >> - measure(cnt, n); >> - >> - for (i = 0; i < n; i++) >> + for (i = 0; i < n; i++) { >> + measure(&cnt[i], 1); >> if (!verify_counter(&cnt[i])) >> break; >> + } >> >> report("all counters", i == n); >> } >> > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Ok I see now where this patch doesn't make sense. > With the latest kvm tree I get: > > sudo ./x86-run x86/pmu.flat -smp 1 -cpu host | grep -v PASS > > > qemu-system-x86_64 -enable-kvm -device pc-testdev -device > isa-debug-exit,iobase=0xf4,iosize=0x4 -display none -serial stdio > -device pci-testdev -kernel x86/pmu.flat -smp 1 -cpu host > enabling apic > paging enabled > cr0 = 80010011 > cr3 = 7fff000 > cr4 = 20 > PMU version: 2 > GP counters: 4 > GP counter width: 48 > Mask length: 7 > Fixed counters: 3 > Fixed counter width: 48 > FAIL: all counters > > SUMMARY: 67 tests, 1 unexpected failures > Return value from qemu: 3 > > I've tested this on a few Intel platforms (sandybridge/haswell), I'll > look into the code more then. Are you using the NMI watchdog in the host? It eats one PMU counter and makes this test fail. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/25/2014 02:32 PM, Paolo Bonzini wrote: >> Ok I see now where this patch doesn't make sense. >> With the latest kvm tree I get: >> >> sudo ./x86-run x86/pmu.flat -smp 1 -cpu host | grep -v PASS >> >> >> qemu-system-x86_64 -enable-kvm -device pc-testdev -device >> isa-debug-exit,iobase=0xf4,iosize=0x4 -display none -serial stdio >> -device pci-testdev -kernel x86/pmu.flat -smp 1 -cpu host >> enabling apic >> paging enabled >> cr0 = 80010011 >> cr3 = 7fff000 >> cr4 = 20 >> PMU version: 2 >> GP counters: 4 >> GP counter width: 48 >> Mask length: 7 >> Fixed counters: 3 >> Fixed counter width: 48 >> FAIL: all counters >> >> SUMMARY: 67 tests, 1 unexpected failures >> Return value from qemu: 3 >> >> I've tested this on a few Intel platforms (sandybridge/haswell), I'll >> look into the code more then. > > > Are you using the NMI watchdog in the host? It eats one PMU counter > and makes this test fail. > > Paolo > Ah, I didn't know that. Yes disabling NMI watchdog via: echo 0 | sudo tee /proc/sys/kernel/nmi_watchdog Allows this test to pass. Would it make sense to have a check if nmi_watchdog is enabled in this test case, and skip the all counters test? --chris j arges -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Il 25/08/2014 21:38, Chris J Arges ha scritto: > Ah, I didn't know that. Yes disabling NMI watchdog via: > echo 0 | sudo tee /proc/sys/kernel/nmi_watchdog > Allows this test to pass. > > Would it make sense to have a check if nmi_watchdog is enabled in this > test case, and skip the all counters test? You can check for it in procfs, but that means testing in runtests.sh. You can add a test for it and just skip the entire PMU test. Bonus points for somehow adding the test to unittests.cfg instead of hardcoding the test name in the shell script. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/x86/pmu.c b/x86/pmu.c index 5c85146..3402d1e 100644 --- a/x86/pmu.c +++ b/x86/pmu.c @@ -287,11 +287,11 @@ static void check_counters_many(void) n++; } - measure(cnt, n); - - for (i = 0; i < n; i++) + for (i = 0; i < n; i++) { + measure(&cnt[i], 1); if (!verify_counter(&cnt[i])) break; + } report("all counters", i == n); }
In the check_counters_many function measure was only being called on the last counter, causing the pmu test to fail. This ensures that measure is called for each counter in the array before calling verify_counter. Signed-off-by: Chris J Arges <chris.j.arges@canonical.com> --- x86/pmu.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)