[PATCH] cpufreq: intel_pstate: Fix rounding of core_pct

[PATCH] cpufreq: intel_pstate: Fix rounding of core_pct

[Stratos Karafotis]

Local variable core_pct holds fixed point values.
When we round it we add "1" to core_pct. This has almost
no effect.

So, add int_toftp(1) to core_pct when rounding.

For example, in a given sample point (values taken from
tracepoint) with:
aperf = 5024
mperf = 10619

the core_pct is (before rounding):
core_pct = 12111
fp_toint(core_pct) = 47

After rounding:
core_pct = 12112
fp_toint(core_pct) = 47

After rounding with int_toftp(1):
core_pct = 12367
fp_toint(core_pct) = 48

Signed-off-by: Stratos Karafotis <stratosk@semaphore.gr>
---

Hi Rafael,

I'm sorry for submitting again in merge window, but
I thought that maybe we need this fix for 3.16.


 drivers/cpufreq/intel_pstate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 4e7f492..dd80aa2 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -564,7 +564,7 @@ static inline void intel_pstate_calc_busy(struct cpudata *cpu)
 	core_pct = div_u64_rem(core_pct, int_tofp(sample->mperf), &rem);
 
 	if ((rem << 1) >= int_tofp(sample->mperf))
-		core_pct += 1;
+		core_pct += int_tofp(1);
 
 	sample->freq = fp_toint(
 		mul_fp(int_tofp(cpu->pstate.max_pstate * 1000), core_pct));
-- 
1.9.3

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

[Doug Smythies]

On 2014.06.11 05:34 Stratos Karafotis wrote:

> Local variable core_pct holds fixed point values.
> When we round it we add "1" to core_pct. This has almost
> no effect.
>
> So, add int_toftp(1) to core_pct when rounding.
>
> For example, in a given sample point (values taken from
> tracepoint) with:
> aperf = 5024
> mperf = 10619
>
> the core_pct is (before rounding):
> core_pct = 12111
> fp_toint(core_pct) = 47
>
> After rounding:
> core_pct = 12112
> fp_toint(core_pct) = 47
>
> After rounding with int_toftp(1):
> core_pct = 12367
> fp_toint(core_pct) = 48
>
> Signed-off-by: Stratos Karafotis <stratosk@semaphore.gr>
> ---
>
> Hi Rafael,
>
> I'm sorry for submitting again in merge window, but
> I thought that maybe we need this fix for 3.16.
>
>
> drivers/cpufreq/intel_pstate.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index 4e7f492..dd80aa2 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -564,7 +564,7 @@ static inline void intel_pstate_calc_busy(struct cpudata *cpu)
> 	core_pct = div_u64_rem(core_pct, int_tofp(sample->mperf), &rem);
> 
> 	if ((rem << 1) >= int_tofp(sample->mperf))
> -		core_pct += 1;
> +		core_pct += int_tofp(1);
> 
> 	sample->freq = fp_toint(
> 		mul_fp(int_tofp(cpu->pstate.max_pstate * 1000), core_pct));
> -- 
> 1.9.3

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.

... Doug

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

[Doug Smythies]

On 2014.06.11 05:34 Stratos Karafotis wrote:

> Local variable core_pct holds fixed point values.
> When we round it we add "1" to core_pct. This has almost
> no effect.
>
> So, add int_toftp(1) to core_pct when rounding.
>
> For example, in a given sample point (values taken from
> tracepoint) with:
> aperf = 5024
> mperf = 10619
>
> the core_pct is (before rounding):
> core_pct = 12111
> fp_toint(core_pct) = 47
>
> After rounding:
> core_pct = 12112
> fp_toint(core_pct) = 47
>
> After rounding with int_toftp(1):
> core_pct = 12367
> fp_toint(core_pct) = 48
>
> Signed-off-by: Stratos Karafotis <stratosk@semaphore.gr>
> ---
>
> Hi Rafael,
>
> I'm sorry for submitting again in merge window, but
> I thought that maybe we need this fix for 3.16.
>
>
> drivers/cpufreq/intel_pstate.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index 4e7f492..dd80aa2 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -564,7 +564,7 @@ static inline void intel_pstate_calc_busy(struct cpudata *cpu)
> 	core_pct = div_u64_rem(core_pct, int_tofp(sample->mperf), &rem);
> 
> 	if ((rem << 1) >= int_tofp(sample->mperf))
> -		core_pct += 1;
> +		core_pct += int_tofp(1);
> 
> 	sample->freq = fp_toint(
> 		mul_fp(int_tofp(cpu->pstate.max_pstate * 1000), core_pct));
> -- 
> 1.9.3

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.

... Doug

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

[Stratos Karafotis]

On 11/06/2014 04:41 μμ, Doug Smythies wrote:
> 
> On 2014.06.11 05:34 Stratos Karafotis wrote:
> 
>> Local variable core_pct holds fixed point values.
>> When we round it we add "1" to core_pct. This has almost
>> no effect.
>>
>> So, add int_toftp(1) to core_pct when rounding.
>>
>> For example, in a given sample point (values taken from
>> tracepoint) with:
>> aperf = 5024
>> mperf = 10619
>>
>> the core_pct is (before rounding):
>> core_pct = 12111
>> fp_toint(core_pct) = 47
>>
>> After rounding:
>> core_pct = 12112
>> fp_toint(core_pct) = 47
>>
>> After rounding with int_toftp(1):
>> core_pct = 12367
>> fp_toint(core_pct) = 48
>>
>> Signed-off-by: Stratos Karafotis <stratosk@semaphore.gr>
>> ---
>>
>> Hi Rafael,
>>
>> I'm sorry for submitting again in merge window, but
>> I thought that maybe we need this fix for 3.16.
>>
>>
>> drivers/cpufreq/intel_pstate.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
>> index 4e7f492..dd80aa2 100644
>> --- a/drivers/cpufreq/intel_pstate.c
>> +++ b/drivers/cpufreq/intel_pstate.c
>> @@ -564,7 +564,7 @@ static inline void intel_pstate_calc_busy(struct cpudata *cpu)
>> 	core_pct = div_u64_rem(core_pct, int_tofp(sample->mperf), &rem);
>>
>> 	if ((rem << 1) >= int_tofp(sample->mperf))
>> -		core_pct += 1;
>> +		core_pct += int_tofp(1);
>>
>> 	sample->freq = fp_toint(
>> 		mul_fp(int_tofp(cpu->pstate.max_pstate * 1000), core_pct));
>> -- 
>> 1.9.3
> 
> 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? 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.


Stratos

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

[Doug Smythies]

On 2014.06.11 06:42 Doug Smythies wrote:
On 2014.06.11 05:34 Stratos Karafotis wrote:

>> 	if ((rem << 1) >= int_tofp(sample->mperf))
>> -		core_pct += 1;
>> +		core_pct += int_tofp(1);
>> 
>> 	sample->freq = fp_toint(
>> 		mul_fp(int_tofp(cpu->pstate.max_pstate * 1000), core_pct));
>> -- 
>> 1.9.3

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

I forgot to mention there are other related roundings that are being considered.
I do not recall clearly, but I think Dirk and I agreed to hold off until
the recent panics had settled.

The analysis as to the importance needs to be re-done, as it was all done when FRACBITS was 6. Things were very "chunky" when
FRACBITS was 6.

These are what I was considering putting forward:

static inline int32_t fp_toint(int32_t x)
{
        if (x >= 0)
                x +=  (1 << (FRAC_BITS -1));
         else
                x -=  (1 << (FRAC_BITS -1));
        return (x >> FRAC_BITS);
}

static inline int32_t mul_fp(int32_t x, int32_t y)
{
        int64_t temp;
        temp = (int64_t)x * (int64_t)y;
        if (temp >= 0)
                temp +=  (1 << (FRAC_BITS -1));
         else
                temp -=  (1 << (FRAC_BITS -1));
        return (temp >> FRAC_BITS);
}

static inline int32_t div_fp(int32_t x, int32_t y)
{

        /* currently, there are only positive numbers to worry about here */

        int32_t rem;

        x = div_s64_rem((int64_t)x << FRAC_BITS, (int64_t)y, &rem);
        if((rem << 1) >= y) x++;
        return(x);
}

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

[Doug Smythies]

On 2014.06.11 06:42 Doug Smythies wrote:
On 2014.06.11 05:34 Stratos Karafotis wrote:

>> 	if ((rem << 1) >= int_tofp(sample->mperf))
>> -		core_pct += 1;
>> +		core_pct += int_tofp(1);
>> 
>> 	sample->freq = fp_toint(
>> 		mul_fp(int_tofp(cpu->pstate.max_pstate * 1000), core_pct));
>> -- 
>> 1.9.3

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

I forgot to mention there are other related roundings that are being considered.
I do not recall clearly, but I think Dirk and I agreed to hold off until
the recent panics had settled.

The analysis as to the importance needs to be re-done, as it was all done when FRACBITS was 6. Things were very "chunky" when
FRACBITS was 6.

These are what I was considering putting forward:

static inline int32_t fp_toint(int32_t x)
{
        if (x >= 0)
                x +=  (1 << (FRAC_BITS -1));
         else
                x -=  (1 << (FRAC_BITS -1));
        return (x >> FRAC_BITS);
}

static inline int32_t mul_fp(int32_t x, int32_t y)
{
        int64_t temp;
        temp = (int64_t)x * (int64_t)y;
        if (temp >= 0)
                temp +=  (1 << (FRAC_BITS -1));
         else
                temp -=  (1 << (FRAC_BITS -1));
        return (temp >> FRAC_BITS);
}

static inline int32_t div_fp(int32_t x, int32_t y)
{

        /* currently, there are only positive numbers to worry about here */

        int32_t rem;

        x = div_s64_rem((int64_t)x << FRAC_BITS, (int64_t)y, &rem);
        if((rem << 1) >= y) x++;
        return(x);
}

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

[Doug Smythies]

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.

... Doug

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

[Doug Smythies]

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.

... Doug

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

[Rafael J. Wysocki]

On Wed, Jun 11, 2014 at 5:02 PM, Doug Smythies <dsmythies@telus.net> 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.

Well, can you please do it if that's not a problem?

Rafael

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

[Stratos Karafotis]

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.

Thanks,
Stratos

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

[Doug Smythies]

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

... Doug

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

[Doug Smythies]

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

... Doug

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

[Doug Smythies]

-----Original Message-----
From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf Of Rafael J. Wysocki
Sent: June-11-2014 11:29
To: Doug Smythies
Cc: Stratos Karafotis; linux-pm@vger.kernel.org; Linux Kernel Mailing List; Rafael J. Wysocki; viresh.kumar@linaro.org; dirk.j.brandewie@intel.com
Subject: Re: [PATCH] cpufreq: intel_pstate: Fix rounding of core_pct

On 2014.06.11 11:29 Rafael J. Wysocki wrote:
> On Wed, Jun 11, 2014 at 5:02 PM, Doug Smythies <dsmythies@telus.net> 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.

> Well, can you please do it if that's not a problem?

Yes, of course, but it is a matter of priorities. All I meant by the above comment was that we might be able to forget about the remainder and just do a mindless divide, but leaving it as it is now does no harm in the meantime.

Myself, I consider the issue of excessive deferred timer times to be a much higher priority (see my on-list e-mail from Monday). Correct me if I am wrong.
Even without the "excessive" part, and for a 250 Hz kernel, the current kick in point can be hit routinely, unduly biasing the CPU frequency downwards.
A random example (250 Hz kernel): 23% load at 25 Hertz load / sleep frequency for 300 total seconds.

Duration histrogram:

Occurrences duration (seconds)
     16 0.044
     39 0.024
     45 0.028
     46 0.016
     48 0.032
     61 0.036
    166 0.012
    198 0.020
   7166 0.040

Where you can see that the majority of the time the duration is such that the code will force the CPU frequency downwards. It runs at minimum pstate instead of maximum pstate where it should be.

... Doug

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

[Doug Smythies]

-----Original Message-----
From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf Of Rafael J. Wysocki
Sent: June-11-2014 11:29
To: Doug Smythies
Cc: Stratos Karafotis; linux-pm@vger.kernel.org; Linux Kernel Mailing List; Rafael J. Wysocki; viresh.kumar@linaro.org; dirk.j.brandewie@intel.com
Subject: Re: [PATCH] cpufreq: intel_pstate: Fix rounding of core_pct

