|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 10/14 RESEND] xen: Add SET_CPUFREQ_HWP xen_sysctl_pm_op
On Mon, May 8, 2023 at 7:27 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 01.05.2023 21:30, Jason Andryuk wrote:
> > @@ -531,6 +533,100 @@ int get_hwp_para(const struct cpufreq_policy *policy,
> > return 0;
> > }
> >
> > +int set_hwp_para(struct cpufreq_policy *policy,
> > + struct xen_set_hwp_para *set_hwp)
>
> const?
set_hwp can be const. policy is passed to hwp_cpufreq_target() &
on_selected_cpus(), so it cannot readily be made const.
> > +{
> > + unsigned int cpu = policy->cpu;
> > + struct hwp_drv_data *data = per_cpu(hwp_drv_data, cpu);
> > +
> > + if ( data == NULL )
> > + return -EINVAL;
> > +
> > + /* Validate all parameters first */
> > + if ( set_hwp->set_params & ~XEN_SYSCTL_HWP_SET_PARAM_MASK )
> > + return -EINVAL;
> > +
> > + if ( set_hwp->activity_window & ~XEN_SYSCTL_HWP_ACT_WINDOW_MASK )
> > + return -EINVAL;
>
> Below you limit checks to when the respective control bit is set. I
> think you want the same here.
Not sure if you mean feature_hwp_activity_window or the bit in
set_params as control bit. But, yes, they can both use some
additional checking. IIRC, I wanted to always check
~XEN_SYSCTL_HWP_ACT_WINDOW_MASK, because bits should never be set
whether or not the activity window is supported by hardware.
> > + if ( !feature_hwp_energy_perf &&
> > + (set_hwp->set_params & XEN_SYSCTL_HWP_SET_ENERGY_PERF) &&
> > + set_hwp->energy_perf > IA32_ENERGY_BIAS_MAX_POWERSAVE )
> > + return -EINVAL;
> > +
> > + if ( (set_hwp->set_params & XEN_SYSCTL_HWP_SET_DESIRED) &&
> > + set_hwp->desired != 0 &&
> > + (set_hwp->desired < data->hw.lowest ||
> > + set_hwp->desired > data->hw.highest) )
> > + return -EINVAL;
> > +
> > + /*
> > + * minimum & maximum are not validated as hardware doesn't seem to care
> > + * and the SDM says CPUs will clip internally.
> > + */
> > +
> > + /* Apply presets */
> > + switch ( set_hwp->set_params & XEN_SYSCTL_HWP_SET_PRESET_MASK )
> > + {
> > + case XEN_SYSCTL_HWP_SET_PRESET_POWERSAVE:
> > + data->minimum = data->hw.lowest;
> > + data->maximum = data->hw.lowest;
> > + data->activity_window = 0;
> > + if ( feature_hwp_energy_perf )
> > + data->energy_perf = HWP_ENERGY_PERF_MAX_POWERSAVE;
> > + else
> > + data->energy_perf = IA32_ENERGY_BIAS_MAX_POWERSAVE;
> > + data->desired = 0;
> > + break;
> > +
> > + case XEN_SYSCTL_HWP_SET_PRESET_PERFORMANCE:
> > + data->minimum = data->hw.highest;
> > + data->maximum = data->hw.highest;
> > + data->activity_window = 0;
> > + data->energy_perf = HWP_ENERGY_PERF_MAX_PERFORMANCE;
> > + data->desired = 0;
> > + break;
> > +
> > + case XEN_SYSCTL_HWP_SET_PRESET_BALANCE:
> > + data->minimum = data->hw.lowest;
> > + data->maximum = data->hw.highest;
> > + data->activity_window = 0;
> > + if ( feature_hwp_energy_perf )
> > + data->energy_perf = HWP_ENERGY_PERF_BALANCE;
> > + else
> > + data->energy_perf = IA32_ENERGY_BIAS_BALANCE;
> > + data->desired = 0;
> > + break;
> > +
> > + case XEN_SYSCTL_HWP_SET_PRESET_NONE:
> > + break;
> > +
> > + default:
> > + return -EINVAL;
> > + }
>
> So presets set all the values for which the individual item control bits
> are clear. That's not exactly what I would have expected, and it took me
> reading the code several times until I realized that you write life per-
> CPU data fields here, not fields of some intermediate variable. I think
> this could do with saying explicitly in the public header (if indeed the
> intended model).
The commit message mentioned the idea of using a preset and further
refinement. The comments above "/* Apply presets */" and below "/*
Further customize presets if needed */" were an attempt to highlight
that. But you are right that the public header should state this
clearly.
> > + /* Further customize presets if needed */
> > + if ( set_hwp->set_params & XEN_SYSCTL_HWP_SET_MINIMUM )
> > + data->minimum = set_hwp->minimum;
> > +
> > + if ( set_hwp->set_params & XEN_SYSCTL_HWP_SET_MAXIMUM )
> > + data->maximum = set_hwp->maximum;
> > +
> > + if ( set_hwp->set_params & XEN_SYSCTL_HWP_SET_ENERGY_PERF )
> > + data->energy_perf = set_hwp->energy_perf;
> > +
> > + if ( set_hwp->set_params & XEN_SYSCTL_HWP_SET_DESIRED )
> > + data->desired = set_hwp->desired;
> > +
> > + if ( set_hwp->set_params & XEN_SYSCTL_HWP_SET_ACT_WINDOW )
> > + data->activity_window = set_hwp->activity_window &
> > + XEN_SYSCTL_HWP_ACT_WINDOW_MASK;
> > +
> > + hwp_cpufreq_target(policy, 0, 0);
> > +
> > + return 0;
>
> I don't think you should assume here that hwp_cpufreq_target() will
> only ever return 0. Plus by returning its return value here you
> allow the compiler to tail-call optimize this code.
Thanks for catching that. Yeah, I made hwp_cpufreq_target() return a
value per your earlier comment, so its value should be returned now.
> > --- a/xen/drivers/acpi/pmstat.c
> > +++ b/xen/drivers/acpi/pmstat.c
> > @@ -398,6 +398,20 @@ static int set_cpufreq_para(struct xen_sysctl_pm_op
> > *op)
> > return ret;
> > }
> >
> > +static int set_cpufreq_hwp(struct xen_sysctl_pm_op *op)
>
> const?
Yes
> > --- a/xen/include/public/sysctl.h
> > +++ b/xen/include/public/sysctl.h
> > @@ -317,6 +317,34 @@ struct xen_hwp_para {
> > uint8_t energy_perf;
> > };
> >
> > +/* set multiple values simultaneously when set_args bit is set */
>
> What "set_args bit" does this comment refer to?
That should be set_params. IIRC, set_args was the previous name.
> > +struct xen_set_hwp_para {
> > +#define XEN_SYSCTL_HWP_SET_DESIRED (1U << 0)
> > +#define XEN_SYSCTL_HWP_SET_ENERGY_PERF (1U << 1)
> > +#define XEN_SYSCTL_HWP_SET_ACT_WINDOW (1U << 2)
> > +#define XEN_SYSCTL_HWP_SET_MINIMUM (1U << 3)
> > +#define XEN_SYSCTL_HWP_SET_MAXIMUM (1U << 4)
> > +#define XEN_SYSCTL_HWP_SET_PRESET_MASK 0xf000
> > +#define XEN_SYSCTL_HWP_SET_PRESET_NONE 0x0000
> > +#define XEN_SYSCTL_HWP_SET_PRESET_BALANCE 0x1000
> > +#define XEN_SYSCTL_HWP_SET_PRESET_POWERSAVE 0x2000
> > +#define XEN_SYSCTL_HWP_SET_PRESET_PERFORMANCE 0x3000
> > +#define XEN_SYSCTL_HWP_SET_PARAM_MASK ( \
> > + XEN_SYSCTL_HWP_SET_PRESET_MASK | \
> > + XEN_SYSCTL_HWP_SET_DESIRED | \
> > + XEN_SYSCTL_HWP_SET_ENERGY_PERF | \
> > + XEN_SYSCTL_HWP_SET_ACT_WINDOW | \
> > + XEN_SYSCTL_HWP_SET_MINIMUM | \
> > + XEN_SYSCTL_HWP_SET_MAXIMUM )
> > + uint16_t set_params; /* bitflags for valid values */
> > +#define XEN_SYSCTL_HWP_ACT_WINDOW_MASK 0x03ff
> > + uint16_t activity_window; /* See comment in struct xen_hwp_para */
> > + uint8_t minimum;
> > + uint8_t maximum;
> > + uint8_t desired;
> > + uint8_t energy_perf; /* 0-255 or 0-15 depending on HW support */
>
> Instead of (or in addition to) the "HW support" reference, could this
> gain a reference to the "get para" bit determining which range to use?
I've removed the fallback 0-15 support locally, so this will be removed.
Regards,
Jason
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |