linux-kernel - Re: [PATCH v6 03/10] perf/x86/rapl: Remove the cpu_to_rapl

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: <2fd6bad3-cb37-4a4a-8b47-d7c2ffc96346@amd.com>
Date: Mon, 4 Nov 2024 08:45:00 +0530
From: Dhananjay Ugwekar <Dhananjay.Ugwekar@....com>
To: "Zhang, Rui" <rui.zhang@...el.com>,
 "gautham.shenoy@....com" <gautham.shenoy@....com>
Cc: "linux-perf-users@...r.kernel.org" <linux-perf-users@...r.kernel.org>,
 "alexander.shishkin@...ux.intel.com" <alexander.shishkin@...ux.intel.com>,
 "ananth.narayan@....com" <ananth.narayan@....com>,
 "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
 "ravi.bangoria@....com" <ravi.bangoria@....com>,
 "Hunter, Adrian" <adrian.hunter@...el.com>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "mingo@...hat.com" <mingo@...hat.com>,
 "irogers@...gle.com" <irogers@...gle.com>,
 "tglx@...utronix.de" <tglx@...utronix.de>,
 "kan.liang@...ux.intel.com" <kan.liang@...ux.intel.com>,
 "mark.rutland@....com" <mark.rutland@....com>,
 "peterz@...radead.org" <peterz@...radead.org>, "bp@...en8.de"
 <bp@...en8.de>, "acme@...nel.org" <acme@...nel.org>,
 "kprateek.nayak@....com" <kprateek.nayak@....com>,
 "jolsa@...nel.org" <jolsa@...nel.org>,
 "namhyung@...nel.org" <namhyung@...nel.org>, "x86@...nel.org"
 <x86@...nel.org>
Subject: Re: [PATCH v6 03/10] perf/x86/rapl: Remove the cpu_to_rapl_pmu()
 function

Hello Rui,

Thanks for reviewing and testing the series!,

On 11/1/2024 1:36 PM, Zhang, Rui wrote:
> On Mon, 2024-10-28 at 14:49 +0530, Dhananjay Ugwekar wrote:
>> Hello Gautham,
>>
>> On 10/28/2024 2:23 PM, Gautham R. Shenoy wrote:
>>> Hello Dhananjay,
>>>
>>> On Fri, Oct 25, 2024 at 11:13:41AM +0000, Dhananjay Ugwekar wrote:
>>>> Prepare for the addition of RAPL core energy counter support.
>>>> Post which, one CPU might be mapped to more than one rapl_pmu
>>>> (package/die one and a core one). So, remove the
>>>> cpu_to_rapl_pmu()
>>>> function.
>>>>
>>>> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@....com>
>>>> ---
>>>>  arch/x86/events/rapl.c | 19 ++++++-------------
>>>>  1 file changed, 6 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
>>>> index f70c49ca0ef3..d20c5b1dd0ad 100644
>>>> --- a/arch/x86/events/rapl.c
>>>> +++ b/arch/x86/events/rapl.c
>>>> @@ -162,17 +162,6 @@ static inline unsigned int
>>>> get_rapl_pmu_idx(int cpu)
>>>>                                         
>>>> topology_logical_die_id(cpu);
>>>>  }
>>>>  
>>>> -static inline struct rapl_pmu *cpu_to_rapl_pmu(unsigned int cpu)
>>>> -{
>>>> -       unsigned int rapl_pmu_idx = get_rapl_pmu_idx(cpu);
>>>> -
>>>> -       /*
>>>> -        * The unsigned check also catches the '-1' return value
>>>> for non
>>>> -        * existent mappings in the topology map.
>>>> -        */
>>>
>>>
>>> See the comment here why rapl_pmu_idx should be an "unsigned int".
>>>
>>>
>>>> -       return rapl_pmu_idx < rapl_pmus->nr_rapl_pmu ? rapl_pmus-
>>>>> pmus[rapl_pmu_idx] : NULL;
>>>> -}
>>>> -
>>>>  static inline u64 rapl_read_counter(struct perf_event *event)
>>>>  {
>>>>         u64 raw;
>>>> @@ -348,7 +337,7 @@ static void rapl_pmu_event_del(struct
>>>> perf_event *event, int flags)
>>>>  static int rapl_pmu_event_init(struct perf_event *event)
>>>>  {
>>>>         u64 cfg = event->attr.config & RAPL_EVENT_MASK;
>>>> -       int bit, ret = 0;
>>>> +       int bit, rapl_pmu_idx, ret = 0;
>>>
>>> Considering that, shouldn't rapl_pmu_idx be an "unsigned int" no?
>>
>> Correct, with unsigned int we will be able to check for negative
>> values as well with the 
>> "if (rapl_pmu_idx >= rapl_pmus->nr_rapl_pmu)" check. Will fix this in
>> next version.
>>
> you can stick with unsigned int here, but in patch 10/10, IMO, making
> get_rapl_pmu_idx() return int instead of unsigned int is more
> straightforward.

But I have one doubt, there wont be any functional difference in returning 
"unsigned int" vs "int" right?, we will still need to check the same condition 
for the return value i.e. "if (rapl_pmu_idx >= rapl_pmus->nr_rapl_pmu)" 
(assuming we are still storing the return value in "unsigned int rapl_pmu_idx"), 
I think I didnt get your point.

Thanks,
Dhananjay

> 
> thanks,
> rui
> 
>> Thanks,
>> Dhananjay
>>
>>>
>>> --
>>> Thanks and Regards
>>> gautham.
>>>
>>>
>>>>         struct rapl_pmu *pmu;
>>>>  
>>>>         /* only look at RAPL events */
>>>> @@ -376,8 +365,12 @@ static int rapl_pmu_event_init(struct
>>>> perf_event *event)
>>>>         if (event->attr.sample_period) /* no sampling */
>>>>                 return -EINVAL;
>>>>  
>>>> +       rapl_pmu_idx = get_rapl_pmu_idx(event->cpu);
>>>> +       if (rapl_pmu_idx >= rapl_pmus->nr_rapl_pmu)
>>>> +               return -EINVAL;
>>>> +
>>>>         /* must be done before validate_group */
>>>> -       pmu = cpu_to_rapl_pmu(event->cpu);
>>>> +       pmu = rapl_pmus->pmus[rapl_pmu_idx];
>>>>         if (!pmu)
>>>>                 return -EINVAL;
>>>>         event->pmu_private = pmu;
>>>> -- 
>>>> 2.34.1
>>>>
>