Re: [PATCH 4/5] perf arm-spe: Implement find_snapshot callback

[German Gomez]

Hi Leo,

On 17/10/2021 13:05, Leo Yan wrote:
> On Thu, Sep 16, 2021 at 04:46:34PM +0100, German Gomez wrote:
>
> [...]
>
> If run a test case (the test is pasted at the end of the reply), I
> can get quite different AUX trace data with passing different wait
> period before sending the first USR2 signal.
>
>   # sh test_arm_spe_snapshot.sh 2
>   Couldn't synthesize bpf events.
>   stress: info: [5768] dispatching hogs: 1 cpu, 0 io, 0 vm, 0 hdd
>   [ perf record: Woken up 3 times to write data ]
>   [ perf record: Captured and wrote 2.833 MB perf.data ]
>
>   # sh test_arm_spe_snapshot.sh 10
>   Couldn't synthesize bpf events.
>   stress: info: [5776] dispatching hogs: 1 cpu, 0 io, 0 vm, 0 hdd
>   [ perf record: Woken up 3 times to write data ]
>   [ perf record: Captured and wrote 24.356 MB perf.data ]
>
> The first command passes argument '2' so the test will wait for 2
> seconds before send USR2 signal for snapshot, and the perf data file is
> 2.833 MB (so this means the Arm SPE trace data is about 2MB) for three
> snapshots.  In the second command, the argument '10' means it will wait
> for 10 seconds before sending the USR2 signals, and every time it records
> the trace data from the full AUX buffer (8MB), at the end it gets 24MB
> AUX trace data.
>
> The issue happens in the second command, waiting for 10 seconds leads
> to the *full* AUX ring buffer is filled by Arm SPE, so the function
> arm_spe_buffer_has_wrapped() always return back true for this case.
> Afterwards, arm_spe_find_snapshot() doesn't respect the passed old
> header (from '*old') and assumes the trace data size is 'mm->len'.

Returning the entire contents of the buffer once the first wrap-around
was detected was the intention of the patch, so I don't currently see it
as wrong. What were the values you were expecting to see in the test?

If the handling of snapshot mode by the perf tool can be improved after
upstreaming the changes to the driver, we could submit a followup patch
after that has been fixed.

>
> To allow arm_spe_buffer_has_wrapped() to work properly, I think we
> need to clean up the top 8 bytes of the AUX buffer in Arm SPE driver
> when start the PMU event (please note, this change has an assumption
> that is meantioned in another email that suggests to remove redundant
> PERF_RECORD_AUX events so the function arm_spe_perf_aux_output_begin()
> is invoked only once when start PMU event, so we can use the top 8
> bytes in AUX buffer to indicate trace is wrap around or not).
>
>
> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> index d44bcc29d99c..eb35f85d0efb 100644
> --- a/drivers/perf/arm_spe_pmu.c
> +++ b/drivers/perf/arm_spe_pmu.c
> @@ -493,6 +493,16 @@ static void arm_spe_perf_aux_output_begin(struct perf_output_handle *handle,
>         if (limit)
>                 limit |= BIT(SYS_PMBLIMITR_EL1_E_SHIFT);
>
> +       /*
> +        * Cleanup the top 8 bytes for snapshot mode; these 8 bytes are
> +        * used to indicate if trace data is wrap around if they are not
> +        * zero.
> +        */
> +       if (buf->snapshot) {
> +               void *tail = buf->base + (buf->nr_pages << PAGE_SHIFT) - 8;
> +               memset(tail, 0x0, 8);
> +       }
> +
>         limit += (u64)buf->base;
>         base = (u64)buf->base + PERF_IDX2OFF(handle->head, buf);
>         write_sysreg_s(base, SYS_PMBPTR_EL1);
>
> Thanks,
> Leo

I will try these and the other driver changes and discuss them with the
team internally, thanks!

>
> ---8<---
>
> #!/bin/sh
>
> ./perf record -e arm_spe/period=148576/u -C 0 -S -m8M,8M -- taskset --cpu-list 0 stress --cpu 1 &
>
> PERFPID=$!
>
> echo "sleep $1 seconds" > /sys/kernel/debug/tracing/trace_marker
>
> # Wait for perf program
> sleep  $1
>
> # Send signal to snapshot trace data
> kill -USR2 $PERFPID
> sleep .03
> kill -USR2 $PERFPID
> sleep .03
> kill -USR2 $PERFPID
>
> echo "Stop snapshot" > /sys/kernel/debug/tracing/trace_marker
>
> kill $PERFPID
> wait $PERFPID