Re: [PATCH] x86/pmstat: fold two allocations in get_cpufreq_para()

[Jan Beulich <jbeulich@xxxxxxxx>]

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 <jbeulich@xxxxxxxx>
> 
> Wow this function is a mess.
> 
> It is an improvement, so Acked-by: Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>,

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