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

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

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@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?
> 
> 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
>>>>>>
>>>
>