| Message ID | 1399579047-5792-5-git-send-email-dirk.j.brandewie@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Dirk, I don't get why you remove the c0 tracking. This is my understanding wrong. Suppose it is removed, then the objective of the PID control is: delta(aperf) / delta(mperf) * max_pstate / current_pstate But delta(aperf) / delta(mperf) * max_pstate is the average frequency of the last time frame, and current_pstate is the last requested frequency, then the objective is: last_freq_average / last_requested_freq ==> setpoint What does it mean, SW satisfaction of freq request? Why control that? Thanks Yuyang > @@ -561,46 +559,37 @@ static inline void intel_pstate_calc_busy(struct cpudata *cpu, > struct sample *sample) > { > int32_t core_pct; > - int32_t c0_pct; > > core_pct = div_fp(int_tofp((sample->aperf)), > int_tofp((sample->mperf))); > core_pct = mul_fp(core_pct, int_tofp(100)); > FP_ROUNDUP(core_pct); > > - c0_pct = div_fp(int_tofp(sample->mperf), int_tofp(sample->tsc)); > - > sample->freq = fp_toint( > mul_fp(int_tofp(cpu->pstate.max_pstate * 1000), core_pct)); > > - sample->core_pct_busy = mul_fp(core_pct, c0_pct); > + sample->core_pct_busy = core_pct; > } -- 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, May 08, 2014 12:57:26 PM dirk.brandewie@gmail.com wrote: > From: Dirk Brandewie <dirk.j.brandewie@intel.com> > > Commit fcb6a15c intel_pstate: Take core C0 time into account for core busy > introduced a regression referenced below. The issue with "lockup" > after suspend that this commit was addressing is now dealt with in the > suspend path. > > References: > https://bugzilla.kernel.org/show_bug.cgi?id=66581 > https://bugzilla.kernel.org/show_bug.cgi?id=75121 > > Reported-by: Doug Smythies <dsmythies@telus.net> > Signed-off-by: Dirk Brandewie <dirk.j.brandewie@intel.com> Do we need this in -stable? > --- > drivers/cpufreq/intel_pstate.c | 13 +------------ > 1 file changed, 1 insertion(+), 12 deletions(-) > > diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c > index bb20881..4c26faf 100644 > --- a/drivers/cpufreq/intel_pstate.c > +++ b/drivers/cpufreq/intel_pstate.c > @@ -59,7 +59,6 @@ struct sample { > int32_t core_pct_busy; > u64 aperf; > u64 mperf; > - unsigned long long tsc; > int freq; > }; > > @@ -100,7 +99,6 @@ struct cpudata { > > u64 prev_aperf; > u64 prev_mperf; > - unsigned long long prev_tsc; > struct sample sample; > }; > > @@ -561,46 +559,37 @@ static inline void intel_pstate_calc_busy(struct cpudata *cpu, > struct sample *sample) > { > int32_t core_pct; > - int32_t c0_pct; > > core_pct = div_fp(int_tofp((sample->aperf)), > int_tofp((sample->mperf))); > core_pct = mul_fp(core_pct, int_tofp(100)); > FP_ROUNDUP(core_pct); > > - c0_pct = div_fp(int_tofp(sample->mperf), int_tofp(sample->tsc)); > - > sample->freq = fp_toint( > mul_fp(int_tofp(cpu->pstate.max_pstate * 1000), core_pct)); > > - sample->core_pct_busy = mul_fp(core_pct, c0_pct); > + sample->core_pct_busy = core_pct; > } > > static inline void intel_pstate_sample(struct cpudata *cpu) > { > u64 aperf, mperf; > - unsigned long long tsc; > > rdmsrl(MSR_IA32_APERF, aperf); > rdmsrl(MSR_IA32_MPERF, mperf); > - tsc = native_read_tsc(); > > aperf = aperf >> FRAC_BITS; > mperf = mperf >> FRAC_BITS; > - tsc = tsc >> FRAC_BITS; > > cpu->sample.aperf = aperf; > cpu->sample.mperf = mperf; > - cpu->sample.tsc = tsc; > cpu->sample.aperf -= cpu->prev_aperf; > cpu->sample.mperf -= cpu->prev_mperf; > - cpu->sample.tsc -= cpu->prev_tsc; > > intel_pstate_calc_busy(cpu, &cpu->sample); > > cpu->prev_aperf = aperf; > cpu->prev_mperf = mperf; > - cpu->prev_tsc = tsc; > } > > static inline void intel_pstate_set_sample_time(struct cpudata *cpu) >
Hi, On 12/05/2014 05:14 ??, Stratos Karafotis wrote: > From: Dirk Brandewie <dirk.j.brandewie@intel.com> > > Commit fcb6a15c intel_pstate: Take core C0 time into account for core busy > introduced a regression referenced below. The issue with "lockup" > after suspend that this commit was addressing is now dealt with in the > suspend path. > > References: > https://bugzilla.kernel.org/show_bug.cgi?id=66581 > https://bugzilla.kernel.org/show_bug.cgi?id=75121 > > Reported-by: Doug Smythies <dsmythies@telus.net> > Signed-off-by: Dirk Brandewie <dirk.j.brandewie@intel.com> > --- > drivers/cpufreq/intel_pstate.c | 13 +------------ > 1 file changed, 1 insertion(+), 12 deletions(-) > > diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c > index bb20881..4c26faf 100644 > --- a/drivers/cpufreq/intel_pstate.c > +++ b/drivers/cpufreq/intel_pstate.c > @@ -59,7 +59,6 @@ struct sample { > int32_t core_pct_busy; > u64 aperf; > u64 mperf; > - unsigned long long tsc; > int freq; > }; > > @@ -100,7 +99,6 @@ struct cpudata { > > u64 prev_aperf; > u64 prev_mperf; > - unsigned long long prev_tsc; > struct sample sample; > }; > > @@ -561,46 +559,37 @@ static inline void intel_pstate_calc_busy(struct > cpudata *cpu, > struct sample *sample) > { > int32_t core_pct; > - int32_t c0_pct; > > core_pct = div_fp(int_tofp((sample->aperf)), > int_tofp((sample->mperf))); > core_pct = mul_fp(core_pct, int_tofp(100)); > FP_ROUNDUP(core_pct); > > - c0_pct = div_fp(int_tofp(sample->mperf), int_tofp(sample->tsc)); > - > sample->freq = fp_toint( > mul_fp(int_tofp(cpu->pstate.max_pstate * 1000), core_pct)); > > - sample->core_pct_busy = mul_fp(core_pct, c0_pct); > + sample->core_pct_busy = core_pct; > } > > static inline void intel_pstate_sample(struct cpudata *cpu) > { > u64 aperf, mperf; > - unsigned long long tsc; > > rdmsrl(MSR_IA32_APERF, aperf); > rdmsrl(MSR_IA32_MPERF, mperf); > - tsc = native_read_tsc(); > > aperf = aperf >> FRAC_BITS; > mperf = mperf >> FRAC_BITS; > - tsc = tsc >> FRAC_BITS; > > cpu->sample.aperf = aperf; > cpu->sample.mperf = mperf; > - cpu->sample.tsc = tsc; > cpu->sample.aperf -= cpu->prev_aperf; > cpu->sample.mperf -= cpu->prev_mperf; > - cpu->sample.tsc -= cpu->prev_tsc; > > intel_pstate_calc_busy(cpu, &cpu->sample); > > cpu->prev_aperf = aperf; > cpu->prev_mperf = mperf; > - cpu->prev_tsc = tsc; > } > > static inline void intel_pstate_set_sample_time(struct cpudata *cpu) > -- > 1.9.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > With this patch, my CPU (Core i7-3770 @ 3.90GHz) seems to never use lowest frequencies. Even on an idle system I get always ~2GHz. Normally, on an idle system it used to be 1.6GHz. On very small loads (mp3 decoding) the CPU goes up to 2.7G GHz (it used to be 1.6GHz) Reverting, this patch on my local build, the problem is resolved. Thanks, Stratos Karafotis -- 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 Monday, May 12, 2014 05:27:25 AM Stratos Karafotis wrote: > Hi, > [cut] > > With this patch, my CPU (Core i7-3770 @ 3.90GHz) seems to never use lowest > frequencies. Even on an idle system I get always ~2GHz. Normally, > on an idle system it used to be 1.6GHz. > On very small loads (mp3 decoding) the CPU goes up to 2.7G GHz (it used to > be 1.6GHz) > > Reverting, this patch on my local build, the problem is resolved. Dirk, seriously, I can't regard this as a fix that can go into -rc6. Which of the other patch in the series are must-go for 3.15? [1-2/5] I guess? And do we need [2/5] it in -stable too?
On 05/12/2014 05:16 AM, Rafael J. Wysocki wrote: > On Monday, May 12, 2014 05:27:25 AM Stratos Karafotis wrote: >> Hi, >> > > [cut] > >> >> With this patch, my CPU (Core i7-3770 @ 3.90GHz) seems to never use lowest >> frequencies. Even on an idle system I get always ~2GHz. Normally, >> on an idle system it used to be 1.6GHz. >> On very small loads (mp3 decoding) the CPU goes up to 2.7G GHz (it used to >> be 1.6GHz) >> >> Reverting, this patch on my local build, the problem is resolved. > > Dirk, seriously, I can't regard this as a fix that can go into -rc6. > Ok I will resubmit after more testing > Which of the other patch in the series are must-go for 3.15? [1-2/5] I guess? > And do we need [2/5] it in -stable too? 1/5 is for stable it fixes a random MCE on baytrail. 2/5 is for stable it should have went with the stop_cpu patch 5/5 can go too since it is just adding CPU IDs > > -- 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 Monday, May 12, 2014 07:39:35 AM Dirk Brandewie wrote: > On 05/12/2014 05:16 AM, Rafael J. Wysocki wrote: > > On Monday, May 12, 2014 05:27:25 AM Stratos Karafotis wrote: > >> Hi, > >> > > > > [cut] > > > >> > >> With this patch, my CPU (Core i7-3770 @ 3.90GHz) seems to never use lowest > >> frequencies. Even on an idle system I get always ~2GHz. Normally, > >> on an idle system it used to be 1.6GHz. > >> On very small loads (mp3 decoding) the CPU goes up to 2.7G GHz (it used to > >> be 1.6GHz) > >> > >> Reverting, this patch on my local build, the problem is resolved. > > > > Dirk, seriously, I can't regard this as a fix that can go into -rc6. > > > > Ok I will resubmit after more testing > > > Which of the other patch in the series are must-go for 3.15? [1-2/5] I guess? > > And do we need [2/5] it in -stable too? > > 1/5 is for stable it fixes a random MCE on baytrail. > 2/5 is for stable it should have went with the stop_cpu patch Which -stable are these two for? All of them? > 5/5 can go too since it is just adding CPU IDs Well, I'm a bit reluctant to push that for -rc6. There were cases where new IDs broke stuff that worked without them just fine.
diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c index bb20881..4c26faf 100644 --- a/drivers/cpufreq/intel_pstate.c +++ b/drivers/cpufreq/intel_pstate.c @@ -59,7 +59,6 @@ struct sample { int32_t core_pct_busy; u64 aperf; u64 mperf; - unsigned long long tsc; int freq; }; @@ -100,7 +99,6 @@ struct cpudata { u64 prev_aperf; u64 prev_mperf; - unsigned long long prev_tsc; struct sample sample; }; @@ -561,46 +559,37 @@ static inline void intel_pstate_calc_busy(struct cpudata *cpu, struct sample *sample) { int32_t core_pct; - int32_t c0_pct; core_pct = div_fp(int_tofp((sample->aperf)), int_tofp((sample->mperf))); core_pct = mul_fp(core_pct, int_tofp(100)); FP_ROUNDUP(core_pct); - c0_pct = div_fp(int_tofp(sample->mperf), int_tofp(sample->tsc)); - sample->freq = fp_toint( mul_fp(int_tofp(cpu->pstate.max_pstate * 1000), core_pct)); - sample->core_pct_busy = mul_fp(core_pct, c0_pct); + sample->core_pct_busy = core_pct; } static inline void intel_pstate_sample(struct cpudata *cpu) { u64 aperf, mperf; - unsigned long long tsc; rdmsrl(MSR_IA32_APERF, aperf); rdmsrl(MSR_IA32_MPERF, mperf); - tsc = native_read_tsc(); aperf = aperf >> FRAC_BITS; mperf = mperf >> FRAC_BITS; - tsc = tsc >> FRAC_BITS; cpu->sample.aperf = aperf; cpu->sample.mperf = mperf; - cpu->sample.tsc = tsc; cpu->sample.aperf -= cpu->prev_aperf; cpu->sample.mperf -= cpu->prev_mperf; - cpu->sample.tsc -= cpu->prev_tsc; intel_pstate_calc_busy(cpu, &cpu->sample); cpu->prev_aperf = aperf; cpu->prev_mperf = mperf; - cpu->prev_tsc = tsc; } static inline void intel_pstate_set_sample_time(struct cpudata *cpu)