Re: [PATCH 3/3] perf/x86: Add CAP_NO_INTERRUPT for uncore PMUs

[Liang, Kan]

On 2023-11-21 1:30 p.m., Namhyung Kim wrote:
> Hi Kan,
> 
> On Tue, Nov 21, 2023 at 7:59 AM Liang, Kan <kan.liang@xxxxxxxxxxxxxxx> wrote:
>>
>>
>>
>> On 2023-11-20 5:19 p.m., Namhyung Kim wrote:
>>> It doesn't support sampling in uncore PMU events.  While it's
>>> technically possible to generate interrupts, let's treat it as if it
>>> has no interrupt in order to skip the freq adjust/unthrottling logic
>>> in the timer handler which is only meaningful to sampling events.
>>>
>>> Also remove the sampling event check because it'd be done in the general
>>> code in the perf_event_open syscall.
>>>
>>> Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
>>> ---
>>>  arch/x86/events/intel/uncore.c | 11 ++++++-----
>>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
>>> index 69043e02e8a7..f7e6228bd1b1 100644
>>> --- a/arch/x86/events/intel/uncore.c
>>> +++ b/arch/x86/events/intel/uncore.c
>>> @@ -744,10 +744,6 @@ static int uncore_pmu_event_init(struct perf_event *event)
>>>       if (pmu->func_id < 0)
>>>               return -ENOENT;
>>>
>>> -     /* Sampling not supported yet */
>>> -     if (hwc->sample_period)
>>> -             return -EINVAL;
>>> -
>>>       /*
>>>        * Place all uncore events for a particular physical package
>>>        * onto a single cpu
>>> @@ -919,7 +915,12 @@ static int uncore_pmu_register(struct intel_uncore_pmu *pmu)
>>>                       .stop           = uncore_pmu_event_stop,
>>>                       .read           = uncore_pmu_event_read,
>>>                       .module         = THIS_MODULE,
>>> -                     .capabilities   = PERF_PMU_CAP_NO_EXCLUDE,
>>> +                     /*
>>> +                      * It doesn't allow sampling for uncore events, let's
>>> +                      * treat the PMU has no interrupts to skip them in the
>>> +                      * perf_adjust_freq_unthr_context().
>>> +                      */
>>> +                     .capabilities   = PERF_PMU_CAP_NO_EXCLUDE | PERF_PMU_CAP_NO_INTERRUPT,
>>>                       .attr_update    = pmu->type->attr_update,
>>>               };
>>
>>
>> There is a special customized uncore PMU which needs the flag as well.
> 
> Ok, I will add that too.
> 
> Btw, during the work I noticed many PMU drivers didn't set the
> CAP_NO_INTERRUPT flag even if they didn't support sampling and
> rejected the sampling events manually in the ->event_init() callback.
> 
> I guess it's because the name of the flag is somewhat misleading.
> As the PMU drivers handle IRQ (for overflows), they thought they had
> interrupts and didn't set the flag.  I think it'd be better to rename it to
> CAP_NO_SAMPLING to reveal the intention.  And then we could just set
> the flag in the pmu.capabilities and remove the manual checks.
> 
> The benefit is it can skip the PMUs in the timer tick handler even if
> it needs to unthrottle some events.  What do you think?
> 

I agree. The current name is kind of misleading.

The patch, which introduced the flag (commit id 53b25335dd60 ("perf:
Disable sampled events if no PMU interrupt")), also tried to disable the
sampled events on a no-sampling supported platform.

The renaming sounds good to me.

Thanks,
Kan