linux-kernel - Re: [PATCH 3/3] perf/x86: Add CAP_NO

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <25e006f6-43a2-4046-a14e-a856285f5eed@linux.intel.com>
Date:   Tue, 21 Nov 2023 14:26:32 -0500
From:   "Liang, Kan" <kan.liang@...ux.intel.com>
To:     Namhyung Kim <namhyung@...nel.org>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Ian Rogers <irogers@...gle.com>,
        Mingwei Zhang <mizhang@...gle.com>
Subject: Re: [PATCH 3/3] perf/x86: Add CAP_NO_INTERRUPT for uncore PMUs



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@...ux.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@...nel.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