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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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>, 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