On 2014.06.11 11:29 Rafael J. Wysocki wrote:
> On Wed, Jun 11, 2014 at 5:02 PM, Doug Smythies <dsmythies@telus.net> 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.

> Well, can you please do it if that's not a problem?

Yes, of course, but it is a matter of priorities. All I meant by the above comment was that we might be able to forget about the remainder and just do a mindless divide, but leaving it as it is now does no harm in the meantime.

Myself, I consider the issue of excessive deferred timer times to be a much higher priority (see my on-list e-mail from Monday). Correct me if I am wrong.
Even without the "excessive" part, and for a 250 Hz kernel, the current kick in point can be hit routinely, unduly biasing the CPU frequency downwards.
A random example (250 Hz kernel): 23% load at 25 Hertz load / sleep frequency for 300 total seconds.

Duration histrogram:

Occurrences duration (seconds)
     16 0.044
     39 0.024
     45 0.028
     46 0.016
     48 0.032
     61 0.036
    166 0.012
    198 0.020
   7166 0.040

Where you can see that the majority of the time the duration is such that the code will force the CPU frequency downwards. It runs at minimum pstate instead of maximum pstate where it should be.

... Doug

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

[Rafael J. Wysocki]

On Wed, Jun 11, 2014 at 11:40 PM, Doug Smythies <dsmythies@telus.net> wrote:
>
>
> -----Original Message-----
> From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf Of Rafael J. Wysocki
> Sent: June-11-2014 11:29
> To: Doug Smythies
> Cc: Stratos Karafotis; linux-pm@vger.kernel.org; Linux Kernel Mailing List; Rafael J. Wysocki; viresh.kumar@linaro.org; dirk.j.brandewie@intel.com
> Subject: Re: [PATCH] cpufreq: intel_pstate: Fix rounding of core_pct
>
> On 2014.06.11 11:29 Rafael J. Wysocki wrote:
>> On Wed, Jun 11, 2014 at 5:02 PM, Doug Smythies <dsmythies@telus.net> 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.
>
>> Well, can you please do it if that's not a problem?
>
> Yes, of course, but it is a matter of priorities. All I meant by the above comment was that we might be able to forget about the remainder and just do a mindless divide, but leaving it as it is now does no harm in the meantime.
>
> Myself, I consider the issue of excessive deferred timer times to be a much higher priority (see my on-list e-mail from Monday). Correct me if I am wrong.
> Even without the "excessive" part, and for a 250 Hz kernel, the current kick in point can be hit routinely, unduly biasing the CPU frequency downwards.
> A random example (250 Hz kernel): 23% load at 25 Hertz load / sleep frequency for 300 total seconds.
>
> Duration histrogram:
>
> Occurrences duration (seconds)
>      16 0.044
>      39 0.024
>      45 0.028
>      46 0.016
>      48 0.032
>      61 0.036
>     166 0.012
>     198 0.020
>    7166 0.040
>
> Where you can see that the majority of the time the duration is such that the code will force the CPU frequency downwards. It runs at minimum pstate instead of maximum pstate where it should be.

I see.

What would you suggest to do to address this problem, then?

Rafael

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

[Doug Smythies]

On 2014.06.11 14:45 Rafael J. Wysocki wrote:
> On Wed, Jun 11, 2014 at 11:40 PM, Doug Smythies <dsmythies@telus.net> wrote:
>
>> Myself, I consider the issue of excessive deferred timer times to be a much higher priority (see my on-list e-mail from Monday). Correct me if I am wrong.
>> Even without the "excessive" part, and for a 250 Hz kernel, the current kick in point can be hit routinely, unduly biasing the CPU frequency downwards.
>> A random example (250 Hz kernel): 23% load at 25 Hertz load / sleep frequency for 300 total seconds.
>>
>> Duration histrogram:
>>
>> Occurrences duration (seconds)
>>      16 0.044
>>      39 0.024
>>      45 0.028
>>      46 0.016
>>      48 0.032
>>      61 0.036
>>     166 0.012
>>     198 0.020
>>    7166 0.040
>>
>> Where you can see that the majority of the time the duration is such that the code will force the CPU frequency downwards.
>> It runs at minimum pstate instead of maximum pstate where it should be.

