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: linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org,
	John Garry <john.garry@huawei.com>, Will Deacon <will@kernel.org>,
	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
Subject: Re: [PATCH 4/5] perf arm-spe: Implement find_snapshot callback
Date: Mon, 4 Oct 2021 20:27:24 +0800	[thread overview]
Message-ID: <20211004122724.GC174271@leoy-ThinkPad-X240s> (raw)
In-Reply-To: <1c6a3a73-27dc-6673-7fe7-34bc7fcb0a68@arm.com>

Hi German,

On Thu, Sep 30, 2021 at 01:26:15PM +0100, German Gomez wrote:

[...]

> The patch is indeed based on that commit. The reason behind it is that the
> values for *head are being wrapped in the driver side (see the macro
> PERF_IDX2OFF which is used at various points in
> /drivers/perf/arm_spe_pmu.c).

Yes, I noted that Arm SPE driver doesn't use monotonical increasing
for AUX head.

> If this callback is not to be added, I believe the driver needs to be
> updated > first so that the head pointer monotonically increases like in cs-etm. Do
> you think this makes sense for SPE?

Please note, there have two cases should be handled for snapshot mode:
- Wrap-around case, somehow function __auxtrace_mmap__read() has
  handled this case, see [1];
- It's possible that there have overrun case for snapshot mode, e.g.
  the kernel space receives multiple signals and take snapshot to save
  Arm SPE trace data into AUX buffer for multiple times, but the
  userspace tool cannot catch up to save AUX data into perf.data file.
  Thus the AUX head might be wrapped around for multiple times, for
  this case, I think monotonically increasing AUX head is the right
  solution to handle overrun issue.

So simply say, I think the head pointer monotonically increasing is
the right thing to do in Arm SPE driver.

> (note that the patch will skip the wrap-around detection if this is the
> case,
> in order to handle both cases in the userspace perf tool).

Almost agree, I read multiple times but have no idea what's the
"both cases" in the last sentence.

Please let me know if anything is not clear.

Thanks,
Leo

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/util/auxtrace.c#n1804