Re: [PATCH] cpufreq: intel_pstate: Fix rounding of core_pct

[Stratos Karafotis <stratosk@semaphore.gr>]

On 12/06/2014 12:15 πμ, Doug Smythies wrote:
> 
> 
> -----Original Message-----
> From: Stratos Karafotis [mailto:stratosk@semaphore.gr] 
> Sent: June-11-2014 13:20
> To: Doug Smythies
> Cc: linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org; rjw@rjwysocki.net; viresh.kumar@linaro.org; dirk.j.brandewie@intel.com
> Subject: Re: [PATCH] cpufreq: intel_pstate: Fix rounding of core_pct
> 
> On 2014.06.11 13:20 Stratos Karafotis wrote:
>> On 11/06/2014 06:02 μμ, Doug Smythies wrote:
>>>
>>> On 2104.06.11 07:08 Stratos Karafotis wrote:
>>>> On 11/06/2014 04:41 μμ, Doug Smythies wrote:
>>>>
>>>> No.
>>>
>>>> The intent was only ever to round properly the pseudo floating point result of the divide.
>>>> It was much more important (ugh, well 4 times more) when FRACBITS was still 6, which also got changed to 8 in a recent patch.
>>>>
>>>
>>> Are you sure?
>>>
>>> Yes.
>>>
>>>> This rounding was very recently added.
>>>> As far as I can understand, I don't see the meaning of this rounding, as is.
>>>> Even if FRAC_BITS was 6, I think it would have almost no improvement in
>>>> calculations.
>>>
>>> Note: I had not seen this e-mail when I wrote a few minutes ago:
>>>
>>> You may be correct.
>>> If Dirk agrees, I will re-analyse the entire driver for rounding effects soon.
>>> When FRACBITS was 6 there were subtle cases where the driver would get stuck, and not make a final pstate change, with the default PID gains.
>>> Other things have changed, and the analysis needs to be re-done.
>>>
> 
>> Could you please elaborate a little bit more what we need these 2 lines below?
>>
>>        if ((rem << 1) >= int_tofp(sample->mperf))
>>                core_pct += 1;
>>
>> Because nothing is mentioned for them in commit's changelog.
>> Do we need to round core_pct or not?
>> Because if we try to round it, I think this patch should work.
> 
> As mentioned originally, they are there just to round the pseudo floating number, not the integer portion only.
> Let us bring back the very numbers you originally gave and work through it.
> 
> aperf = 5024
> mperf = 10619
> 
> core_pct = 47.31142292%
> or 47 and 79.724267 256ths
> or to the closest kept fractional part 47 and 80 256ths
> or 12112 as a pseudo float.
> The maximum error with this rounding will be 1 part in 512 and symmetric instead of the 1 part in 256 always in one direction without.
> 
> Now if FRACBITS was still 6:
> core_pct = 47.31142292%
> or 47 and 19.931 64ths
> or to the closest kept fractional part 47 and 20 64ths
> or 3028 as a pseudo float.
> The maximum error with this rounding will be 1 part in 128 and symmetric instead of the 1 part in 64 (1.6% !!!) always in one direction without.
> 
> Hope this helps.
> 

Yes, it helps. Thanks a lot!

But please note that the maximum error without this rounding will be 1.6% _only_
in fractional part. In the real number it will be much smaller:

47.19 instead of 47.20

And using FRAC_BITS 8:

47.79 instead of 47.80

This is a 0.0002% difference. I can't see how this is can affect the calculations
even with FRAC_BITS 6.

Another thing is that this algorithm generally is used to round to the
nearest integer. I'm not sure if it's valid to apply it for the rounding of
the fractional part of fixed point number.

Please see below the algorithm (with numbers of the specific example presented
as real):

aperf = 5024
mperf = 10619

aperf * 100 / mperf = 47.31142292

core_pct = 47
rem = 3307

if (3307 * 2) >= 10619
	core_pct = core_pct + 0.004

The original algorithm adds 1 to the quotient to round the integer part of the
division. In the specific example we add 0.004 to round the fractional part.

Fortunately, as you mentioned, this does not change the final calculation
considering that we do _not_ want an integer rounding.

IMHO, If we need integer rounding we also need this patch, otherwise
we can safely remove this rounding.


Thanks,
Stratos