> I see.
> What would you suggest to do to address this problem, then?

The above specific example can be solved by increasing the kick in factor from "sample_rate * 3" to something more.

As mentioned in my e-mail of Monday, I do not know how to proceed further with investigating the excessive deferral issue.

There are some ideas (I think originally from Dirk) that wouldn't involve "[PATCH 3/4] intel_pstate: add sample time scaling" at all, but so far they have had issues also. There is something I would like to try, but it will take at least a few days.

... Doug

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

[Doug Smythies]

On 2014.06.11 14:45 Rafael J. Wysocki wrote:
> On Wed, Jun 11, 2014 at 11:40 PM, Doug Smythies <dsmythies@telus.net> wrote:
>
>> Myself, I consider the issue of excessive deferred timer times to be a much higher priority (see my on-list e-mail from Monday). Correct me if I am wrong.
>> Even without the "excessive" part, and for a 250 Hz kernel, the current kick in point can be hit routinely, unduly biasing the CPU frequency downwards.
>> A random example (250 Hz kernel): 23% load at 25 Hertz load / sleep frequency for 300 total seconds.
>>
>> Duration histrogram:
>>
>> Occurrences duration (seconds)
>>      16 0.044
>>      39 0.024
>>      45 0.028
>>      46 0.016
>>      48 0.032
>>      61 0.036
>>     166 0.012
>>     198 0.020
>>    7166 0.040
>>
>> Where you can see that the majority of the time the duration is such that the code will force the CPU frequency downwards.
>> It runs at minimum pstate instead of maximum pstate where it should be.

> I see.
> What would you suggest to do to address this problem, then?

The above specific example can be solved by increasing the kick in factor from "sample_rate * 3" to something more.

As mentioned in my e-mail of Monday, I do not know how to proceed further with investigating the excessive deferral issue.

There are some ideas (I think originally from Dirk) that wouldn't involve "[PATCH 3/4] intel_pstate: add sample time scaling" at all, but so far they have had issues also. There is something I would like to try, but it will take at least a few days.

... Doug

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

[Stratos Karafotis]

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

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

[Rafael J. Wysocki]

On Thursday, June 12, 2014 05:35:59 PM Stratos Karafotis wrote:
> 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.

Depending on the original reason, it may or may not be.

In theory, it may help reduce numerical drift resulting from rounding always in
one direction only, but I'm not really sure if that matters here.

Doug seems to have carried out full analysis, though.

Rafael

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

[Doug Smythies]

On 2014.06.12 13:03 Rafael J. Wysocki wrote:
> On Thursday, June 12, 2014 05:35:59 PM Stratos Karafotis wrote:
>> On 12/06/2014 12:15 πμ, Doug Smythies wrote:
>>> 
>>> 
>>> 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:

Fair comment. Thanks.

>>
>> 47.19 instead of 47.20
>> 
>> And using FRAC_BITS 8:
>> 
>> 47.79 instead of 47.80
>> 

I really wouldn't write it that way, as I find it misleading. It is really 47 and 19 256ths...
Anyway, I think we all understand.

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

O.K. The solution is overkill and div_u64 could have been used instead of div_u64_rem.
On my list, it is the lowest of priorities.

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

I'm not sure how to reply, a pseudo floating point number is just an integer.

> Depending on the original reason, it may or may not be.

The original reason for that overall code patch was to address the possible overflow of the math, which (as far I know and have tested) it does.
I think we have gone down a bit of rat hole here in terms of the detail.

... Doug

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

[Doug Smythies]

On 2014.06.12 13:03 Rafael J. Wysocki wrote:
> On Thursday, June 12, 2014 05:35:59 PM Stratos Karafotis wrote:
>> On 12/06/2014 12:15 πμ, Doug Smythies wrote:
>>> 
>>> 
>>> 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:

Fair comment. Thanks.

>>
>> 47.19 instead of 47.20
>> 
>> And using FRAC_BITS 8:
>> 
>> 47.79 instead of 47.80
>> 

I really wouldn't write it that way, as I find it misleading. It is really 47 and 19 256ths...
Anyway, I think we all understand.

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

O.K. The solution is overkill and div_u64 could have been used instead of div_u64_rem.
On my list, it is the lowest of priorities.

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

