linux-kernel - Re: [PATCH 1/2] cpufreq, intel_pstate, Fix limits->max_policy

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]

Message-ID: <564FB111.5050403@redhat.com>
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/