lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Fri, 20 Nov 2015 18:47:29 -0500
From:	Prarit Bhargava <prarit@...hat.com>
To:	"Pandruvada, Srinivas" <srinivas.pandruvada@...el.com>
CC:	"Brown, Len" <len.brown@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"viresh.kumar@...aro.org" <viresh.kumar@...aro.org>,
	"kristen@...ux.intel.com" <kristen@...ux.intel.com>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	"rjw@...ysocki.net" <rjw@...ysocki.net>,
	"Yates, Alexandra" <alexandra.yates@...el.com>
Subject: Re: [PATCH 1/2] cpufreq, intel_pstate, Fix limits->max_policy_pct
 rounding error



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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists