Re: [PATCH RFC 2/3] perf/x86/intel/pt: Add support for pause_resume()

[Adrian Hunter <adrian.hunter@intel.com>]

On 29/11/23 12:58, Peter Zijlstra wrote:
> On Wed, Nov 29, 2023 at 09:53:39AM +0000, James Clark wrote:
>> On 23/11/2023 12:18, Adrian Hunter wrote:
> 
>>> +static void pt_event_pause_resume(struct perf_event *event)
>>> +{
>>> +	if (event->aux_paused)
>>> +		pt_config_stop(event);
>>> +	else if (!event->hw.state)
>>> +		pt_config_start(event);
>>> +}
>>
>> It seems like having a single pause/resume callback rather than separate
>> pause and resume ones pushes some of the event state management into the
>> individual drivers and would be prone to code duplication and divergent
>> behavior.
>>
>> Would it be possible to move the conditions from here into the core code
>> and call separate functions instead?
>>
>>> +
>>>  static void pt_event_start(struct perf_event *event, int mode)
>>>  {
>>>  	struct hw_perf_event *hwc = &event->hw;
>>> @@ -1798,6 +1809,7 @@ static __init int pt_init(void)
>>>  	pt_pmu.pmu.del			 = pt_event_del;
>>>  	pt_pmu.pmu.start		 = pt_event_start;
>>>  	pt_pmu.pmu.stop			 = pt_event_stop;
>>> +	pt_pmu.pmu.pause_resume		 = pt_event_pause_resume;
>>
>> The general idea seems ok to me. Is there a reason to not use the
>> existing start() stop() callbacks, rather than adding a new one?
>>
>> I assume it's intended to be something like an optimisation where you
>> can turn it on and off without having to do the full setup, teardown and
>> emit an AUX record because you know the process being traced never gets
>> switched out?
> 
> So the actual scheduling uses ->add() / ->del(), the ->start() /
> ->stop() methods are something that can be used after ->add() and before
> ->del() to 'temporarily' pause things.
> 
> Pretty much exactly what is required here I think. We currently use this
> for PMI throttling and adaptive frequency stuff, but there is no reason
> it could not also be used for this.
> 
> As is, we don't track the paused state across ->del() / ->add(), but
> perhaps that can be fixed. We can easily add more PERF_EF_ / PERF_HES_
> bits to manage things.
> 
> 

I am not sure stop / start play nice with NMI's from other events e.g.

PMC NMI wants to pause or resume AUX but what if AUX event is currently
being processed in ->stop() or ->start()?  Or maybe that can't happen?