I'm not sure how to reply, a pseudo floating point number is just an integer.

> Depending on the original reason, it may or may not be.

The original reason for that overall code patch was to address the possible overflow of the math, which (as far I know and have tested) it does.
I think we have gone down a bit of rat hole here in terms of the detail.

... Doug

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

[Dirk Brandewie]

On 06/12/2014 01:03 PM, Rafael J. Wysocki wrote:
> On Thursday, June 12, 2014 05:35:59 PM Stratos Karafotis wrote:
>> 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?
>>>>

Sorry for being MIA on this thread I have been up to my eyeballs.

>>>>         if ((rem << 1) >= int_tofp(sample->mperf))
>>>>                 core_pct += 1;

The rounding should have been
        core_pct += (1 << (FRAC_BITS-1));
Since core_pct is is in fixeded point notation at this point. Adding .5 to
core_pct to round up.

As Stratos pointed out the the current code only adds 1/256 to core_pct

Since core_pct_busy stays in fixed point through out the rest of the
calculations ans we only do the rounding when the PID is returning an
int I think we can safely remove these two lines.

>>>>


>>>> 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.
>
> Depending on the original reason, it may or may not be.
>
> In theory, it may help reduce numerical drift resulting from rounding always in
> one direction only, but I'm not really sure if that matters here.
>
> Doug seems to have carried out full analysis, though.
>
> Rafael
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

[Doug Smythies]

On 2014.06.12 06:49 Dirk Brandewie wrote:
> On 06/12/2014 01:03 PM, Rafael J. Wysocki wrote:
>> On Thursday, June 12, 2014 05:35:59 PM Stratos Karafotis wrote:
>>> On 12/06/2014 12:15 πμ, Doug Smythies wrote:

>>>>> Could you please elaborate a little bit more what we need these 2 lines below?
>>>>>

> Sorry for being MIA on this thread I have been up to my eyeballs.

>>>>         if ((rem << 1) >= int_tofp(sample->mperf))
>>>>                 core_pct += 1;

> The rounding should have been
>        core_pct += (1 << (FRAC_BITS-1));
> Since core_pct is is in fixeded point notation at this point. Adding .5 to
> core_pct to round up.

> As Stratos pointed out the the current code only adds 1/256 to core_pct

> Since core_pct_busy stays in fixed point through out the rest of the
> calculations ans we only do the rounding when the PID is returning an
> int I think we can safely remove these two lines.

Absolutely, no.

That code was doing exactly what I wanted it to do.
But, as and I have already admitted, it was overkill, and yes the entire thing
can be changed to use div_64 instead.

We do not want to add 1/2 to core percent here at this spot.
You would just be bringing back the arbitrary and incorrect biasing
of core_pct upwards that used to be there in two spots before.

