Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753737AbeAFQkm (ORCPT + 1 other); Sat, 6 Jan 2018 11:40:42 -0500 Received: from cmta18.telus.net ([209.171.16.91]:41445 "EHLO cmta18.telus.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753591AbeAFQkk (ORCPT ); Sat, 6 Jan 2018 11:40:40 -0500 X-Authority-Analysis: v=2.2 cv=MfCDg93f c=1 sm=1 tr=0 a=zJWegnE7BH9C0Gl4FFgQyA==:117 a=zJWegnE7BH9C0Gl4FFgQyA==:17 a=Pyq9K9CWowscuQLKlpiwfMBGOR0=:19 a=IkcTkHD0fZMA:10 a=pGLkceISAAAA:8 a=aatUQebYAAAA:8 a=bWQFuwEW0lS5aXwLot4A:9 a=QEXdDO2ut3YA:10 a=7715FyvI7WU-l6oqrZBK:22 From: "Doug Smythies" To: "'Rafael J. Wysocki'" Cc: "'Srinivas Pandruvada'" , "'Rafael J. Wysocki'" , "'Linux Kernel Mailing List'" , "'Linux PM'" , "Doug Smythies" References: <1515190444-3254-1-git-send-email-dsmythies@telus.net> XaqeeUTMLTqmMXaqfe1WhS In-Reply-To: XaqeeUTMLTqmMXaqfe1WhS Subject: RE: [PATCH V4] cpufreq: intel_pstate: allow trace in passive mode Date: Sat, 6 Jan 2018 08:40:34 -0800 Message-ID: <000101d3870d$150c7960$3f256c20$@net> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Mailer: Microsoft Office Outlook 12.0 Thread-Index: AdOGd9qQ81YHlEeSTS+SyWTwf9R2mwAkdgSA Content-Language: en-ca X-CMAE-Envelope: MS4wfHN23uwGhvowinvOiR8QXtG/jIw1omKjhVQ+A62Kji7Hl/tsSvAMgzh7sKIh+16tqKvRqhnjFVuxkpIQc20TE5OCZkzqrR/XMHB339J4+De7fOFZj1Lv OiwcfRGJ6sHVoHksksVyxX/mRpzx331dN7+TRggtFLhoPVCtAlsha8D7Q8xF1/KuibyjNkQuas0YHiR00zO74Z7plJAnxA0b9F1JcugaknYctIlpcjpLvMNb f3/guWpEijNkZZH84ECG2DuA6G9Bzspa50oK5/BZ2NYzg+uuhBy0leHBfc1JHRJAIp23wsHE6YJ+LK762t5HQ/psdbSa47x873Pv1IFN3G4CuYajmwIe9IhO /CWK6Cvq Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: On 2018.01.05 14:52 Rafael J. Wysocki wrote: > On Fri, Jan 5, 2018 at 11:14 PM, Doug Smythies wrote: >> Allow use of the trace_pstate_sample trace function >> when the intel_pstate driver is in passive mode. >> Since the core_busy and scaled_busy fields are not >> used, and it might be desirable to know which path >> through the driver was used, either intel_cpufreq_target >> or intel_cpufreq_fast_switch, re-task the core_busy >> field as a flag indicator. >> >> The user can then use the intel_pstate_tracer.py utility >> to summarize and plot the trace. >> >> Sometimes, in passive mode, the driver is not called for >> many tens or even hundreds of seconds. The user >> needs to understand, and not be confused by, this limitation. > > The description of the changes between different versions should go > under the Signed-off-by: tag, separated by an extra "---" from it. O.K. sorry. > Also please see a couple of cosmetic comments below. > >> V4: Only execute the trace specific overhead code if trace >> is enabled. Suggested by Srinivas Pandruvada. >> >> V3: Move largely duplicate code to a subroutine. >> Suggested by Rafael J. Wysocki. >> >> V2: prepare for resend. Rebase to current kernel, 4.15-rc3. >> Signed-off-by: Doug Smythies >> --- >> drivers/cpufreq/intel_pstate.c | 35 +++++++++++++++++++++++++++++++++-- >> 1 file changed, 33 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c >> index 93a0e88..53bb953 100644 >> --- a/drivers/cpufreq/intel_pstate.c >> +++ b/drivers/cpufreq/intel_pstate.c >> @@ -1943,13 +1943,40 @@ static int intel_cpufreq_verify_policy(struct cpufreq_policy *policy) >> return 0; >> } >> >> +static void intel_cpufreq_trace(struct cpudata *cpu, int fast, int from) > > Please use "bool" for "fast" and I'd call it "fast_switch". O.K. thanks. >> +{ >> + struct sample *sample; >> + u64 time; >> + >> + time = ktime_get(); > > It is pointless to evaluate ktime_get() if > trace_pstate_sample_enabled() returns "false". Of course, thanks. >> + if (trace_pstate_sample_enabled()) { >> + if (intel_pstate_sample(cpu, time)) { > > And the extra indentation here is not very useful, so I'd write it as > > if (!trace_pstate_sample_enabled()) > return; > > if (!intel_pstate_sample(cpu, ktime_get())) > return; > > (note that you don't need the "time" variable any more with this). That is much better, Thanks. >> + sample = &cpu->sample; >> + /* In passvie mode the trace core_busy field is > > "passive" (typo) > >> + * re-assigned to indicate if the driver call >> + * was via the normal or fast switch path. >> + * The scaled_busy field is not used, set to 0. >> + */ >> + trace_pstate_sample(fast, >> + 0, >> + from, >> + cpu->pstate.current_pstate, >> + sample->mperf, >> + sample->aperf, >> + sample->tsc, >> + get_avg_frequency(cpu), >> + fp_toint(cpu->iowait_boost * 100)); >> + } >> + } >> +} >> + >> static int intel_cpufreq_target(struct cpufreq_policy *policy, >> unsigned int target_freq, >> unsigned int relation) >> { >> struct cpudata *cpu = all_cpu_data[policy->cpu]; >> struct cpufreq_freqs freqs; >> - int target_pstate; >> + int target_pstate, from; > > I would call the new variable "old_pstate" or "orig_pstate" (so that > it is visibly clear that it represents a P-state). O.K. I used "from" because that is what Dirk called it in the trace buffer stuff. >> >> update_turbo_state(); >> >> @@ -1969,12 +1996,14 @@ static int intel_cpufreq_target(struct cpufreq_policy *policy, >> break; >> } >> target_pstate = intel_pstate_prepare_request(cpu, target_pstate); >> + from = cpu->pstate.current_pstate; >> if (target_pstate != cpu->pstate.current_pstate) { >> cpu->pstate.current_pstate = target_pstate; >> wrmsrl_on_cpu(policy->cpu, MSR_IA32_PERF_CTL, >> pstate_funcs.get_val(cpu, target_pstate)); >> } >> freqs.new = target_pstate * cpu->pstate.scaling; >> + intel_cpufreq_trace(cpu, 0, from); >> cpufreq_freq_transition_end(policy, &freqs, false); >> >> return 0; >> @@ -1984,13 +2013,15 @@ static unsigned int intel_cpufreq_fast_switch(struct cpufreq_policy *policy, >> unsigned int target_freq) >> { >> struct cpudata *cpu = all_cpu_data[policy->cpu]; >> - int target_pstate; >> + int target_pstate, from; >> >> update_turbo_state(); >> >> target_pstate = DIV_ROUND_UP(target_freq, cpu->pstate.scaling); >> target_pstate = intel_pstate_prepare_request(cpu, target_pstate); >> + from = cpu->pstate.current_pstate; >> intel_pstate_update_pstate(cpu, target_pstate); >> + intel_cpufreq_trace(cpu, 100, from); > > Why are you passing 100 here? Anything different from 0 should > suffice, 1 in particular. And I'd pass "false" or "true" (they will > be converted to 0 and 1 for output anyway). Well, I wanted to just re-use the existing graphs generated by tools/power/x86/intel_pstate_tracer/intel_pstate_tracer.py and so wanted to pass 0 or 100% to it. On purpose, those graphs do not autoscale on the y-axis. When investigating, the graphs can be used as a way to determine where to look in more detail at the raw csv files. >> return target_pstate * cpu->pstate.scaling; >> } >>