[PATCH 0/2 v2] cpufreq, intel_pstate, Fix rounding errors

[PATCH 0/2 v2] cpufreq, intel_pstate, Fix rounding errors

[Prarit Bhargava]

I have a Intel (6,63) processor with a "marketing" frequency (from
/proc/cpuinfo) of 2100MHz, and a max turbo frequency of 2600MHz.  I
can execute

cpupower frequency-set -g powersave --min 1200MHz --max 2100MHz

and the max_freq_pct is set to 80.  When adding load to the system I noticed
that the cpu frequency only reached 2000MHZ and not 2100MHz as expected.

I wrote a little test program to set the frequencies in decrements of
100MHz and compared the targeted frequency (the frequency set through
the cpupower command) and the actual frequency (from /proc/cpuinfo), as
well as dumping out the value of the MSR_IA32_PERF_CTL.

Target     Achieved     Difference     MSR(0x199)
 3300        2900          -400          0x1e00
 3200        2900          -300          0x1e00
 3100        2900          -200          0x1e00
 3000        2900          -100          0x1d00
 2900        2800          -100          0x1c00
 2800        2700          -100          0x1b00
 2700        2600          -100          0x1a00
 2600        2500          -100          0x1900
 2500        2400          -100          0x1800
 2400        2300          -100          0x1700
 2300        2200          -100          0x1600
 2200        2100          -100          0x1500
 2100        2000          -100          0x1400
 2000        1900          -100          0x1300
 1900        1800          -100          0x1200
 1800        1700          -100          0x1100
 1700        1600          -100          0x1000
 1600        1500          -100          0xf00
 1500        1400          -100          0xe00
 1400        1300          -100          0xd00
 1300        1200          -100          0xc00
 1200        1200             0          0xc00

As can be seen the frequencies are consistently off by 100MHz.  After
some examination I found a rounding error in intel_pstate_set_policy() for the
calculation of limits->max_policy_pct which needs to be rounded up to the
nearest percentage point.  For example, setting a frequency of 2100MHz on this
system results in limits->max_policy_pct = ((2100 * 100) / 2600) = 80.
However, ((2100 * 100) / 2600) is actually 80.7, or 81.   This is fixed
by expanding the calculation an extra decimal point and rounding to the
nearest percentage point.

A second rounding error was found in the calculation of limits->max_perf
in intel_pstate_set_policy(), which is used to calculate the max and min
pstate values in intel_pstate_get_min_max().  In this case, limits->max_perf
is truncated to 2 hex digits such that, for example, 0x169 was incorrectly
truncated to 0x16 instead of 0x17.  This resulted in the pstate being set
one level too low.

After applying these two fixes we consistently reach the targeted
frequency.

Target     Achieved     Difference     MSR(0x199)
 3300        2900          -400          0x1e00
 3200        2900          -300          0x1e00
 3100        2900          -200          0x1e00
 3000        2900          -100          0x1d00
 2900        2900             0          0x1d00
 2800        2800             0          0x1c00
 2700        2700             0          0x1b00
 2600        2600             0          0x1a00
 2500        2500             0          0x1900
 2400        2400             0          0x1800
 2300        2300             0          0x1700
 2200        2200             0          0x1600
 2100        2100             0          0x1500
 2000        2000             0          0x1400
 1900        1900             0          0x1300
 1800        1800             0          0x1200
 1700        1700             0          0x1100
 1600        1600             0          0x1000
 1500        1500             0          0xf00
 1400        1400             0          0xe00
 1300        1300             0          0xd00
 1200        1200             0          0xc00

Additional tests were run on a (6,78) with HWP enabled and a (6,79)
system.  Testing on both systems showed that the problem was resolved.

Cc: Srinivas Pandruvada <srinivas.pandruvada@intel.com>
Cc: Len Brown <len.brown@intel.com>
Cc: Alexandra Yates <alexandra.yates@intel.com>
Cc: Kristen Carlson Accardi <kristen@linux.intel.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Prarit Bhargava <prarit@redhat.com>

[v2]: Separate into two patches, and rebase onto git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
Prarit Bhargava (2):
  cpufreq, intel_pstate, Fix limits->max_policy_pct rounding error
  cpufreq, intel_pstate, fix limits->max_perf rounding error

 drivers/cpufreq/intel_pstate.c | 3 +++
 1 file changed, 3 insertions(+)

-- 
1.8.3.1

[PATCH 1/2] cpufreq, intel_pstate, Fix limits->max_policy_pct rounding error

[Prarit Bhargava]

I have a Intel (6,63) processor with a "marketing" frequency (from
/proc/cpuinfo) of 2100MHz, and a max turbo frequency of 2600MHz.  I
can execute

cpupower frequency-set -g powersave --min 1200MHz --max 2100MHz

and the max_freq_pct is set to 80.  When adding load to the system I noticed
that the cpu frequency only reached 2000MHZ and not 2100MHz as expected.

This is because limits->max_policy_pct is calculated as 2100 * 100 /2600 = 80.7
and is rounded down to 80 when it should be rounded up to 81.  This patch
adds a DIV_ROUND_UP() which will return the correct value.

Cc: Srinivas Pandruvada <srinivas.pandruvada@intel.com>
Cc: Len Brown <len.brown@intel.com>
Cc: Alexandra Yates <alexandra.yates@intel.com>
Cc: Kristen Carlson Accardi <kristen@linux.intel.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Prarit Bhargava <prarit@redhat.com>
---
 drivers/cpufreq/intel_pstate.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 001a532..6b63374 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -1110,6 +1110,8 @@ static int intel_pstate_set_policy(struct cpufreq_policy *policy)
 	limits->min_policy_pct = clamp_t(int, limits->min_policy_pct, 0 , 100);
 	limits->max_policy_pct = (policy->max * 100) / policy->cpuinfo.max_freq;
 	limits->max_policy_pct = clamp_t(int, limits->max_policy_pct, 0 , 100);
+	limits->max_policy_pct = DIV_ROUND_UP(policy->max * 100,
+					      policy->cpuinfo.max_freq);
 
 	/* Normalize user input to [min_policy_pct, max_policy_pct] */
 	limits->min_perf_pct = max(limits->min_policy_pct,
-- 
1.8.3.1

[PATCH 2/2] cpufreq, intel_pstate, fix limits->max_perf rounding error

[Prarit Bhargava]

A rounding error was found in the calculation of limits->max_perf
in intel_pstate_set_policy(), which is used to calculate the max and min
pstate values in intel_pstate_get_min_max().  In that code,
limits->max_perf is truncated to 2 hex digits such that, for example,
0x169 was incorrectly calculated to 0x16 instead of 0x17.  This resulted in
the pstate being set one level too low.  This patch rounds the value of
limits->max_perf up instead of down so that the correct max pstate can
be reached.

Cc: Srinivas Pandruvada <srinivas.pandruvada@intel.com>
Cc: Len Brown <len.brown@intel.com>
Cc: Alexandra Yates <alexandra.yates@intel.com>
Cc: Kristen Carlson Accardi <kristen@linux.intel.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Prarit Bhargava <prarit@redhat.com>
---
 drivers/cpufreq/intel_pstate.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 6b63374..8b8e331 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -1122,6 +1122,7 @@ static int intel_pstate_set_policy(struct cpufreq_policy *policy)
 				   limits->max_sysfs_pct);
 	limits->max_perf_pct = max(limits->min_policy_pct,
 				   limits->max_perf_pct);
+	limits->max_perf = round_up(limits->max_perf, 8);
 
 	/* Make sure min_perf_pct <= max_perf_pct */
 	limits->min_perf_pct = min(limits->max_perf_pct, limits->min_perf_pct);
-- 
1.8.3.1

Re: [PATCH 1/2] cpufreq, intel_pstate, Fix limits->max_policy_pct rounding error

[Viresh Kumar]

On 20-11-15, 07:32, Prarit Bhargava wrote:
> I have a Intel (6,63) processor with a "marketing" frequency (from
> /proc/cpuinfo) of 2100MHz, and a max turbo frequency of 2600MHz.  I
> can execute
> 
> cpupower frequency-set -g powersave --min 1200MHz --max 2100MHz
> 
> and the max_freq_pct is set to 80.  When adding load to the system I noticed
> that the cpu frequency only reached 2000MHZ and not 2100MHz as expected.
> 
> This is because limits->max_policy_pct is calculated as 2100 * 100 /2600 = 80.7
> and is rounded down to 80 when it should be rounded up to 81.  This patch
> adds a DIV_ROUND_UP() which will return the correct value.
> 
> Cc: Srinivas Pandruvada <srinivas.pandruvada@intel.com>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Alexandra Yates <alexandra.yates@intel.com>
> Cc: Kristen Carlson Accardi <kristen@linux.intel.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
> ---
>  drivers/cpufreq/intel_pstate.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index 001a532..6b63374 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -1110,6 +1110,8 @@ static int intel_pstate_set_policy(struct cpufreq_policy *policy)
>  	limits->min_policy_pct = clamp_t(int, limits->min_policy_pct, 0 , 100);
>  	limits->max_policy_pct = (policy->max * 100) / policy->cpuinfo.max_freq;

Forgot to remove this line ?

>  	limits->max_policy_pct = clamp_t(int, limits->max_policy_pct, 0 , 100);

And put this after the later one ?

> +	limits->max_policy_pct = DIV_ROUND_UP(policy->max * 100,
> +					      policy->cpuinfo.max_freq);
>  
>  	/* Normalize user input to [min_policy_pct, max_policy_pct] */
>  	limits->min_perf_pct = max(limits->min_policy_pct,

Sure you tested it  ? :)

-- 
viresh

Re: [PATCH 1/2] cpufreq, intel_pstate, Fix limits->max_policy_pct rounding error

[Prarit Bhargava]

On 11/20/2015 08:18 AM, Viresh Kumar wrote:
> On 20-11-15, 07:32, Prarit Bhargava wrote:
>> I have a Intel (6,63) processor with a "marketing" frequency (from
>> /proc/cpuinfo) of 2100MHz, and a max turbo frequency of 2600MHz.  I
>> can execute
>>
>> cpupower frequency-set -g powersave --min 1200MHz --max 2100MHz
>>
>> and the max_freq_pct is set to 80.  When adding load to the system I noticed
>> that the cpu frequency only reached 2000MHZ and not 2100MHz as expected.
>>
>> This is because limits->max_policy_pct is calculated as 2100 * 100 /2600 = 80.7
>> and is rounded down to 80 when it should be rounded up to 81.  This patch
>> adds a DIV_ROUND_UP() which will return the correct value.
>>
>> Cc: Srinivas Pandruvada <srinivas.pandruvada@intel.com>
>> Cc: Len Brown <len.brown@intel.com>
>> Cc: Alexandra Yates <alexandra.yates@intel.com>
>> Cc: Kristen Carlson Accardi <kristen@linux.intel.com>
>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>> Cc: Viresh Kumar <viresh.kumar@linaro.org>
>> Cc: linux-pm@vger.kernel.org
>> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
>> ---
>>  drivers/cpufreq/intel_pstate.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
>> index 001a532..6b63374 100644
>> --- a/drivers/cpufreq/intel_pstate.c
>> +++ b/drivers/cpufreq/intel_pstate.c
>> @@ -1110,6 +1110,8 @@ static int intel_pstate_set_policy(struct cpufreq_policy *policy)
>>  	limits->min_policy_pct = clamp_t(int, limits->min_policy_pct, 0 , 100);
>>  	limits->max_policy_pct = (policy->max * 100) / policy->cpuinfo.max_freq;
> 
> Forgot to remove this line ?
> 
>>  	limits->max_policy_pct = clamp_t(int, limits->max_policy_pct, 0 , 100);
> 
> And put this after the later one ?
> 
>> +	limits->max_policy_pct = DIV_ROUND_UP(policy->max * 100,
>> +					      policy->cpuinfo.max_freq);
>>  
>>  	/* Normalize user input to [min_policy_pct, max_policy_pct] */
>>  	limits->min_perf_pct = max(limits->min_policy_pct,
> 
> Sure you tested it  ? :)

