powercap: intel_rapl: Optimize rp->domains memory allocation

In the memory allocation of rp->domains in rapl_detect_domains(), there
is an additional memory of struct rapl_domain allocated to prevent the
pointer out of bounds called later.

Optimize the code here to save sizeof(struct rapl_domain) bytes of
memory.

Test in Intel NUC (i5-1135G7).

Signed-off-by: xiongxin <xiongxin@kylinos.cn>
Tested-by: xiongxin <xiongxin@kylinos.cn>
---
 drivers/powercap/intel_rapl_common.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

在 2023/4/5 00:30, Rafael J. Wysocki 写道:
> On Tue, Apr 4, 2023 at 10:37 AM xiongxin <xiongxin@kylinos.cn> wrote:
>>
>> In the memory allocation of rp->domains in rapl_detect_domains(), there
>> is an additional memory of struct rapl_domain allocated to prevent the
>> pointer out of bounds called later.
>>
>> Optimize the code here to save sizeof(struct rapl_domain) bytes of
>> memory.
>>
>> Test in Intel NUC (i5-1135G7).
>>
>> Signed-off-by: xiongxin <xiongxin@kylinos.cn>
>> Tested-by: xiongxin <xiongxin@kylinos.cn>
>> ---
>>   drivers/powercap/intel_rapl_common.c | 15 ++++++++-------
>>   1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/powercap/intel_rapl_common.c b/drivers/powercap/intel_rapl_common.c
>> index 8970c7b80884..f8971282498a 100644
>> --- a/drivers/powercap/intel_rapl_common.c
>> +++ b/drivers/powercap/intel_rapl_common.c
>> @@ -1171,13 +1171,14 @@ static int rapl_package_register_powercap(struct rapl_package *rp)
>>   {
>>          struct rapl_domain *rd;
>>          struct powercap_zone *power_zone = NULL;
>> -       int nr_pl, ret;
>> +       int nr_pl, ret, i;
>>
>>          /* Update the domain data of the new package */
>>          rapl_update_domain_data(rp);
>>
>>          /* first we register package domain as the parent zone */
>> -       for (rd = rp->domains; rd < rp->domains + rp->nr_domains; rd++) {
>> +       for (i = 0; i < rp->nr_domains; i++) {
>> +               rd = &rp->domains[i];
>>                  if (rd->id == RAPL_DOMAIN_PACKAGE) {
>>                          nr_pl = find_nr_power_limit(rd);
>>                          pr_debug("register package domain %s\n", rp->name);
>> @@ -1201,8 +1202,9 @@ static int rapl_package_register_powercap(struct rapl_package *rp)
>>                  return -ENODEV;
>>          }
>>          /* now register domains as children of the socket/package */
>> -       for (rd = rp->domains; rd < rp->domains + rp->nr_domains; rd++) {
>> +       for (i = 0; i < rp->nr_domains; i++) {
>>                  struct powercap_zone *parent = rp->power_zone;
>> +               rd = &rp->domains[i];
>>
>>                  if (rd->id == RAPL_DOMAIN_PACKAGE)
>>                          continue;
>> @@ -1302,7 +1304,6 @@ static void rapl_detect_powerlimit(struct rapl_domain *rd)
>>    */
>>   static int rapl_detect_domains(struct rapl_package *rp, int cpu)
>>   {
>> -       struct rapl_domain *rd;
>>          int i;
>>
>>          for (i = 0; i < RAPL_DOMAIN_MAX; i++) {
>> @@ -1319,15 +1320,15 @@ static int rapl_detect_domains(struct rapl_package *rp, int cpu)
>>          }
>>          pr_debug("found %d domains on %s\n", rp->nr_domains, rp->name);
>>
>> -       rp->domains = kcalloc(rp->nr_domains + 1, sizeof(struct rapl_domain),
>> +       rp->domains = kcalloc(rp->nr_domains, sizeof(struct rapl_domain),
>>                                GFP_KERNEL);
> 
> Why can't you modify just the above statement alone?  What would break
> if you did that?
At the beginning, I just didn't understand what this '+1' was for, but
contacting the for loop behind, here '+1', just because the rd pointer
will not point outside the memory of 'rp->domains' before the for loop
exits.

if '+1' is to prevent the invalidation of the rd pointer, and apply for
an additional struct rapl_domains here, then my patch may not be 
completely modified.

> 
>>          if (!rp->domains)
>>                  return -ENOMEM;
>>
>>          rapl_init_domains(rp);
>>
>> -       for (rd = rp->domains; rd < rp->domains + rp->nr_domains; rd++)
>> -               rapl_detect_powerlimit(rd);

If '+1' is directly removed, before the for loop exits, the rd pointer 
has already pointed to outside the memory of rp->domains.

>> +       for (i = 0; i < rp->nr_domains; i++)
>> +               rapl_detect_powerlimit(&rp->domains[i]);
>>
>>          return 0;
>>   }
>> --
>> 2.34.1
>>

On Thu, Apr 6, 2023 at 3:23 AM TGSP <xiongxin@kylinos.cn> wrote:
>
> 在 2023/4/5 00:30, Rafael J. Wysocki 写道:
> > On Tue, Apr 4, 2023 at 10:37 AM xiongxin <xiongxin@kylinos.cn> wrote:
> >>
> >> In the memory allocation of rp->domains in rapl_detect_domains(), there
> >> is an additional memory of struct rapl_domain allocated to prevent the
> >> pointer out of bounds called later.
> >>
> >> Optimize the code here to save sizeof(struct rapl_domain) bytes of
> >> memory.
> >>
> >> Test in Intel NUC (i5-1135G7).
> >>
> >> Signed-off-by: xiongxin <xiongxin@kylinos.cn>
> >> Tested-by: xiongxin <xiongxin@kylinos.cn>
> >> ---
> >>   drivers/powercap/intel_rapl_common.c | 15 ++++++++-------
> >>   1 file changed, 8 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/powercap/intel_rapl_common.c b/drivers/powercap/intel_rapl_common.c
> >> index 8970c7b80884..f8971282498a 100644
> >> --- a/drivers/powercap/intel_rapl_common.c
> >> +++ b/drivers/powercap/intel_rapl_common.c
> >> @@ -1171,13 +1171,14 @@ static int rapl_package_register_powercap(struct rapl_package *rp)
> >>   {
> >>          struct rapl_domain *rd;
> >>          struct powercap_zone *power_zone = NULL;
> >> -       int nr_pl, ret;
> >> +       int nr_pl, ret, i;
> >>
> >>          /* Update the domain data of the new package */
> >>          rapl_update_domain_data(rp);
> >>
> >>          /* first we register package domain as the parent zone */
> >> -       for (rd = rp->domains; rd < rp->domains + rp->nr_domains; rd++) {
> >> +       for (i = 0; i < rp->nr_domains; i++) {
> >> +               rd = &rp->domains[i];
> >>                  if (rd->id == RAPL_DOMAIN_PACKAGE) {
> >>                          nr_pl = find_nr_power_limit(rd);
> >>                          pr_debug("register package domain %s\n", rp->name);
> >> @@ -1201,8 +1202,9 @@ static int rapl_package_register_powercap(struct rapl_package *rp)
> >>                  return -ENODEV;
> >>          }
> >>          /* now register domains as children of the socket/package */
> >> -       for (rd = rp->domains; rd < rp->domains + rp->nr_domains; rd++) {
> >> +       for (i = 0; i < rp->nr_domains; i++) {
> >>                  struct powercap_zone *parent = rp->power_zone;
> >> +               rd = &rp->domains[i];
> >>
> >>                  if (rd->id == RAPL_DOMAIN_PACKAGE)
> >>                          continue;
> >> @@ -1302,7 +1304,6 @@ static void rapl_detect_powerlimit(struct rapl_domain *rd)
> >>    */
> >>   static int rapl_detect_domains(struct rapl_package *rp, int cpu)
> >>   {
> >> -       struct rapl_domain *rd;
> >>          int i;
> >>
> >>          for (i = 0; i < RAPL_DOMAIN_MAX; i++) {
> >> @@ -1319,15 +1320,15 @@ static int rapl_detect_domains(struct rapl_package *rp, int cpu)
> >>          }
> >>          pr_debug("found %d domains on %s\n", rp->nr_domains, rp->name);
> >>
> >> -       rp->domains = kcalloc(rp->nr_domains + 1, sizeof(struct rapl_domain),
> >> +       rp->domains = kcalloc(rp->nr_domains, sizeof(struct rapl_domain),
> >>                                GFP_KERNEL);
> >
> > Why can't you modify just the above statement alone?  What would break
> > if you did that?
> At the beginning, I just didn't understand what this '+1' was for, but
> contacting the for loop behind, here '+1', just because the rd pointer
> will not point outside the memory of 'rp->domains' before the for loop
> exits.
>
> if '+1' is to prevent the invalidation of the rd pointer, and apply for
> an additional struct rapl_domains here, then my patch may not be
> completely modified.
>
> >
> >>          if (!rp->domains)
> >>                  return -ENOMEM;
> >>
> >>          rapl_init_domains(rp);
> >>
> >> -       for (rd = rp->domains; rd < rp->domains + rp->nr_domains; rd++)
> >> -               rapl_detect_powerlimit(rd);
>
> If '+1' is directly removed, before the for loop exits, the rd pointer
> has already pointed to outside the memory of rp->domains.

But that value is never dereferenced AFAICS.

> >> +       for (i = 0; i < rp->nr_domains; i++)
> >> +               rapl_detect_powerlimit(&rp->domains[i]);
> >>
> >>          return 0;
> >>   }
> >> --