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

[Namhyung Kim]

On Tue, Nov 21, 2023 at 11:26 AM Liang, Kan <kan.liang@xxxxxxxxxxxxxxx> wrote:
>
>
>
> 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.

Thank Kan for the review.

Peter and Ingo, would you please pick up the first two patches
if you don't have any concerns?  Then I can work on the
renaming on top.

Thanks,
Namhyung