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.
- */
- 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;
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.1On Fri, 2024-10-25 at 11:13 +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>
Reviewed-by: Zhang Rui <rui.zhang@intel.com>
Tested-by: Zhang Rui <rui.zhang@intel.com>
thanks,
rui
> ---
> 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.
> - */
> - 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;
> 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;
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?
--
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
>
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.
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
>>
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.
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
> > >
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
>>>>
>
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.
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
> > > > >
> >
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
>>>>>>
>>>
>
© 2016 - 2024 Red Hat, Inc.