Re: [PATCH 03/12] trace-cmd analyze: Show how much tasks run on each CPU

From: Kalesh Singh <kaleshsingh@google.com>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: John Keeping <john@metanate.com>,
	linux-trace-devel@vger.kernel.org, williskung@google.com,
	linux-rt-users@vger.kernel.org
Subject: Re: [PATCH 03/12] trace-cmd analyze: Show how much tasks run on each CPU
Date: Mon, 28 Mar 2022 09:23:30 -0700	[thread overview]
Message-ID: <CAC_TJvdq8_DwkNQKE4=ypHxSv07zDv8w7_84Dw=bAePAO8P9wA@mail.gmail.com> (raw)
In-Reply-To: <20220326111632.09424859@rorschach.local.home>

On Sat, Mar 26, 2022 at 8:16 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Sat, 26 Mar 2022 11:07:03 +0000
> John Keeping <john@metanate.com> wrote:
>
> > > > > +               print_time(cpu_tasks[i]->runtime, '_');
> > > > > +               printf(" (%%%lld)\n", (task->runtime * 100) / total_time);
> > > >
> > > > Is there a reason for using the CPU-specific runtime for the value and
> > > > the total runtime for the percentage?
> > > >
> > > > I expected the percentage to be the percentage of this CPU's time spend
> > > > running the task.
> > >
> > > We modify it so that each CPU has the same run time, unless there's missed
> > > events at the start (later patches), and then we change total_time to be
> > > the total time of the events on the CPU and not the entire trace.
> >
> > I think we're talking about different things here, I probably wasn't
> > entirely clear about this.  It's the numerator of this division that I'm
> > concerned about and I wonder if this should be:
> >
> >       (cpu_tasks[i]->runtime * 100) / total_time
>
> Ah, I did misunderstand you. Yes, that's a typo. Thanks for pointing
> that out. I'll go fix it.

> > > > > +       if (idle_task) {
> > > > > +               printf("idle:\t");
> > > > > +               print_time(idle_task->runtime, '_');
> > > > > +               printf(" (%%%lld)\n", (idle_task->runtime * 100) / total_time);

IIUC the idle_task->runtime works out the same as the per-cpu runtime.
But for consistency I think we should use idle_cpu_task->runtime here
as well

Thanks,
Kalesh

>
> -- Steve
>
>