Re: [PATCH v6 03/10] perf/x86/rapl: Remove the cpu_to_rapl_pmu() function

[Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>]

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@amd.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
>>>>
>