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

From: Leo Yan <leo.yan@linaro.org>
To: German Gomez <german.gomez@arm.com>
Cc: Will Deacon <will@kernel.org>,
	linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org,
	John Garry <john.garry@huawei.com>,
	Mathieu Poirier <mathieu.poirier@linaro.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Jiri Olsa <jolsa@redhat.com>, Namhyung Kim <namhyung@kernel.org>,
	Mike Leach <mike.leach@linaro.org>,
	linux-arm-kernel@lists.infradead.org, coresight@lists.linaro.org,
	James Clark <James.Clark@arm.com>
Subject: Re: [PATCH 4/5] perf arm-spe: Implement find_snapshot callback
Date: Sun, 17 Oct 2021 14:13:05 +0800	[thread overview]
Message-ID: <20211017061305.GA130233@leoy-ThinkPad-X240s> (raw)
In-Reply-To: <35209d5c-6387-5248-ab61-a1e1cb0553de@arm.com>

Hi German, Will,

On Fri, Oct 15, 2021 at 01:33:39PM +0100, German Gomez wrote:

[...]

> $ ./perf record -vvv -e arm_spe/period=148576/u -S1000 -m16,16 -- taskset --cpu-list 0 stress --cpu 1 &

When testing Arm SPE snapshot mode with the command (it's quite
similiar with up command but not exactly same):

# ./perf --debug verbose=3 record -e arm_spe/period=148576/u -C 0 -S1000 -m16,16 \
    -- taskset --cpu-list 0 stress --cpu 1 &
# kill -USR2 [pid_num]

... then I wait for long time and didn't stop the perf program, then
I observed the output file contains many redundant events
PERF_RECORD_AUX.  E.g. in the shared perf data file [1], you could use
below commands to see tons of the events PERF_RECORD_AUX which I only
send only one USR2 signal for taking snapshot:

  # perf report -D -i perf.data --stdio | grep -E 'RECORD_AUX' | wc -l
  2245787

  # perf report -D -i perf.data --stdio | grep -E 'SPE'
  . ... ARM SPE data: size 0x3e8 bytes
  Binary file (standard input) matches

I looked into the Arm SPE driver and found it doesn't really support
free run mode for AUX ring buffer when the driver runs in snapshot
mode, the pair functions perf_aux_output_end() and
perf_aux_output_begin() are invoked when every time handle the
interrupt.  The detailed flow is:

  arm_spe_pmu_irq_handler()
    `> arm_spe_pmu_buf_get_fault_act()
         `> arm_spe_perf_aux_output_end()
              `> set SPE registers
              `> perf_aux_output_end()
    `> arm_spe_perf_aux_output_begin()
         `> perf_aux_output_begin()
         `> set SPE registers

Seems to me, a possible solution is to add an extra parameter 'int
in_interrupt' for functions arm_spe_perf_aux_output_end() and
arm_spe_perf_aux_output_begin(), if this parameter is passed as 1 in
the interrupt handling, these two functions should skip invoking
perf_aux_output_end() and perf_aux_output_begin() so can avoid the
redundant perf event PERF_RECORD_AUX.

  arm_spe_pmu_irq_handler()
    `> arm_spe_pmu_buf_get_fault_act()
         `> arm_spe_perf_aux_output_end(..., in_interrupt=1)
              `> set SPE registers
    `> arm_spe_perf_aux_output_begin(..., in_interrupt=1)
         `> set SPE registers

P.s. I think Intel-PT has supported free run mode for snapshot mode,
so it should not generate interrupt in this mode.  Thus Intel-PT can
avoid this issue, please see the code [2].

Thanks,
Leo

[1] https://people.linaro.org/~leo.yan/spe/snapshot_test/perf.data
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/events/intel/pt.c#n753