Hi,
The test looks much more readable now, some comments below.
On Wed, May 31, 2023 at 10:14:37PM +0200, Eric Auger wrote:
> Add a new basic test that runs MEM_ACCESS loop over
> 100 iterations and make sure the number of measured
> MEM_ACCESS never overflows the margin. Some other
> pmu tests rely on this pattern and if the MEM_ACCESS
> measurement is not reliable, it is better to report
> it beforehand and not confuse the user any further.
>
> Without the subsequent patch, this typically fails on
> ThunderXv2 with the following logs:
>
> INFO: pmu: pmu-mem-access-reliability: 32-bit overflows:
> overflow=1 min=21 max=41 COUNT=20 MARGIN=15
> FAIL: pmu: pmu-mem-access-reliability: 32-bit overflows:
> mem_access count is reliable
>
> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
>
> ---
>
> v1 -> v2:
> - use mem-access instead of memaccess as suggested by Mark
> - simplify the logic and add comments in the test loop
> ---
> arm/pmu.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++
> arm/unittests.cfg | 6 +++++
> 2 files changed, 64 insertions(+)
>
> diff --git a/arm/pmu.c b/arm/pmu.c
> index 74c9f6f9..925f277c 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -56,6 +56,7 @@
> #define EXT_COMMON_EVENTS_HIGH 0x403F
>
> #define ALL_SET_32 0x00000000FFFFFFFFULL
> +#define ALL_SET_64 0xFFFFFFFFFFFFFFFFULL
> #define ALL_CLEAR 0x0000000000000000ULL
> #define PRE_OVERFLOW_32 0x00000000FFFFFFF0ULL
> #define PRE_OVERFLOW_64 0xFFFFFFFFFFFFFFF0ULL
> @@ -67,6 +68,10 @@
> * for some observed variability we take into account a given @MARGIN
> */
> #define PRE_OVERFLOW2_32 (ALL_SET_32 - COUNT - MARGIN)
> +#define PRE_OVERFLOW2_64 (ALL_SET_64 - COUNT - MARGIN)
> +
> +#define PRE_OVERFLOW2(__overflow_at_64bits) \
> + (__overflow_at_64bits ? PRE_OVERFLOW2_64 : PRE_OVERFLOW2_32)
>
> #define PRE_OVERFLOW(__overflow_at_64bits) \
> (__overflow_at_64bits ? PRE_OVERFLOW_64 : PRE_OVERFLOW_32)
> @@ -747,6 +752,56 @@ static void disable_chain_counter(int even)
> isb();
> }
>
> +/*
> + * This test checks that a mem access loop featuring COUNT accesses
> + * does not overflow with an init value of PRE_OVERFLOW2. It also
> + * records the min/max access count to see how much the counting
> + * is (un)reliable
> + */
> +static void test_mem_access_reliability(bool overflow_at_64bits)
> +{
> + uint32_t events[] = {MEM_ACCESS};
> + void *addr = malloc(PAGE_SIZE);
> + uint64_t count, delta, max = 0, min = pmevcntr_mask();
> + uint64_t pre_overflow2 = PRE_OVERFLOW2(overflow_at_64bits);
> + uint64_t pmcr_lp = overflow_at_64bits ? PMU_PMCR_LP : 0;
> + bool overflow = false;
> +
> + if (!satisfy_prerequisites(events, ARRAY_SIZE(events)) ||
> + !check_overflow_prerequisites(overflow_at_64bits))
> + return;
> +
> + pmu_reset();
> + write_regn_el0(pmevtyper, 0, MEM_ACCESS | PMEVTYPER_EXCLUDE_EL0);
> + for (int i = 0; i < 100; i++) {
> + pmu_reset();
> + write_regn_el0(pmevcntr, 0, pre_overflow2);
> + write_sysreg_s(0x1, PMCNTENSET_EL0);
> + isb();
> + mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> + count = read_regn_el0(pmevcntr, 0);
> + if (count >= pre_overflow2) {
> + /* not counter overflow, as expected */
> + delta = count - pre_overflow2;
Personally, I find the names confusing. Since the test tries to see how
much the counting is unreliable, I would have have expected delta to be
difference between the expected number of events incremented (i.e., COUNT)
and the actual number of events recorded in the counter. I would rename
count to cntr_val and delta to num_events, but that might be just personal
bias and I leave it up to you if think this might be useful.
> + } else {
> + /*
> + * unexpected counter overflow meaning the actual
> + * mem access count, delta, is count + COUNT + MARGIN
> + */
> + delta = count + COUNT + MARGIN;
This assumes that PRE_OVERFLOW2_{32,64} = ALL_SET{32,64} - COUNT - MARGIN,
which might change in the future.
I think a better way to do that is:
delta = count + all_set - pre_overflow2, where
all_set = overflow_at_64bits ? ALL_SET64 : ALL_SET32.
That is a lot easier to parse for someone who doesn't know exactly how
PRE_OVERFLOW2_* is defined, and more robust.
> + overflow = true;
> + report_info("iter=%d count=%ld min=%ld max=%ld overflow!!!",
> + i, delta, min, max);
I find this message extremely confusing: it does not print count (the value
read from counter 0) like the text displayed suggests, it prints delta,
which represents the number of events counted by the counter.
Besides those minor issues, the patch looks correct. Also ran the test on a
rockpro64 under KVM + qemu.
Thanks,
Alex
> + }
> + /* record extreme value */
> + max = MAX(delta, max);
> + min = MIN(delta, min);
> + }
> + report_info("overflow=%d min=%ld max=%ld COUNT=%d MARGIN=%d",
> + overflow, min, max, COUNT, MARGIN);
> + report(!overflow, "mem_access count is reliable");
> +}
> +
> static void test_chain_promotion(bool unused)
> {
> uint32_t events[] = {MEM_ACCESS, CHAIN};
> @@ -1204,6 +1259,9 @@ int main(int argc, char *argv[])
> } else if (strcmp(argv[1], "pmu-basic-event-count") == 0) {
> run_event_test(argv[1], test_basic_event_count, false);
> run_event_test(argv[1], test_basic_event_count, true);
> + } else if (strcmp(argv[1], "pmu-mem-access-reliability") == 0) {
> + run_event_test(argv[1], test_mem_access_reliability, false);
> + run_event_test(argv[1], test_mem_access_reliability, true);
> } else if (strcmp(argv[1], "pmu-mem-access") == 0) {
> run_event_test(argv[1], test_mem_access, false);
> run_event_test(argv[1], test_mem_access, true);
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index 5e67b558..fe601cbb 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -90,6 +90,12 @@ groups = pmu
> arch = arm64
> extra_params = -append 'pmu-mem-access'
>
> +[pmu-mem-access-reliability]
> +file = pmu.flat
> +groups = pmu
> +arch = arm64
> +extra_params = -append 'pmu-mem-access-reliability'
> +
> [pmu-sw-incr]
> file = pmu.flat
> groups = pmu
> --
> 2.38.1
>