Oops -- and yeah, tested.  It works because I rewrite the value of
max_policy_pct :).  I'll repost shortly.

P.

> 

Re: [PATCH 1/2] cpufreq, intel_pstate, Fix limits->max_policy_pct rounding error

[Viresh Kumar]

On 20-11-15, 10:10, Prarit Bhargava wrote:
> >>  	limits->max_policy_pct = clamp_t(int, limits->max_policy_pct, 0 , 100);
> > 
> > And put this after the later one ?
> > 
> >> +	limits->max_policy_pct = DIV_ROUND_UP(policy->max * 100,
> >> +					      policy->cpuinfo.max_freq);
> >>  
> >>  	/* Normalize user input to [min_policy_pct, max_policy_pct] */
> >>  	limits->min_perf_pct = max(limits->min_policy_pct,
> > 
> > Sure you tested it  ? :)
> 
> Oops -- and yeah, tested.  It works because I rewrite the value of
> max_policy_pct :).  I'll repost shortly.

But we aren't doing below anymore, doesn't this change the
calculations at all?

  	limits->max_policy_pct = clamp_t(int, limits->max_policy_pct, 0 , 100);

-- 
viresh

Re: [PATCH 1/2] cpufreq, intel_pstate, Fix limits->max_policy_pct rounding error

[Prarit Bhargava]

On 11/20/2015 10:19 AM, Viresh Kumar wrote:
> On 20-11-15, 10:10, Prarit Bhargava wrote:
>>>>  	limits->max_policy_pct = clamp_t(int, limits->max_policy_pct, 0 , 100);
>>>
>>> And put this after the later one ?
>>>
>>>> +	limits->max_policy_pct = DIV_ROUND_UP(policy->max * 100,
>>>> +					      policy->cpuinfo.max_freq);
>>>>  
>>>>  	/* Normalize user input to [min_policy_pct, max_policy_pct] */
>>>>  	limits->min_perf_pct = max(limits->min_policy_pct,
>>>
>>> Sure you tested it  ? :)
>>
>> Oops -- and yeah, tested.  It works because I rewrite the value of
>> max_policy_pct :).  I'll repost shortly.
> 
> But we aren't doing below anymore, doesn't this change the
> calculations at all?
> 
>   	limits->max_policy_pct = clamp_t(int, limits->max_policy_pct, 0 , 100);

The clamp only confines the max_policy between 0 and 100.  AFAIK it doesn't make
any change tothe value of limits->max_policy_pct unless it was outside of that
range.

P.
> 

Re: [PATCH 1/2] cpufreq, intel_pstate, Fix limits->max_policy_pct rounding error

[Pandruvada, Srinivas]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2840 bytes --]

On Fri, 2015-11-20 at 10:43 -0500, Prarit Bhargava wrote:
> 
> On 11/20/2015 10:19 AM, Viresh Kumar wrote:
> > On 20-11-15, 10:10, Prarit Bhargava wrote:
> >>>>  	limits->max_policy_pct = clamp_t(int, limits->max_policy_pct, 0 , 100);
> >>>
> >>> And put this after the later one ?
> >>>
> >>>> +	limits->max_policy_pct = DIV_ROUND_UP(policy->max * 100,
> >>>> +					      policy->cpuinfo.max_freq);
> >>>>  
> >>>>  	/* Normalize user input to [min_policy_pct, max_policy_pct] */
> >>>>  	limits->min_perf_pct = max(limits->min_policy_pct,
> >>>
> >>> Sure you tested it  ? :)
> >>
> >> Oops -- and yeah, tested.  It works because I rewrite the value of
> >> max_policy_pct :).  I'll repost shortly.
> > 
> > But we aren't doing below anymore, doesn't this change the
> > calculations at all?
> > 
> >   	limits->max_policy_pct = clamp_t(int, limits->max_policy_pct, 0 , 100);
> 
> The clamp only confines the max_policy between 0 and 100.  AFAIK it doesn't make
> any change tothe value of limits->max_policy_pct unless it was outside of that
> range.
> 
> P.
> > 

