Message-ID: <20131105115802.GA12045@gmail.com>
Date:	Tue, 5 Nov 2013 12:58:02 +0100
From:	Ingo Molnar <mingo@...nel.org>
To:	Namhyung Kim <namhyung@...nel.org>
Cc:	Pekka Enberg <penberg@...nel.org>,
	Arnaldo Carvalho de Melo <acme@...stprotocols.net>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Paul Mackerras <paulus@...ba.org>,
	Namhyung Kim <namhyung.kim@....com>,
	LKML <linux-kernel@...r.kernel.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Stephane Eranian <eranian@...gle.com>,
	Jiri Olsa <jolsa@...hat.com>,
	Rodrigo Campos <rodrigo@...g.com.ar>,
	Arun Sharma <asharma@...com>
Subject: Re: [RFC/PATCHSET 00/14] perf report: Add support to accumulate hist
 periods (v2)


* Namhyung Kim <namhyung@...nel.org> wrote:

> On Tue, 5 Nov 2013 08:46:50 +0100, Ingo Molnar wrote:
> > * Namhyung Kim <namhyung@...nel.org> wrote:
> >> I think it'd better to separate the option and pass column and
> >> (optional) sort key argument.
> >> 
> >>   --cumulative both,total (default)
> >>   --cumulative both,self
> >>   --cumulative total
> >>   --cumulative self (meaningless?)
> >> 
> >> Maybe we need a config option and a single letter option for the default
> >> case like --call-graph and -g options do.
> >> 
> >> What do you think?
> >
> > So why restrict it to 'cumulative'? Why not have a generic --fields/-F, 
> > with a good default. The ordering of the fields determines sorting 
> > behavior.
> 
> Ah, I didn't know you meant that too. :)
> 
> But the 'cumulative' (btw, I feel a bit hard to type this word..) is 
> different in that it *generates* entries didn't get sampled originally. 
> And as it requires callchains, total field will not work if callchains 
> are missing.

Well, 'total' should disappear if it's not available.

We already have some 'column elimination/optimization' logic - like the 
'dso' will disappear already if it's a single dso everywhere, IIRC?

> So I tried to make it a standalone option.
> 
> >
> > The default would be something like:
> >
> >   -F total,self,process,dso,name
> >
> > Whether 'cumulative' data is calculated is not a function of any direct 
> > option, but simply a function of whether the 'total' field is in the -F 
> > list of columns displayed or not.
> 
> So you want to turn the cumulative behavior always on, right?

Yes.

> But as Frederic noted, it might affect the performance of perf report, 
> so it might be better to delay this behavior to make default after users 
> feel comfortable with an option?

I think with call-chain speedups it should be fast enough, right?

We can argue about the default separately - if it's all done correctly 
then it should be really easy to change the default layout of 'perf 
report'.

> > With that scheme we could also do things like this to get old-style 
> > sorting:
> >
> >  -F self,process,dso,name
> >
> > Or a really frugal 'readprofile'-style output:
> >
> >  -F self,name
> >
> > if one is only interested in percentages and raw function names.
> 
> s/name/sym(bol)/ :)

Yeah.

> Yes, this is what we do with -s option now.
> 
> > Wrt. sorting order, by default the first column in the list of columns 
> > would be the primary (and only) sort key.
> 
> Ah, I never thought it like this way.  It makes sense as sort orders 
> really affect the output sorting.
> 
> > (The -F field setup list could also be specified in the .perfconfig.)
> >
> > With this method we could do away with all this geometrical explosion 
> > of somewhat inconsistent formatting and sorting options...
> 
> For now, there're two kind of columns:
> 
> - one for showing entry's overhead percentage: self, sys, user,
>   guest_sys and guest_user.  So the 'total' should go into this
>   category.  I named it hpp (hist_entry period percentage) functions and
>   yes, I know it's an awfully bad name. :)  Please see perf_hpp__format.
> 
>   There're controlled by a couple of options:  --show-total-period,
>   --show-nr-samples and --showcpuutilization (I hate this!).  And event
>   group also can affect its output.
> 
> - one for grouping entries: cpu, pid, comm, dso, symbol, srcline and
>   parent.  We call it "sort keys" but confusingly it doesn't affect 
>   output sorting for now.

Well, it's still a sort key in a sense, a string lexicographical ordering 
in essence, right?

> So I think cleaning this up with -F option is good and I've been wanting 
> this discussion for a long time. :)

Okay :-)

> > If there's demand then we could decouple sort keys from the display 
> > order, by slightly augmenting the field format:
> >
> >  -F total,self:2,process:0,dso:1,name
> >
> > This would sort by 'process' field as the primary key, 'dso' the secondary 
> > key and 'self' as the tertiary key.
> >
> > And we could also keep the -s/--sort option:
> >
> >  -s process,dso,self
> >
> > So the above -F line would be equivalent to:
> >
> >  -F total,self,process,dso,name -s process,dso,self
> >
> > What do you think?
> 
> I like the second one.  It can sustain the old way but can support the 
> new way easily.
>
> But for compatibility we need to use 'self' sort key internally iff 
> neither the -F option nor the config option was given by user.  And it 
> might warn (or notice) users to add 'self' column in the sort key for 
> future use.

Mind explaining what the problem here is? I don't think I get it.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists