Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
MIME-Version: 1.0
References: <20231120221932.213710-1-namhyung@kernel.org> <20231120221932.213710-3-namhyung@kernel.org>
 <66f74af2-21b6-477b-ada1-a8816ee115dc@linux.intel.com> <CAM9d7chDB0nPcu7_Ks4kp2R=+iVNU8SyEFKHVEsd3d0S=mYRnw@mail.gmail.com>
 <25e006f6-43a2-4046-a14e-a856285f5eed@linux.intel.com>
In-Reply-To: <25e006f6-43a2-4046-a14e-a856285f5eed@linux.intel.com>
From:   Namhyung Kim <namhyung@kernel.org>
Date:   Fri, 1 Dec 2023 12:29:53 -0800
Message-ID: <CAM9d7cj+6CEH89NSs4Jo+BusU3WG-Cw1w9qK7JFbCHHem+381A@mail.gmail.com>
Subject: Re: [PATCH 3/3] perf/x86: Add CAP_NO_INTERRUPT for uncore PMUs
To:     "Liang, Kan" <kan.liang@linux.intel.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@kernel.org>
Cc:     Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Ian Rogers <irogers@google.com>,
        Mingwei Zhang <mizhang@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Tue, Nov 21, 2023 at 11:26 AM Liang, Kan <kan.liang@linux.intel.com> 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@linux.intel.com> 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@kernel.org>
> >>> ---
> >>>  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