Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <deaffe26-d164-cf59-5847-f80ee4437dff@amd.com>
Date:   Mon, 8 Aug 2022 14:59:15 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [BUG] perf/x86/intel/pebs: PEBS timestamps overwritten
Content-Language: en-US
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
References: <CABPqkBTqQaaYH+ySu3reLm5i+X6P4BsqKycp8NBOP1gW3ZV4QA@mail.gmail.com>
 <4fdaeb87-7f0a-78e0-2952-3035a3776198@linux.intel.com>
From:   Ravi Bangoria <ravi.bangoria@amd.com>
In-Reply-To: <4fdaeb87-7f0a-78e0-2952-3035a3776198@linux.intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?VWlTckF0TWt6OTNkclVYeDFoQk45czFBcGI4dVphMDNBS0RXb25XU3BuS1Nh?=
 =?utf-8?B?clBUK0Y4RVAzTGQvWmdJc0k0bUtuUDBjRm1XK3dkZVhEZml4L1EvdnBjL1Bo?=
 =?utf-8?B?RXplRlpBdzBGNUdDRE5XSWFTUlBRK2o4ejlPelN4QTJ4UmtGWXBZb1BhWFZr?=
 =?utf-8?B?dXBJVi9EVHcyeTUvbkRCY3VSbUJyVVF1cG1JN1hSUnBYMm1lQTBKZ2hsbTFH?=
 =?utf-8?B?VUlWbGtwdlczWjk0c2JnWmwzTmJoc0dycnpXYW9XWkF5d0VVeXdrSzU0RXhx?=
 =?utf-8?B?V2pweVVaNlYzTENER1dLTVFYbGo1K3dJdUZud0w4WW1VUkFtMzBCd1kralo3?=
 =?utf-8?B?SVVTN0N6VWIwc2wyaGREZmdBVFNmYmFBQVhSbkRkbmNXVTBUTS91NUk0b0Zz?=
 =?utf-8?B?MzhicDhMQmFad3lPdlNMZnRvTTV2d3NRVWQxUWtuK0gvNHQ0SmVhSnZkQWVW?=
 =?utf-8?B?WUF3TVlHQmRlcklhdUtZNnBEOERhUk5QM1pQWEVRaTUycTFKay9YbEVRMG5B?=
 =?utf-8?B?N21uMXJ1MHZJaVJia1VRVzE0NXhOclZxTzBCbVQwc0tyTHE1TXZaR0dNSWtP?=
 =?utf-8?B?VTkzK1Fjb3FaTyttdHhxNFdYejRiZXdyN2t6NnlZeW5rM1ZIVmZnNG5JOFlw?=
 =?utf-8?B?Njg0c1JsY1RlS3gxZ1VzVmtndlA2aDFNSWhTbkRaNmJ0WUxPQ1N4NUV2cVFy?=
 =?utf-8?B?Ky84bEVOTUNqS3FsUFlveGMzYjVpb0hxcDdhYUp1VkhoUmxXamo1aDJKd3Vm?=
 =?utf-8?B?Q3Y0V01meSs1dDBKdDVlQlNMUjRpWWZVMUdaQWh4MVlLR0J4c29zZVVnaVow?=
 =?utf-8?B?TzN2SUZMRmMrTFRtblo4dFl5c1NtTHhGVlBEcllvQ0tldVBtanZvSkFzcTBr?=
 =?utf-8?B?QTRwMG9xNG1nd2NxYlZJV2xRaUhINjlPVlkxRHJEOHpPUWlVOEdTVXpqVWIv?=
 =?utf-8?B?akZ2NHAyclEreGhKaDExSy8wZWEwK1p4R1R6dEd6REJyYXNOclhFa2pSNnl0?=
 =?utf-8?B?RTd0bHVxMXFwbHUweG9RWEhwdk1IZDQ4cElzTUhMcHFnNVVweFNtc2FtRmVt?=
 =?utf-8?B?S1N6WHl0c3ZDN3NuZEF3Y2cxTWYvMVVFalczdi9HNGFrakYxRjVlWEdqUldx?=
 =?utf-8?B?eHVhYS9KaS9SekpIY25aMUxCWDRHaFhCaWhxOVQ0eTNBV2tQelc0Q0lHMThu?=
 =?utf-8?B?V21aWXBtd2VIMEFwaWtKMFJrdUdIUVh6dVlSY1VwNmxZMjRhR2Eza2ZoV2Q2?=
 =?utf-8?B?dUM4WjVpYklMdGJLWjZJYkJ5SHk2Y2Ywa2pFTEdwT3BuVC8vYytPa004MklZ?=
 =?utf-8?B?ZytSWEUyMTk0cmNvaU1vVWFoNzJLa1NJUit2VTdQL0NnME5SR0JhZDNJNTBN?=
 =?utf-8?B?NjBNQWwyK08xcFV4STkxK01McDRCNFNMeWFkTjJseWxpeVZKUXc0NW1saVhL?=
 =?utf-8?B?ZnJmNitSRkg0QnYrQU8yVys3UXR5ZnM0eTBCTnBRTWdoZFIrRG91VHhXRkdQ?=
 =?utf-8?B?QWZPbElGVjZrK3QwTm9EaHZiQ09CRmFxMEJNNEFxb2tZSkRYMzB5SGtzVCth?=
 =?utf-8?B?RnhOWTZYRmRPTGVVYmswL3hBOHY0YXppQ1FWNEt6VDhQTDZvZCttWVFpRGlI?=
 =?utf-8?B?YlZTSVJwbmg3N091ellucUR0OXZHdm9GTEZ0THdBa1hucEcxRXFmVi81L1VP?=
 =?utf-8?B?ekVWVDBnYUR1MkpnT3ovaVp2WU0rbE9xbWVuMDUwSXorQUdESlNLNldSdnEy?=
 =?utf-8?B?SkVuaFNRSnRNK2gzaU8yVWEzZkVKV3VlMTlMekQ0VUtMT3oyRG1BNTJBelBy?=
 =?utf-8?B?SGZNMUc3MStMNVgxbFMzU2RNTDV6eDRIOGZLTW5ibEo2c3FNNEUrd3kwZUo2?=
 =?utf-8?B?TEVodFViZTN1eVpOUGdlSm5UWjV4OEpaSjF1T3lLQ2lpdzVKY29ENFVTKzRy?=
 =?utf-8?B?RzhlRzVuUnBucm5oaGNvc3ZCM2pOMGxRNzhhdSt1RWdEYm5CUzB6Y05zc0Nx?=
 =?utf-8?B?bkgyajcrc0R4bVVxV3d1OXFCQWpsbVM2TzF6K3BieWM0SkZad1FiY2FmdmJN?=
 =?utf-8?B?WW5RZDZhWEp1MDRRcFgycGpCZFZWU1Z3UHZINDN5VU1XcUk4U3pZU0wxOXZo?=
 =?utf-8?Q?4WapjGycGFhbOw9t3Ar6DQDuR?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 688ae933-c37e-4125-ebf7-08da79207da3
X-MS-Exchange-CrossTenant-AuthSource: PH7PR12MB6588.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Aug 2022 09:29:28.3436
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: ElZ7r3LdZ/BV7IZtTlbCjn5tCggxNffS8Y9kW1+blGJQK2YGMk2Wj77e8kb3v/mze8itECQ1eM9DLmqkEHlfmw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR12MB2839
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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