Messages in this thread | | | Date | Tue, 21 Nov 2023 14:26:32 -0500 | Subject | Re: [PATCH 3/3] perf/x86: Add CAP_NO_INTERRUPT for uncore PMUs | From | "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@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.
Thanks, Kan
| |