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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.05.15 at 11:47, <wei.w.wang@xxxxxxxxx> wrote:
> On 26/05/2015 21:00, Jan Beulich wrote
>> >>> On 13.05.16 at 09:50, <wei.w.wang@xxxxxxxxx> wrote:
>> > --- 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.  

There are no "people" involved here. If you think negative values
make sense to be accepted by the xenpm tool, deal with them there.
Already at the hypercall interface only sane values should be passed
(and insane ones, like percentage values outside the [0,100] range,
probably be rejected).

>> > @@ -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.

Perhaps - what I do mind is the combination of the when and where.
Fixing the when may eliminate the where complaint.

Jan


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