With the changes below (as suggested above), I did tests. Except two
cases, it did correct. Those two are in turbo range, so I am OK with
that. 


diff --git a/drivers/cpufreq/intel_pstate.c
b/drivers/cpufreq/intel_pstate.c
index b78abe9..c3bcca4 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -1111,9 +1111,9 @@ static int intel_pstate_set_policy(struct
cpufreq_policy *policy)
 	limits = &powersave_limits;
 	limits->min_policy_pct = (policy->min * 100) /
policy->cpuinfo.max_freq;
 	limits->min_policy_pct = clamp_t(int, limits->min_policy_pct, 0 ,
100);
-	limits->max_policy_pct = (policy->max * 100) /
policy->cpuinfo.max_freq;
+	limits->max_policy_pct = DIV_ROUND_UP(policy->max * 100,
+                                             policy->cpuinfo.max_freq);
 	limits->max_policy_pct = clamp_t(int, limits->max_policy_pct, 0 ,
100);
-
 	/* Normalize user input to [min_policy_pct, max_policy_pct] */
 	limits->min_perf_pct = max(limits->min_policy_pct,
 				   limits->min_sysfs_pct);
@@ -1131,7 +1131,7 @@ static int intel_pstate_set_policy(struct
cpufreq_policy *policy)
 				  int_tofp(100));
 	limits->max_perf = div_fp(int_tofp(limits->max_perf_pct),
 				  int_tofp(100));
-
+	limits->max_perf = round_up(limits->max_perf, 8);
 	if (hwp_active)
 		intel_pstate_hwp_set();


3300 : OK
3200 : 1 less
3100 : 1 less
3000 : 1 less
2900 : OK
2800 : OK
2700 : OK
2600 : OK
2500 : OK
2400 : OK
2300 : OK
2200 : OK
2100 : OK
2000 : OK
1900 : OK
1800 : OK
1700 : OK
1600 : OK
1500 : OK
1400 : OK
1300 : OK
1200 : OK
1100 : OK
1000 : OK
900  : OK
800 : OK

Thanks,
Srinivas
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 1/2] cpufreq, intel_pstate, Fix limits->max_policy_pct rounding error

[Pandruvada, Srinivas]

On Fri, 2015-11-20 at 10:43 -0500, Prarit Bhargava wrote:
> 
> On 11/20/2015 10:19 AM, Viresh Kumar wrote:
> > On 20-11-15, 10:10, Prarit Bhargava wrote:
> >>>>  	limits->max_policy_pct = clamp_t(int, limits->max_policy_pct, 0 , 100);
> >>>
> >>> And put this after the later one ?
> >>>
> >>>> +	limits->max_policy_pct = DIV_ROUND_UP(policy->max * 100,
> >>>> +					      policy->cpuinfo.max_freq);
> >>>>  
> >>>>  	/* Normalize user input to [min_policy_pct, max_policy_pct] */
> >>>>  	limits->min_perf_pct = max(limits->min_policy_pct,
> >>>
> >>> Sure you tested it  ? :)
> >>
> >> Oops -- and yeah, tested.  It works because I rewrite the value of
> >> max_policy_pct :).  I'll repost shortly.
> > 
> > But we aren't doing below anymore, doesn't this change the
> > calculations at all?
> > 
> >   	limits->max_policy_pct = clamp_t(int, limits->max_policy_pct, 0 , 100);
> 
> The clamp only confines the max_policy between 0 and 100.  AFAIK it doesn't make
> any change tothe value of limits->max_policy_pct unless it was outside of that
> range.
> 
> P.
> > 

With the changes below (as suggested above), I did tests. Except two
cases, it did correct. Those two are in turbo range, so I am OK with
that. 


diff --git a/drivers/cpufreq/intel_pstate.c
b/drivers/cpufreq/intel_pstate.c
index b78abe9..c3bcca4 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -1111,9 +1111,9 @@ static int intel_pstate_set_policy(struct
cpufreq_policy *policy)
 	limits = &powersave_limits;
 	limits->min_policy_pct = (policy->min * 100) /
