Re: [Xen-devel] [PATCH v2 4/9] x86/intel_pstate: add new policy fields and a new driver interface

["Wang, Wei W" <wei.w.wang@xxxxxxxxx>]

On 26/05/2015 21:00, Jan Beulich wrote
> >>> On 13.05.16 at 09:50, <wei.w.wang@xxxxxxxxx> wrote:
> > --- a/xen/drivers/cpufreq/utility.c
> > +++ b/xen/drivers/cpufreq/utility.c
> > @@ -456,6 +456,11 @@ int __cpufreq_set_policy(struct cpufreq_policy
> > *data,
> >
> >      data->min = policy->min;
> >      data->max = policy->max;
> > +    data->min_perf_pct = policy->min_perf_pct;
> > +    data->max_perf_pct = policy->max_perf_pct;
> 
> Shouldn't you do similar sanity checks on them as are being done on
> ->min and ->max? (Admittedly they look a little strange, but I'm not
> asking you to copy what is there without sanity checking the original).
> 
> > +    if (cpufreq_driver->setpolicy)
> 
> Also I wonder whether the additions shouldn't move into this if()'s body.

Will change it to the if()'s body:
        if (cpufreq_driver->setpolicy) {
            data->min_perf_pct = policy->min_perf_pct;
            data->max_perf_pct = policy->max_perf_pct;
            ....
        }


> 
> > --- a/xen/include/acpi/cpufreq/cpufreq.h
> > +++ b/xen/include/acpi/cpufreq/cpufreq.h
> > @@ -52,6 +52,10 @@ struct cpufreq_policy {
> >      unsigned int        max;    /* in kHz */
> >      unsigned int        cur;    /* in kHz, only needed if cpufreq
> >                                   * governors are used */
> > +    int                 min_perf_pct; /* min performance in percentage */
> > +    int                 max_perf_pct; /* max performance in percentage */
> > +    int                 turbo_pct;
> 
> Can any of these legitimately be negative? If not, they should be of unsigned
> int type.

I think we have to keep using "int". Though the expected pct value range is 
0-100, it is possible that people may input a negative value. For example, if 
the input value is "-1": 
1) using int, the output value after clamp_t(0,100) will be 0;
2) using unsigned int, the output value after clamp_t(0,100) will be 100.
I think case 1) is what we need.  


> 
> > @@ -87,6 +91,12 @@ struct cpufreq_freqs {
> >   *                          CPUFREQ GOVERNORS                        *
> >
> >
> **********************************************************
> ***********/
> >
> > +/* the four internal governors used in intel_pstate */
> > +#define CPUFREQ_POLICY_POWERSAVE        (1)
> > +#define CPUFREQ_POLICY_PERFORMANCE      (2)
> > +#define CPUFREQ_POLICY_USERSPACE        (3)
> > +#define CPUFREQ_POLICY_ONDEMAND         (4)
> 
> Why are they being added in this header and at this point in the sequence of
> patches? If they're local to intel_pstate, they should go there. If they're
> needed by multiple files, they should be added when needed, so that one
> can easily judge whether their addition is indeed necessary.

Will change to add the 4 macros in the "main body of intel_pstate driver" patch 
(6/9). 
Can we keep them in this file? These are cpufreq related things.

Best,
Wei


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel