| Message ID | 1402490012-19969-1-git-send-email-stratosk@semaphore.gr (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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 -- 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
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 -- 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
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); } -- 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
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 -- 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
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 -- 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
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 -- 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
-----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 -- 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
-----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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 > -- 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
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 -- 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
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 -- 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
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! :-) -- 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
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));
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(-)