policy->cpuinfo.max_freq;
 	limits->min_policy_pct = clamp_t(int, limits->min_policy_pct, 0 ,
100);
-	limits->max_policy_pct = (policy->max * 100) /
policy->cpuinfo.max_freq;
+	limits->max_policy_pct = DIV_ROUND_UP(policy->max * 100,
+                                             policy->cpuinfo.max_freq);
 	limits->max_policy_pct = clamp_t(int, limits->max_policy_pct, 0 ,
100);
-
 	/* Normalize user input to [min_policy_pct, max_policy_pct] */
 	limits->min_perf_pct = max(limits->min_policy_pct,
 				   limits->min_sysfs_pct);
@@ -1131,7 +1131,7 @@ static int intel_pstate_set_policy(struct
cpufreq_policy *policy)
 				  int_tofp(100));
 	limits->max_perf = div_fp(int_tofp(limits->max_perf_pct),
 				  int_tofp(100));
-
+	limits->max_perf = round_up(limits->max_perf, 8);
 	if (hwp_active)
 		intel_pstate_hwp_set();


3300 : OK
3200 : 1 less
3100 : 1 less
3000 : 1 less
2900 : OK
2800 : OK
2700 : OK
2600 : OK
2500 : OK
2400 : OK
2300 : OK
2200 : OK
2100 : OK
2000 : OK
1900 : OK
1800 : OK
1700 : OK
1600 : OK
1500 : OK
1400 : OK
1300 : OK
1200 : OK
1100 : OK
1000 : OK
900  : OK
800 : OK

Thanks,
Srinivas

Re: [PATCH 1/2] cpufreq, intel_pstate, Fix limits->max_policy_pct rounding error

[Prarit Bhargava]

On 11/20/2015 03:02 PM, Pandruvada, Srinivas wrote:
> On Fri, 2015-11-20 at 10:43 -0500, Prarit Bhargava wrote:
>>
>> On 11/20/2015 10:19 AM, Viresh Kumar wrote:
>>> On 20-11-15, 10:10, Prarit Bhargava wrote:
>>>>>>  	limits->max_policy_pct = clamp_t(int, limits->max_policy_pct, 0 , 100);
>>>>>
>>>>> And put this after the later one ?
>>>>>
>>>>>> +	limits->max_policy_pct = DIV_ROUND_UP(policy->max * 100,
>>>>>> +					      policy->cpuinfo.max_freq);
>>>>>>  
>>>>>>  	/* Normalize user input to [min_policy_pct, max_policy_pct] */
>>>>>>  	limits->min_perf_pct = max(limits->min_policy_pct,
>>>>>
>>>>> Sure you tested it  ? :)
>>>>
>>>> Oops -- and yeah, tested.  It works because I rewrite the value of
>>>> max_policy_pct :).  I'll repost shortly.
>>>
>>> But we aren't doing below anymore, doesn't this change the
>>> calculations at all?
>>>
>>>   	limits->max_policy_pct = clamp_t(int, limits->max_policy_pct, 0 , 100);
>>
>> The clamp only confines the max_policy between 0 and 100.  AFAIK it doesn't make
>> any change tothe value of limits->max_policy_pct unless it was outside of that
>> range.
>>
>> P.
>>>
> 
> With the changes below (as suggested above), I did tests. Except two
> cases, it did correct. Those two are in turbo range, so I am OK with
> that. 
> 
> 
> diff --git a/drivers/cpufreq/intel_pstate.c
> b/drivers/cpufreq/intel_pstate.c
> index b78abe9..c3bcca4 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -1111,9 +1111,9 @@ static int intel_pstate_set_policy(struct
> cpufreq_policy *policy)
>  	limits = &powersave_limits;
>  	limits->min_policy_pct = (policy->min * 100) /
> policy->cpuinfo.max_freq;
>  	limits->min_policy_pct = clamp_t(int, limits->min_policy_pct, 0 ,
> 100);
> -	limits->max_policy_pct = (policy->max * 100) /
> policy->cpuinfo.max_freq;
> +	limits->max_policy_pct = DIV_ROUND_UP(policy->max * 100,
> +                                             policy->cpuinfo.max_freq);
>  	limits->max_policy_pct = clamp_t(int, limits->max_policy_pct, 0 ,
> 100);
> -
>  	/* Normalize user input to [min_policy_pct, max_policy_pct] */
>  	limits->min_perf_pct = max(limits->min_policy_pct,
>  				   limits->min_sysfs_pct);
> @@ -1131,7 +1131,7 @@ static int intel_pstate_set_policy(struct
> cpufreq_policy *policy)
>  				  int_tofp(100));
>  	limits->max_perf = div_fp(int_tofp(limits->max_perf_pct),
>  				  int_tofp(100));
> -
> +	limits->max_perf = round_up(limits->max_perf, 8);
>  	if (hwp_active)
>  		intel_pstate_hwp_set();
> 
> 
> 3300 : OK
> 3200 : 1 less
> 3100 : 1 less
> 3000 : 1 less

The -1 difference here is not unexpected given the other probable rounding
errors in the frequency code.  I have a feeling that no one really has done an
in depth review to find the errors.  I'm not going to because I'm pretty sure
I/we can convince users that 3200 == 3199.98 ;).  FWIW, I've also wondered if
the difference between the marketing frequency and the TSC frequency (which in
theory equals the marketing frequency) can cause this sort of error.

OOC did you try loading the system down (with a kernel build) and switching
between frequencies?  That's a good way to see the effect of the turbo states.
I would expect that the turbo state hits a maximum of about 75% of the max turbo
state value (based on experiment) so the differences should be larger at the
high end.

P.

Re: [PATCH 1/2] cpufreq, intel_pstate, Fix limits->max_policy_pct rounding error

[Pandruvada, Srinivas]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1435 bytes --]

On Fri, 2015-11-20 at 18:47 -0500, Prarit Bhargava wrote:
> 
[cut]
> The -1 difference here is not unexpected given the other probable rounding
> errors in the frequency code.
Yes. Intel P state cpufreq interface is not optimal. We even debate
whether we should have this interface at all.

>   I have a feeling that no one really has done an
> in depth review to find the errors.  I'm not going to because I'm pretty sure
> I/we can convince users that 3200 == 3199.98 ;).  FWIW, I've also wondered if
> the difference between the marketing frequency and the TSC frequency (which in
> theory equals the marketing frequency) can cause this sort of error.
We don't even request correct pstate here, so we will not get that. But
in this case in turbo region is not controllable (after Sandybridge )
above something called turbo activation ratio. So not a big deal.
As long as we can set at lower end we are fine.
> 

Thanks,
Srinivas

> OOC did you try loading the system down (with a kernel build) and switching
> between frequencies?  That's a good way to see the effect of the turbo states.
> I would expect that the turbo state hits a maximum of about 75% of the max turbo
> state value (based on experiment) so the differences should be larger at the
> high end.
> 
> P.

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 1/2] cpufreq, intel_pstate, Fix limits->max_policy_pct rounding error

[Pandruvada, Srinivas]

On Fri, 2015-11-20 at 18:47 -0500, Prarit Bhargava wrote:
> 
[cut]
> The -1 difference here is not unexpected given the other probable rounding
> errors in the frequency code.
Yes. Intel P state cpufreq interface is not optimal. We even debate
whether we should have this interface at all.

>   I have a feeling that no one really has done an
> in depth review to find the errors.  I'm not going to because I'm pretty sure
> I/we can convince users that 3200 == 3199.98 ;).  FWIW, I've also wondered if
> the difference between the marketing frequency and the TSC frequency (which in
> theory equals the marketing frequency) can cause this sort of error.
We don't even request correct pstate here, so we will not get that. But
in this case in turbo region is not controllable (after Sandybridge )
above something called turbo activation ratio. So not a big deal.
As long as we can set at lower end we are fine.
> 

Thanks,
Srinivas

> OOC did you try loading the system down (with a kernel build) and switching
> between frequencies?  That's a good way to see the effect of the turbo states.
> I would expect that the turbo state hits a maximum of about 75% of the max turbo
> state value (based on experiment) so the differences should be larger at the
> high end.
> 
> P.