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

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Thu, Jun 15, 2023 at 11:22 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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?

Certainly.

> > These governor fields get indented, and that needed some re-formatting.
>
> Would it maybe make sense to split the function?

While that could be done, I don't think it's needed.  Maybe you'll
feel otherwise after you look at "cpufreq: Export HWP parameters to
userspace as CPPC".

Regards,
Jason