Re: [PATCH v4 05/15] pmstat&xenpm: Re-arrage for cpufreq union

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.06.2023 17:05, Jason Andryuk wrote:
> On Thu, Jun 15, 2023 at 10:38 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 14.06.2023 20:02, Jason Andryuk wrote:
>>> Move some code around now that common xen_sysctl_pm_op get_para fields
>>> are together.  In particular, the scaling governor information like
>>> scaling_available_governors is inside the union, so it is not always
>>> available.
>>>
>>> With that, gov_num may be 0, so bounce buffer handling needs
>>> to be modified.
>>>
>>> scaling_governor won't be filled for hwp, so this will simplify the
>>> change when it is introduced.
>>
>> While I think this suitably describes the tool stack side changes, ...
>>
>>> --- a/xen/drivers/acpi/pmstat.c
>>> +++ b/xen/drivers/acpi/pmstat.c
>>> @@ -239,11 +239,24 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op 
>>> *op)
>>>      if ( ret )
>>>          return ret;
>>>
>>> +    op->u.get_para.cpuinfo_cur_freq =
>>> +        cpufreq_driver.get ? cpufreq_driver.get(op->cpuid) : policy->cur;
>>> +    op->u.get_para.cpuinfo_max_freq = policy->cpuinfo.max_freq;
>>> +    op->u.get_para.cpuinfo_min_freq = policy->cpuinfo.min_freq;
>>> +    op->u.get_para.turbo_enabled = cpufreq_get_turbo_status(op->cpuid);
>>> +
>>> +    if ( cpufreq_driver.name[0] )
>>> +        strlcpy(op->u.get_para.scaling_driver,
>>> +            cpufreq_driver.name, CPUFREQ_NAME_LEN);
>>> +    else
>>> +        strlcpy(op->u.get_para.scaling_driver, "Unknown", 
>>> CPUFREQ_NAME_LEN);
>>> +
>>>      if ( !(scaling_available_governors =
>>>             xzalloc_array(char, gov_num * CPUFREQ_NAME_LEN)) )
>>>          return -ENOMEM;
>>> -    if ( (ret = 
>>> read_scaling_available_governors(scaling_available_governors,
>>> -                gov_num * CPUFREQ_NAME_LEN * sizeof(char))) )
>>> +    if ( (ret = read_scaling_available_governors(
>>> +                    scaling_available_governors,
>>> +                    gov_num * CPUFREQ_NAME_LEN * sizeof(char))) )
>>>      {
>>>          xfree(scaling_available_governors);
>>>          return ret;
>>> @@ -254,26 +267,16 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op 
>>> *op)
>>>      if ( ret )
>>>          return ret;
>>>
>>> -    op->u.get_para.cpuinfo_cur_freq =
>>> -        cpufreq_driver.get ? cpufreq_driver.get(op->cpuid) : policy->cur;
>>> -    op->u.get_para.cpuinfo_max_freq = policy->cpuinfo.max_freq;
>>> -    op->u.get_para.cpuinfo_min_freq = policy->cpuinfo.min_freq;
>>> -
>>>      op->u.get_para.u.s.scaling_cur_freq = policy->cur;
>>>      op->u.get_para.u.s.scaling_max_freq = policy->max;
>>>      op->u.get_para.u.s.scaling_min_freq = policy->min;
>>>
>>> -    if ( cpufreq_driver.name[0] )
>>> -        strlcpy(op->u.get_para.scaling_driver,
>>> -            cpufreq_driver.name, CPUFREQ_NAME_LEN);
>>> -    else
>>> -        strlcpy(op->u.get_para.scaling_driver, "Unknown", 
>>> CPUFREQ_NAME_LEN);
>>> -
>>>      if ( policy->governor->name[0] )
>>>          strlcpy(op->u.get_para.u.s.scaling_governor,
>>>              policy->governor->name, CPUFREQ_NAME_LEN);
>>>      else
>>> -        strlcpy(op->u.get_para.u.s.scaling_governor, "Unknown", 
>>> CPUFREQ_NAME_LEN);
>>> +        strlcpy(op->u.get_para.u.s.scaling_governor, "Unknown",
>>> +                CPUFREQ_NAME_LEN);
>>>
>>>      /* governor specific para */
>>>      if ( !strncasecmp(op->u.get_para.u.s.scaling_governor,
>>> @@ -291,7 +294,6 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
>>>              &op->u.get_para.u.s.u.ondemand.sampling_rate,
>>>              &op->u.get_para.u.s.u.ondemand.up_threshold);
>>>      }
>>> -    op->u.get_para.turbo_enabled = cpufreq_get_turbo_status(op->cpuid);
>>>
>>>      return ret;
>>>  }
>>
>> ... all I see on the hypervisor side is re-ordering of steps and 
>> re-formatting
>> of over-long lines. It's not clear to me why what you do is necessary for 
>> your
>> purpose.
> 
> The purpose was to move accesses to the nested struct and union
> "op->u.get_para.u.s.u" to the end of the function, and the accesses to
> common fields (e.g. op->u.get_para.turbo_enabled) earlier.  This
> simplifies the changes in "cpufreq: Export HWP parameters to userspace
> as CPPC".

Could you mention this as (part of) the purpose in the description?

> These governor fields get indented, and that needed some re-formatting.

Would it maybe make sense to split the function?

Jan