x86/pmstat: fold two allocations in get_cpufreq_para()

Commit Message

Jan Beulich March 25, 2025, 12:53 p.m. UTC

There's little point in allocation two uint32_t[] arrays separately.
We'll need the bigger of the two anyway, and hence we can use that
bigger one also for transiently storing the smaller number of items.

While there also drop j (we can use i twice) and adjust the type of
the remaining two variables on that line.

Signed-off-by: Jan Beulich <[e-mail]>

Comments

Andrew Cooper March 25, 2025, 1:52 p.m. UTC | #1

On 25/03/2025 12:53 pm, Jan Beulich wrote:
> There's little point in allocation two uint32_t[] arrays separately.
> We'll need the bigger of the two anyway, and hence we can use that
> bigger one also for transiently storing the smaller number of items.
>
> While there also drop j (we can use i twice) and adjust the type of
> the remaining two variables on that line.
>
> Signed-off-by: Jan Beulich <[e-mail]>

Wow this function is a mess.

It is an improvement, so Acked-by: Andrew Cooper
<[e-mail]>, but the allocations could be removed
entirely by restructuring the logic some more.

Also, one extra observation.

>
> --- a/xen/drivers/acpi/pmstat.c
> +++ b/xen/drivers/acpi/pmstat.c
> @@ -193,11 +193,10 @@ static int get_cpufreq_para(struct xen_s
>      const struct processor_pminfo *pmpt;
>      struct cpufreq_policy *policy;
>      uint32_t gov_num = 0;
> -    uint32_t *affected_cpus;
> -    uint32_t *scaling_available_frequencies;
> +    uint32_t *data;
>      char     *scaling_available_governors;
>      struct list_head *pos;
> -    uint32_t cpu, i, j = 0;
> +    unsigned int cpu, i = 0;
>  
>      pmpt = processor_pminfo[op->cpuid];
>      policy = per_cpu(cpufreq_cpu_policy, op->cpuid);
> @@ -219,25 +218,22 @@ static int get_cpufreq_para(struct xen_s
>          return -EAGAIN;
>      }
>  
> -    if ( !(affected_cpus = xzalloc_array(uint32_t, op->u.get_para.cpu_num)) )
> +    if ( !(data = xzalloc_array(uint32_t,
> +                                max(op->u.get_para.cpu_num,
> +                                    op->u.get_para.freq_num))) )
>          return -ENOMEM;
> +
>      for_each_cpu(cpu, policy->cpus)
> -        affected_cpus[j++] = cpu;
> +        data[i++] = cpu;
>      ret = copy_to_guest(op->u.get_para.affected_cpus,
> -                       affected_cpus, op->u.get_para.cpu_num);
> -    xfree(affected_cpus);
> -    if ( ret )
> -        return ret;
> +                        data, op->u.get_para.cpu_num);
>  
> -    if ( !(scaling_available_frequencies =
> -           xzalloc_array(uint32_t, op->u.get_para.freq_num)) )
> -        return -ENOMEM;
>      for ( i = 0; i < op->u.get_para.freq_num; i++ )
> -        scaling_available_frequencies[i] =
> -                        pmpt->perf.states[i].core_frequency * 1000;
> +        data[i] = pmpt->perf.states[i].core_frequency * 1000;
>      ret = copy_to_guest(op->u.get_para.scaling_available_frequencies,
> -                   scaling_available_frequencies, op->u.get_para.freq_num);
> -    xfree(scaling_available_frequencies);
> +                        data, op->u.get_para.freq_num) ?: ret;
> +
> +    xfree(data);
>      if ( ret )
>          return ret;
>  

Not altered by this patch, but `ret` is bogus here.

It's the number of bytes not copied, and needs transforming into -EFAULT
here and later.

~Andrew

Jan Beulich March 25, 2025, 2 p.m. UTC | #2

