Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Message-ID: <25e006f6-43a2-4046-a14e-a856285f5eed@linux.intel.com>
Date:   Tue, 21 Nov 2023 14:26:32 -0500
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 3/3] perf/x86: Add CAP_NO_INTERRUPT for uncore PMUs
Content-Language: en-US
To:     Namhyung Kim <namhyung@kernel.org>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@kernel.org>,
        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>
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>
From:   "Liang, Kan" <kan.liang@linux.intel.com>
In-Reply-To: <CAM9d7chDB0nPcu7_Ks4kp2R=+iVNU8SyEFKHVEsd3d0S=mYRnw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk


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