You would add 1/2 when you want to convert to an integer, not before (and we don't right now, for the call to trace_pstate_sample).

... Doug

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

[Doug Smythies]

On 2014.06.12 06:49 Dirk Brandewie wrote:
> On 06/12/2014 01:03 PM, Rafael J. Wysocki wrote:
>> On Thursday, June 12, 2014 05:35:59 PM Stratos Karafotis wrote:
>>> On 12/06/2014 12:15 πμ, Doug Smythies wrote:

>>>>> Could you please elaborate a little bit more what we need these 2 lines below?
>>>>>

> Sorry for being MIA on this thread I have been up to my eyeballs.

>>>>         if ((rem << 1) >= int_tofp(sample->mperf))
>>>>                 core_pct += 1;

> The rounding should have been
>        core_pct += (1 << (FRAC_BITS-1));
> Since core_pct is is in fixeded point notation at this point. Adding .5 to
> core_pct to round up.

> As Stratos pointed out the the current code only adds 1/256 to core_pct

> Since core_pct_busy stays in fixed point through out the rest of the
> calculations ans we only do the rounding when the PID is returning an
> int I think we can safely remove these two lines.

Absolutely, no.

That code was doing exactly what I wanted it to do.
But, as and I have already admitted, it was overkill, and yes the entire thing
can be changed to use div_64 instead.

We do not want to add 1/2 to core percent here at this spot.
You would just be bringing back the arbitrary and incorrect biasing
of core_pct upwards that used to be there in two spots before.

You would add 1/2 when you want to convert to an integer, not before (and we don't right now, for the call to trace_pstate_sample).

... Doug

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

[Stratos Karafotis]

On 13/06/2014 04:48 μμ, Dirk Brandewie wrote:
> On 06/12/2014 01:03 PM, Rafael J. Wysocki wrote:
>> On Thursday, June 12, 2014 05:35:59 PM Stratos Karafotis wrote:
>>> 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?
>>>>>
> 
> Sorry for being MIA on this thread I have been up to my eyeballs.
> 
>>>>>         if ((rem << 1) >= int_tofp(sample->mperf))
>>>>>                 core_pct += 1;
> 
> The rounding should have been
>        core_pct += (1 << (FRAC_BITS-1));
> Since core_pct is is in fixeded point notation at this point. Adding .5 to
> core_pct to round up.
> 
> As Stratos pointed out the the current code only adds 1/256 to core_pct
> 
> Since core_pct_busy stays in fixed point through out the rest of the
> calculations ans we only do the rounding when the PID is returning an
> int I think we can safely remove these two lines.
> 

Please let me know if you want me to send a new patch for this (after the merge
window). Or will you or Doug handle this?


>> Depending on the original reason, it may or may not be.
>>
>> In theory, it may help reduce numerical drift resulting from rounding always in
>> one direction only, but I'm not really sure if that matters here.
>>
>> Doug seems to have carried out full analysis, though.
>>
>> Rafael
>>
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 
> 

Thank you all, for your comments!

Stratos

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

[Stratos Karafotis]

On 13/06/2014 09:49 πμ, Doug Smythies wrote:
> On 2014.06.12 13:03 Rafael J. Wysocki wrote:
>> On Thursday, June 12, 2014 05:35:59 PM Stratos Karafotis wrote:
>>> On 12/06/2014 12:15 πμ, Doug Smythies wrote:
>>>>
>>>>
>>>> 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:
> 
> Fair comment. Thanks.
> 
>>>
>>> 47.19 instead of 47.20
>>>
>>> And using FRAC_BITS 8:
>>>
>>> 47.79 instead of 47.80
>>>
> 
> I really wouldn't write it that way, as I find it misleading. It is really 47 and 19 256ths...
> Anyway, I think we all understand.
> 
>>> This is a 0.0002% difference. I can't see how this is can affect the calculations
>>> even with FRAC_BITS 6.
> 
> O.K. The solution is overkill and div_u64 could have been used instead of div_u64_rem.
> On my list, it is the lowest of priorities.
> 
>>>
>>> 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.
> 
> I'm not sure how to reply, a pseudo floating point number is just an integer.
> 
>> Depending on the original reason, it may or may not be.
> 
> The original reason for that overall code patch was to address the possible overflow of the math, which (as far I know and have tested) it does.
> I think we have gone down a bit of rat hole here in terms of the detail.
> 

Hi Doug,

I'm sorry if I pushed it too far.
But sometimes many small details could make the difference.
At least we finally agreed to something! :-)

Thanks for your time and for your comments!
Stratos



P.S. Talking about small details and with a sense of humor (I hope)
I present some roughly estimates about the tiny change (the 2-3 lines
removal).

Let's assume that this code needs 100 CPU cycles to run.

In a full active core, at a sampling rate 10ms, the code runs
8,640,000 times/day and if we suppose that the core will be 90%
of the day inactive, it needs 86,400,000 CPU cycles/day.

If the CPU runs in a typical 2GHz freq the code will need
0.0432 secs to be executed (per day). With a 15W avg power
consumption we need 0,648 Joules/day per core.

In a typical quad core PC we need 2.592 Joules/day or
946,08 Joules/year.

I read that there are 2 billion PCs in the planet.
Assuming that 1.5% of them running Linux and 50% of them
will use this driver, the code will run on 30,000,000 PCs.

So, we need:
14,191,200,000 Joules/year = 3,942 KWh

And:
3,942 KWh * 2.3 = 9,066 lb CO2 = 4,112 Kg CO2

Thus, removing these 2 lines we will reduce the global CO2 emissions
by 4,112 Kg! :-)