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

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

On 2024-08-01 12:36 p.m., Peter Zijlstra wrote:
> On Thu, Aug 01, 2024 at 11:31:40AM -0400, Liang, Kan wrote:
>>
>>
>> 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.
> 
> It is perfectly fine for C0 to have PERF_SAMPLE_READ and C1 to be a
> normal counter, sampling or otherwise.
> 
>> Maybe we can add the below check to further prevent the abuse of the
>> interface.
> 
> There is no abuse in the above scenario. You can have a group with all
> sampling events and any number of them can have PERF_SAMPLE_READ. This
> is perfectly fine.
> 
>> WARN_ON_ONCE(!(event->attr.sample_type & PERF_SAMPLE_READ) && val);
> 
> I don't see how PERF_SAMPLE_READ is relevant, *any* PMI for the C1 event
> will cause x86_perf_event_update() to be called. And remember that even
> non-sampling events have EVENTSEL_INT set to deal with counter overflow.
> 
> The problem here is that C0/PEBS will come in late and try to force
> update an out-of-date value.
> 
> If you have C1 be a non-sampling event, this will typically not happen,
> but it still *can*, and when you do, you get your counter moving
> backwards.

Now, only the PEBS records may include an out-of-date value. I think we
can always drain PEBS before handling the overflow from the non PEBS
event overflow.

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index 0c9c2706d4ec..255eb7231181 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -3109,6 +3109,9 @@ static int handle_pmi_common(struct pt_regs *regs,
u64 status)
 		if (!test_bit(bit, cpuc->active_mask))
 			continue;

+		if (event->hw.flags & PERF_X86_EVENT_PEBS_CNTR)
+			x86_pmu.drain_pebs(regs, &data);
+
 		if (!intel_pmu_save_and_restart(event))
 			continue;


Thanks,
Kan