| 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
| ||
|
Message-ID: <e6ee937c-f72b-4a04-acba-b00784414603@amd.com>
Date: Mon, 4 Nov 2024 13:34:30 +0530
From: Dhananjay Ugwekar <Dhananjay.Ugwekar@....com>
To: "Zhang, Rui" <rui.zhang@...el.com>,
"gautham.shenoy@....com" <gautham.shenoy@....com>
Cc: "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>,
"linux-perf-users@...r.kernel.org" <linux-perf-users@...r.kernel.org>,
"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
On 11/4/2024 12:45 PM, Zhang, Rui wrote:
> On Mon, 2024-11-04 at 08:45 +0530, Dhananjay Ugwekar wrote:
>> 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?
>
> yes, this doesn't cause any issue.
>
>> , 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.
>
> With this patch, below comment is removed
> /*
> * The unsigned check also catches the '-1' return
> value for non
> * existent mappings in the topology map.
> */
> And we still rely on the unsigned int -> int conversion for the error
> check.
>
> So IMO, we should either add back a similar comment, or convert
> get_rapl_pmu_idx() to return int and modify the error check.
Correct, I think I'll prefer adding a similar comment and keeping the
error check as is, will fix this.
Thanks,
Dhananjay
>
> thanks,
> rui
>
>
>> 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
>>>>>>
>>>
>
Powered by blists - more mailing lists