On 25.03.2025 14:52, Andrew Cooper wrote:
> On 25/03/2025 12:53 pm, Jan Beulich wrote:
>> There's little point in allocation two uint32_t[] arrays separately.
>> We'll need the bigger of the two anyway, and hence we can use that
>> bigger one also for transiently storing the smaller number of items.
>>
>> While there also drop j (we can use i twice) and adjust the type of
>> the remaining two variables on that line.
>>
>> Signed-off-by: Jan Beulich <[e-mail]>
> 
> Wow this function is a mess.
> 
> It is an improvement, so Acked-by: Andrew Cooper
> <[e-mail]>,

Thanks.

> but the allocations could be removed
> entirely by restructuring the logic some more.

Perhaps.

> Also, one extra observation.
> 
>>
>> --- a/xen/drivers/acpi/pmstat.c
>> +++ b/xen/drivers/acpi/pmstat.c
>> @@ -193,11 +193,10 @@ static int get_cpufreq_para(struct xen_s
>>      const struct processor_pminfo *pmpt;
>>      struct cpufreq_policy *policy;
>>      uint32_t gov_num = 0;
>> -    uint32_t *affected_cpus;
>> -    uint32_t *scaling_available_frequencies;
>> +    uint32_t *data;
>>      char     *scaling_available_governors;
>>      struct list_head *pos;
>> -    uint32_t cpu, i, j = 0;
>> +    unsigned int cpu, i = 0;
>>  
>>      pmpt = processor_pminfo[op->cpuid];
>>      policy = per_cpu(cpufreq_cpu_policy, op->cpuid);
>> @@ -219,25 +218,22 @@ static int get_cpufreq_para(struct xen_s
>>          return -EAGAIN;
>>      }
>>  
>> -    if ( !(affected_cpus = xzalloc_array(uint32_t, op->u.get_para.cpu_num)) )
>> +    if ( !(data = xzalloc_array(uint32_t,
>> +                                max(op->u.get_para.cpu_num,
>> +                                    op->u.get_para.freq_num))) )
>>          return -ENOMEM;
>> +
>>      for_each_cpu(cpu, policy->cpus)
>> -        affected_cpus[j++] = cpu;
>> +        data[i++] = cpu;
>>      ret = copy_to_guest(op->u.get_para.affected_cpus,
>> -                       affected_cpus, op->u.get_para.cpu_num);
>> -    xfree(affected_cpus);
>> -    if ( ret )
>> -        return ret;
>> +                        data, op->u.get_para.cpu_num);
>>  
>> -    if ( !(scaling_available_frequencies =
>> -           xzalloc_array(uint32_t, op->u.get_para.freq_num)) )
>> -        return -ENOMEM;
>>      for ( i = 0; i < op->u.get_para.freq_num; i++ )
>> -        scaling_available_frequencies[i] =
>> -                        pmpt->perf.states[i].core_frequency * 1000;
>> +        data[i] = pmpt->perf.states[i].core_frequency * 1000;
>>      ret = copy_to_guest(op->u.get_para.scaling_available_frequencies,
>> -                   scaling_available_frequencies, op->u.get_para.freq_num);
>> -    xfree(scaling_available_frequencies);
>> +                        data, op->u.get_para.freq_num) ?: ret;
>> +
>> +    xfree(data);
>>      if ( ret )
>>          return ret;
>>  
> 
> Not altered by this patch, but `ret` is bogus here.
> 
> It's the number of bytes not copied, and needs transforming into -EFAULT
> here and later.

Oh, right - I noticed this when making the patch, then forgot again. I can
make another patch, unless you have one in the works already.

Jan

Andrew Cooper March 25, 2025, 2:01 p.m. UTC | #3

