Re: [BUG] perf/x86/intel/pebs: PEBS timestamps overwritten

From: Ravi Bangoria <ravi.bangoria@amd.com>
To: "Liang, Kan" <kan.liang@linux.intel.com>,
	Stephane Eranian <eranian@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
	"Liang, Kan" <kan.liang@intel.com>,
	Andi Kleen <ak@linux.intel.com>,
	andreas.kogler.0x@gmail.com, LKML <linux-kernel@vger.kernel.org>,
	ravi.bangoria@amd.com
Subject: Re: [BUG] perf/x86/intel/pebs: PEBS timestamps overwritten
Date: Mon, 8 Aug 2022 14:59:15 +0530	[thread overview]
Message-ID: <deaffe26-d164-cf59-5847-f80ee4437dff@amd.com> (raw)
In-Reply-To: <4fdaeb87-7f0a-78e0-2952-3035a3776198@linux.intel.com>

On 05-Aug-22 7:06 PM, Liang, Kan wrote:
> 
> 
> On 2022-08-05 6:49 a.m., Stephane Eranian wrote:
>> Hi,
>>
>> I was alerted by an internal user that the PEBS TSC-based timestamps
>> do not appear
>> correctly in the final perf.data output file from perf record.
>>
>> After some investigation, I came to the conclusion that indeed the
>> data->time field setup
>> by PEBS in the setup_pebs_fixed_sample_data() is later overwritten by
>> perf_events generic
>> code in perf_prepare_sample(). There is an ordering problem here.
>>
>> Looking around we found that this problem had been uncovered back in
>> May 2020 and a
>> patch had been posted then:
>> https://lore.kernel.org/lkml/e754b625-bf14-8f5f-bd1a-71d774057005@gmail.com/T/
>>
>> However this patch was never commented upon or committed.
>>
>> The problem is still present in the upstream code today.
>>
>> 1. perf_sample_data_init()
>> 2. setup_pebs_fixed_sample_data(): data->time =
>> native_sched_clock_from_tsc(pebs->tsc);
>> 3. perf_prepare_sample(): data->time = perf_event_clock(event);
>>
>> The patch from 2020 (Andreas Kogler) fixes the problem by making the
>> assignment in 3.
>> conditioned to the value of data->time being 0. Andreas also suggested
>> an alternative which
>> would break up the call to perf_event_ouput() like this is done in the
>> BTS code allowing
>> the prepare_sample() call to be made before PEBS samples are
>> extracted. That would
>> generate some code duplication. Although this approach appears more
>> robust, the one
>> issue I see is that prepare_sample may need data that would be filled
>> by PEBS and
>> therefore it would  need to be called afterwards.
>>
>> Any better ideas?
> 
> I think Andreas's patch is the most straightforward and simplest patch
> to fix the issue. But, if I recall correctly, Peter prefers to minimize
> the cachelines touched by the perf_sample_data_init(). So initializing
> the data->time in the perf_sample_data_init() seems break the rule.
> 
> I think HW will provide more and more such kind of precise information.
> Maybe we can use a flag variable to track whether the information is
> already provided to avoid the overwritten.

fwiw, we had similar quirks in the past. For ex: __PERF_SAMPLE_CALLCHAIN_EARLY

Thanks,
Ravi