Re: [PATCH 1/2] perf arm-spe: Add arm_spe_record to synthesized

From: Ali Saidi <alisaidi@amazon.com>
To: <german.gomez@arm.com>
Cc: <acme@kernel.org>, <alexander.shishkin@linux.intel.com>,
	<alisaidi@amazon.com>, <andrew.kilroy@arm.com>,
	<benh@kernel.crashing.org>, <james.clark@arm.com>,
	<john.garry@huawei.com>, <jolsa@redhat.com>, <leo.yan@linaro.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>,
	<linux-perf-users@vger.kernel.org>, <mark.rutland@arm.com>,
	<mathieu.poirier@linaro.org>, <mingo@redhat.com>,
	<namhyung@kernel.org>, <peterz@infradead.org>, <will@kernel.org>
Subject: Re: [PATCH 1/2] perf arm-spe: Add arm_spe_record to synthesized
Date: Thu, 27 Jan 2022 19:13:45 +0000	[thread overview]
Message-ID: <20220127191345.18173-1-alisaidi@amazon.com> (raw)
In-Reply-To: <c4a96839-1b4b-ac4d-38aa-26c1f4c8645a@arm.com>

On 26/01/2022 19:07, German Gomez wrote:
[...]
>>> Have you tried this with perf-inject? I think it would need the PERF_SAMPLE_RAW bit in the sample_type,
>> Yes I've tried the following and it worked as expected with the original
>> perf.data or the perf.data.jitted after perf-inject. 
>>
>> perf record -e arm_spe_0/jitter=1/ -k 1 java ...
>> perf  inject -f --jit -i perf.data -o perf.data.jitted
>
>This is not injecting the synthesized samples. I think it is still    
>processing from the aux trace. Try adding "--itrace=i1i --strip" to the
>inject command to remove the AUXTRACE events. Judging by the raw
>samples, the data is missing:
>
> [...]

Yep, you're correct here. If I use the command above the raw samples are lost.

>>> Although I quickly looked over the perf inject code and it looks like it's expecting some type of padding:
>>>
>>> [...]
>>>
>>> I'm seeing some comments in utils/event.h related to this on the intel events.
>> Yes i noticed this too,but looking at how the raw data is added to the same
>> other places like intel-pt.c:1703 the perf_synth__raw*() functions are used to
>> strip away the 4 bytes bytes before the data is added to the sample. The other
>> places i can find the padding used is in builtin-script.c but given we have the
>> --dump-raw-trace option it's not clear to me that it's needed to wrap the
>> arm_spe_event in another struct with padding like perf_synth_intel_ptwrite?
>
>I think the intel use case makes sense because the layout of the data
>is fixed and documented. If we modify the struct arm_spe_record later it
>may not be obvious how to match it to the raw data of an older perf.data
>file. And we're generating bigger files with redundant information.

Not injecting the samples into the perf trace, but having a way to support
custom scripts parsing the data would be really useful and much faster than
trying to parse back the --dump-raw-trace output into something useful. The
other way to go would be to put a header that describes the version of the spe
struct at the head of it to address any future changes, but I'm not familiar
with a workflow that would benefit from the added complexity. 

Thanks,
Ali