On 25/03/2025 2:00 pm, Jan Beulich wrote:
> On 25.03.2025 14:52, Andrew Cooper wrote:
>> On 25/03/2025 12:53 pm, Jan Beulich wrote:
>>> --- a/xen/drivers/acpi/pmstat.c
>>> +++ b/xen/drivers/acpi/pmstat.c
>>> @@ -219,25 +218,22 @@ static int get_cpufreq_para(struct xen_s
>>>          return -EAGAIN;
>>>      }
>>>  
>>> -    if ( !(affected_cpus = xzalloc_array(uint32_t, op->u.get_para.cpu_num)) )
>>> +    if ( !(data = xzalloc_array(uint32_t,
>>> +                                max(op->u.get_para.cpu_num,
>>> +                                    op->u.get_para.freq_num))) )
>>>          return -ENOMEM;
>>> +
>>>      for_each_cpu(cpu, policy->cpus)
>>> -        affected_cpus[j++] = cpu;
>>> +        data[i++] = cpu;
>>>      ret = copy_to_guest(op->u.get_para.affected_cpus,
>>> -                       affected_cpus, op->u.get_para.cpu_num);
>>> -    xfree(affected_cpus);
>>> -    if ( ret )
>>> -        return ret;
>>> +                        data, op->u.get_para.cpu_num);
>>>  
>>> -    if ( !(scaling_available_frequencies =
>>> -           xzalloc_array(uint32_t, op->u.get_para.freq_num)) )
>>> -        return -ENOMEM;
>>>      for ( i = 0; i < op->u.get_para.freq_num; i++ )
>>> -        scaling_available_frequencies[i] =
>>> -                        pmpt->perf.states[i].core_frequency * 1000;
>>> +        data[i] = pmpt->perf.states[i].core_frequency * 1000;
>>>      ret = copy_to_guest(op->u.get_para.scaling_available_frequencies,
>>> -                   scaling_available_frequencies, op->u.get_para.freq_num);
>>> -    xfree(scaling_available_frequencies);
>>> +                        data, op->u.get_para.freq_num) ?: ret;
>>> +
>>> +    xfree(data);
>>>      if ( ret )
>>>          return ret;
>>>  
>> Not altered by this patch, but `ret` is bogus here.
>>
>> It's the number of bytes not copied, and needs transforming into -EFAULT
>> here and later.
> Oh, right - I noticed this when making the patch, then forgot again. I can
> make another patch, unless you have one in the works already.

I've not started one.  Please go ahead.

~Andrew

Patch

--- a/xen/drivers/acpi/pmstat.c
+++ b/xen/drivers/acpi/pmstat.c
@@ -193,11 +193,10 @@  static int get_cpufreq_para(struct xen_s
     const struct processor_pminfo *pmpt;
     struct cpufreq_policy *policy;
     uint32_t gov_num = 0;
-    uint32_t *affected_cpus;
-    uint32_t *scaling_available_frequencies;
+    uint32_t *data;
     char     *scaling_available_governors;
     struct list_head *pos;
-    uint32_t cpu, i, j = 0;
+    unsigned int cpu, i = 0;
 
     pmpt = processor_pminfo[op->cpuid];
     policy = per_cpu(cpufreq_cpu_policy, op->cpuid);
@@ -219,25 +218,22 @@  static int get_cpufreq_para(struct xen_s
         return -EAGAIN;
     }
 
-    if ( !(affected_cpus = xzalloc_array(uint32_t, op->u.get_para.cpu_num)) )
+    if ( !(data = xzalloc_array(uint32_t,
+                                max(op->u.get_para.cpu_num,
+                                    op->u.get_para.freq_num))) )
         return -ENOMEM;
+
     for_each_cpu(cpu, policy->cpus)
-        affected_cpus[j++] = cpu;
+        data[i++] = cpu;
     ret = copy_to_guest(op->u.get_para.affected_cpus,
-                       affected_cpus, op->u.get_para.cpu_num);
-    xfree(affected_cpus);
-    if ( ret )
-        return ret;
+                        data, op->u.get_para.cpu_num);
 
-    if ( !(scaling_available_frequencies =
-           xzalloc_array(uint32_t, op->u.get_para.freq_num)) )
-        return -ENOMEM;
     for ( i = 0; i < op->u.get_para.freq_num; i++ )
-        scaling_available_frequencies[i] =
-                        pmpt->perf.states[i].core_frequency * 1000;
+        data[i] = pmpt->perf.states[i].core_frequency * 1000;
     ret = copy_to_guest(op->u.get_para.scaling_available_frequencies,
-                   scaling_available_frequencies, op->u.get_para.freq_num);
-    xfree(scaling_available_frequencies);
+                        data, op->u.get_para.freq_num) ?: ret;
+
+    xfree(data);
     if ( ret )
         return ret;