Re: [Powertop] [PATCH POWERTOP V3] resolve the bug that powertop doesn't show correct frequency stats

On Mon, 2013-10-07 at 22:54 +0800, Shaojie Sun wrote: > add jon.medhurst(a)linaro.org > > On Mon, Oct 7, 2013 at 7:40 PM, Sergey Senozhatsky > <sergey.senozhatsky(a)gmail.com&gt; wrote: > > On (10/07/13 19:31), Shaojie Sun wrote: > >> Date: Mon, 7 Oct 2013 19:31:54 +0800 > >> From: Shaojie Sun <shaojie.sun(a)linaro.org&gt; > >> To: Sergey Senozhatsky <sergey.senozhatsky(a)gmail.com&gt; > >> Cc: Sanjay Rawat <sanjay.rawat(a)linaro.org&gt;, Jon Medhurst <tixy(a)linaro.org&gt;, > >> SunShaoJie <sunshaojie(a)huawei.com&gt;, "Private-PMWG@Linaro" > >> <private-pmwg(a)linaro.org&gt; > >> Subject: Re: [Powertop] [PATCH POWERTOP V3] resolve the bug that powertop > >> doesn't show correct frequency stats > >> > >> Goog question. > >> Could Tixy answer it? > > > > /** Removed Private-PMWG (no permissions), Cc'd powertop list. */ > > > > > > > > the thing is that we tend to trust passed cpunr in many handle_trace_point()-s. > > so it would be better to fix all sample->trace.cpu users, because the > > problme may be a bit bigger than that (e.g. src/process/do_process.cpp > > perf_process_bundle::handle_trace_point()). That's what I thought too and said so when I commented on the Linaro's bug tracking system [1]. I was hoping that someone more familiar with powertop would investigate further as I had never looked at powertop or any perf related code before. [1] https://bugs.launchpad.net/linaro-android/+bug/1042755/comments/14 -- Tixy > > > > -ss > > > >> On Mon, Oct 7, 2013 at 6:03 PM, Sergey Senozhatsky > >> <sergey.senozhatsky(a)gmail.com&gt; wrote: > >> > On (10/07/13 17:26), Shaojie Sun wrote: > >> >> From: Jon Medhurst <tixy(a)linaro.org&gt; > >> >> > >> >> For cpu_frequency events, perf_power_bundle::handle_trace_point() > >> >> is always being called with the same cpunr. so the parameter of > >> >> cpunr is not equal to the cpu which cpu_frequency events happened. > >> >> Lucky the trace event themselves have a cpu_id field to say to which > >> >> CPU event it relates, we should use this cpu_id field. > >> >> > >> >> Signed-off-by: Jon Medhurst <tixy(a)linaro.org&gt; > >> >> Shaojie Sun <shaojie.sun(a)linaro.com&gt; > >> >> --- > >> >> src/cpu/cpu.cpp | 9 +++++++-- > >> >> 1 file changed, 7 insertions(+), 2 deletions(-) > >> >> > >> >> diff --git a/src/cpu/cpu.cpp b/src/cpu/cpu.cpp > >> >> index 7f3af69..e304bbe 100644 > >> >> --- a/src/cpu/cpu.cpp > >> >> +++ b/src/cpu/cpu.cpp > >> >> @@ -781,10 +781,15 @@ void perf_power_bundle::handle_trace_point(void *trace, int cpunr, uint64_t time > >> >> > >> >> if (strcmp(event->name, "power_frequency") == 0 > >> >> || strcmp(event->name, "cpu_frequency") == 0){ > >> >> - > >> >> + ret = pevent_get_field_val(NULL, event, "cpu_id", &rec, &val, 0); > >> >> + if (ret < 0) { > >> >> + fprintf(stderr, _("power or cpu_frequency event returned no cpu?\n")); > >> >> + exit(-1); > >> >> + } > >> >> + cpu = all_cpus[val]; > >> >> ret = pevent_get_field_val(NULL, event, "state", &rec, &val, 0); > >> >> if (ret < 0) { > >> >> - fprintf(stderr, _("power or cpu_frequecny event returned no state?\n")); > >> >> + fprintf(stderr, _("power or cpu_frequency event returned no state?\n")); > >> >> exit(-1); > >> >> } > >> > > >> > Hello, > >> > why not fixing it in src/perf/perf_bundle (perf_sample's perf_event_header), > >> > where we create data for handle_trace_point() ? > >> > > >> > -ss > >> > > >> >> -- > >> >> 1.7.9.5 > >> >> > >> >> _______________________________________________ > >> >> PowerTop mailing list > >> >> PowerTop(a)lists.01.org > >> >> https://lists.01.org/mailman/listinfo/powertop > >> >> > >>