Re: [PATCH V4 1/5] perf/x86: Extend event update interface

["Liang, Kan" <kan.liang@linux.intel.com>]

On 2024-08-01 10:03 a.m., Peter Zijlstra wrote:
> On Wed, Jul 31, 2024 at 07:38:31AM -0700, kan.liang@linux.intel.com wrote:
>> From: Kan Liang <kan.liang@linux.intel.com>
>>
>> The current event update interface directly reads the values from the
>> counter, but the values may not be the accurate ones users require. For
>> example, the sample read feature wants the counter value of the member
>> events when the leader event is overflow. But with the current
>> implementation, the read (event update) actually happens in the NMI
>> handler. There may be a small gap between the overflow and the NMI
>> handler.
> 
> This...
> 
>> The new Intel PEBS counters snapshotting feature can provide
>> the accurate counter value in the overflow. The event update interface
>> has to be updated to apply the given accurate values.
>>
>> Pass the accurate values via the event update interface. If the value is
>> not available, still directly read the counter.
> 
>> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
>> index 12f2a0c14d33..07a56bf71160 100644
>> --- a/arch/x86/events/core.c
>> +++ b/arch/x86/events/core.c
>> @@ -112,7 +112,7 @@ u64 __read_mostly hw_cache_extra_regs
>>   * Can only be executed on the CPU where the event is active.
>>   * Returns the delta events processed.
>>   */
>> -u64 x86_perf_event_update(struct perf_event *event)
>> +u64 x86_perf_event_update(struct perf_event *event, u64 *val)
>>  {
>>  	struct hw_perf_event *hwc = &event->hw;
>>  	int shift = 64 - x86_pmu.cntval_bits;
>> @@ -131,7 +131,10 @@ u64 x86_perf_event_update(struct perf_event *event)
>>  	 */
>>  	prev_raw_count = local64_read(&hwc->prev_count);
>>  	do {
>> -		rdpmcl(hwc->event_base_rdpmc, new_raw_count);
>> +		if (!val)
>> +			rdpmcl(hwc->event_base_rdpmc, new_raw_count);
>> +		else
>> +			new_raw_count = *val;
>>  	} while (!local64_try_cmpxchg(&hwc->prev_count,
>>  				      &prev_raw_count, new_raw_count));
>>  
> 
> Does that mean the following is possible?
> 
> Two counters: C0 and C1, where C0 is a PEBS counter that also samples
> C1.
> 
>   C0: overflow-with-PEBS-assist -> PEBS entry with counter value A
>       (DS buffer threshold not reached)
> 
>   C1: overflow -> PMI -> x86_perf_event_update(C1, NULL)
>       rdpmcl reads value 'A+d', and sets prev_raw_count
> 
>   C0: more assists, hit DS threshold -> PMI
>       PEBS processing does x86_perf_event_update(C1, A)
>       and sets prev_raw_count *backwards*

I think the C0 PMI handler doesn't touch other counters unless
PERF_SAMPLE_READ is applied. For the PERF_SAMPLE_READ, only one counter
does sampling. It's impossible that C0 and C1 do sampling at the same
time. I don't think the above scenario is possible.

Maybe we can add the below check to further prevent the abuse of the
interface.

WARN_ON_ONCE(!(event->attr.sample_type & PERF_SAMPLE_READ) && val);

Thanks,
Kan