From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752404AbcHTGkM (ORCPT ); Sat, 20 Aug 2016 02:40:12 -0400 Received: from cmta20.telus.net ([209.171.16.93]:42371 "EHLO cmta20.telus.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751175AbcHTGkK (ORCPT ); Sat, 20 Aug 2016 02:40:10 -0400 X-Authority-Analysis: v=2.2 cv=Jo8elIwC c=1 sm=1 tr=0 a=zJWegnE7BH9C0Gl4FFgQyA==:117 a=zJWegnE7BH9C0Gl4FFgQyA==:17 a=Pyq9K9CWowscuQLKlpiwfMBGOR0=:19 a=kj9zAlcOel0A:10 a=SApZfaPtBWozuKFnShgA:9 a=3jeAW7_hqKlotQZX:21 a=PVRjW21ebRcgs2Is:21 From: "Doug Smythies" To: "'Peter Zijlstra'" Cc: "'Rafael J. Wysocki'" , "'Srinivas Pandruvada'" , "'Viresh Kumar'" , "'Linux Kernel Mailing List'" , "'Steve Muckle'" , "'Juri Lelli'" , "'Ingo Molnar'" , "'Linux PM list'" References: <3752826.3sXAQIvcIA@vostro.rjw.lan> <1572483.RZjvRFdxPx@vostro.rjw.lan> <001b01d1ee1c$e6a6a170$b3f3e450$@net> <10156236.RI1knH5Wfs@vostro.rjw.lan> <000301d1f57b$9e1fed10$da5fc730$@net> <20160819144729.GL10121@twins.programming.kicks-ass.net> In-Reply-To: <20160819144729.GL10121@twins.programming.kicks-ass.net> Subject: RE: [RFC][PATCH 7/7] cpufreq: intel_pstate: Change P-state selection algorithm for Core Date: Fri, 19 Aug 2016 23:40:04 -0700 Message-ID: <003f01d1faad$b15aa910$140ffb30$@net> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit X-Mailer: Microsoft Office Outlook 12.0 Thread-Index: AdH6KJ/5FK0nZxf5SGiZhXHvOuwSRAAgD52w Content-Language: en-ca X-CMAE-Envelope: MS4wfNq369yvtL62tM1q66eNo8Zwqd8MG26hxiKQ57XANgZXTcQv0urOXwvMKfuNog7WuridANc+36o8Hp9J1nVIt8XmxsRV4nvbJ436YSI+CQe6fNswDVw0 d/n3kc0hyemEp6w8yIReedQndRTOHa4oSCmVI1S+f2n0P6//S2lwEYORh1gihigMeUrTXXM/HvlzWrWc2uVy9WfDJEqJGgmYo+/0HU4lemCU9s0jnGvRe5TW 5iVwGh1fHCPhtIV7LtXd7a7JzjWOJSImC6VoNf/D4o33Pizo6OCuoQDnvfrCOE9VUKdgTUvFFfVstfA/nOAi1ugMPdeqd+LrfPJ5+WppU6kVy1yP8g0Qhj+k NaXrx2w0XZ9jmoUZwPmKRRJzbk2216YeWHc32jpi68amw1+vzGJyyjQVpIaffvt6LR3xG+AwmJ/tbmA7KdiLz/wW35pV+Q== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2016.08.19 07:47 Peter Zijlstra wrote: > On Sat, Aug 13, 2016 at 08:59:01AM -0700, Doug Smythies wrote: >> My previous replies (and see below) have suggested that some filtering >> is needed on the target pstate, otherwise, and dependant on the type of >> workload, it tends to oscillate. >> >> I added the IIR (Infinite Impulse Response) filter that I have suggested in the past: > > One question though; why is this filter intel_pstate specific? Should we > not do this in generic code? I wouldn't know. I'm not familiar with the other CPU frequency scaling drivers or what filtering, if any, they already have. >> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c >> index c43ef55..262ec5f 100644 >> --- a/drivers/cpufreq/intel_pstate.c >> +++ b/drivers/cpufreq/intel_pstate.c >> @@ -1313,7 +1318,74 @@ static inline int32_t get_target_pstate_default(struct cpudata *cpu) >> cpu->iowait_boost >>= 1; >> >> pstate = cpu->pstate.turbo_pstate; >> + unfiltered_target = (pstate + (pstate >> 2)) * busy_frac; >> + duration_ns = cpu->sample.time - cpu->last_sample_time; >> + >> + scaled_gain = div_u64(int_tofp(duration_ns) * >> + int_tofp(pid_params.p_gain_pct), int_tofp(pid_params.sample_rate_ns)); > > Drop int_to_fp() on one of the dividend terms and in the divisor. Same > end result since they divide away against one another but reduces the > risk of overflow. Yes of course. Thanks. > Also, sample_rate_ns, really!? A rate is in [1/s], should that thing be > called period_ns ? Agreed (strongly), however and as Rafael mentioned on his reply, this stuff has been around for a long time, including the externally available: /sys/kernel/debug/pstate_snb/sample_rate_ms Which be referenced by some documentation and scripts (I have some). Myself, I'd be O.K. to change it all to "period". >> + if (scaled_gain > int_tofp(100)) >> + scaled_gain = int_tofp(100); >> + if (scaled_gain < int_tofp(pid_params.p_gain_pct)) >> + scaled_gain = int_tofp(pid_params.p_gain_pct); >> + >> + /* >> + * Bandwidth limit the output. For now, re-task p_gain_pct for this purpose. >> + * Use a smple IIR (Infinite Impulse Response) filter. >> + */ >> + cpu->sample.target = div_u64((int_tofp(100) - scaled_gain) * >> + cpu->sample.target + scaled_gain * >> + unfiltered_target, int_tofp(100)); > > Really hard to read that stuff, maybe cure with a comment: > > /* > * g = dt*p / period > * > * target' = (1 - g)*target + g*input > */ Yes, O.K. I'll add more comments if this continues towards a formal patch submission. >> + >> + return fp_toint(cpu->sample.target + (1 << (FRAC_BITS-1))); >> }