Re: [PATCH 3/4] perf tool: Fix build when sysconf doesn't support cache line size

[Arnaldo Carvalho de Melo]

Em Mon, Jul 04, 2016 at 05:47:12PM -0700, Chris Phlipot escreveu:
> On 07/04/2016 05:26 PM, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Jul 04, 2016 at 05:19:20PM -0700, Chris Phlipot escreveu:
> > > On 07/04/2016 03:48 PM, Arnaldo Carvalho de Melo wrote:
> > > > Em Thu, Jun 30, 2016 at 10:12:34PM -0700, Chris Phlipot escreveu:
> > > > > Enable perf to build on libc implementations where sysconf() doesn't
> > > > > support _SC_LEVEL1_DCACHE_LINESIZE as a parameter.
> > > > > 
> > > > > For example, the Bionic implementation does not support this as a
> > > > > paremter. Older versions of Bionic will throw an error when this is passed
> > > > > in as a parameter, and more recent versions will just return 0 as the
> > > > > cache line size.
> > > > > 
> > > > > Signed-off-by: Chris Phlipot <cphlipot0@xxxxxxxxx>
> > > > > ---
> > > > >    tools/perf/perf.c | 4 ++++
> > > > >    1 file changed, 4 insertions(+)
> > > > > 
> > > > > diff --git a/tools/perf/perf.c b/tools/perf/perf.c
> > > > > index 8f21922..113ca5b 100644
> > > > > --- a/tools/perf/perf.c
> > > > > +++ b/tools/perf/perf.c
> > > > > @@ -509,7 +509,11 @@ int main(int argc, const char **argv)
> > > > > 
> > > > >    	/* The page_size is placed in util object. */
> > > > >    	page_size = sysconf(_SC_PAGE_SIZE);
> > > > > +#ifdef _SC_LEVEL1_DCACHE_LINESIZE
> > > > >    	cacheline_size = sysconf(_SC_LEVEL1_DCACHE_LINESIZE);
> > > > > +#else
> > > > > +	cacheline_size = 0;
> > > > > +#endif
> > > > 
> > > > Couldn't we instead fallback to:
> > > > 
> > > > sysfs__read_int("devices/system/cpu/cpu0/cache/index0/coherency_line_size", &cacheline_size)
> > > > 
> > > > ?
> > > 
> > > I agree that in general this would be a better fallback, but in  all Android
> > > images I have tested so far, "devices/system/cpu/cpu0/cache" does not exist.
> > > I know not know of a good way to retrieve cache line size in this case.
> > > 
> > > I would be ok with attempting to get cacheline size using using the
> > > following methods, unless you have other ideas:
> > > 
> > > 1. attempt to use sysconf(_SC_LEVEL1_DCACHE_LINESIZE)
> > > 2. attempt to use
> > > sysfs__read_int("devices/system/cpu/cpu0/cache/index0/coherency_line_size",
> > > &cacheline_size)
> > > 3. set to zero if both of the above fail.
> > 
> > Ok, but perhaps we should have some sort of warning in places using
> > this?
> 
> Such as printing a warning when cacheline_size is set to zero, or simply
> adding comments to the code in areas where cacheline_size is used?

So:

[acme@jouet linux]$ find tools/perf -name "*.[ch]" | xargs grep cacheline_size
tools/perf/perf.c:	cacheline_size = sysconf(_SC_LEVEL1_DCACHE_LINESIZE);
tools/perf/util/util.c:int cacheline_size;
tools/perf/util/util.h:extern int cacheline_size;
tools/perf/util/sort.h:	return (address & ~(cacheline_size - 1));
tools/perf/util/sort.h:	return (address & (cacheline_size - 1));
[acme@jouet linux]$ 

So it seems this is used by some of the sort keys, i.e. if the user
uses some of:

[acme@jouet linux]$ perf report -h -s

 Usage: perf report [<options>]

    -s, --sort <key[,key2...]>
                          sort by key(s): pid, comm, dso, symbol,
parent, cpu, srcline, ... Please refer the man page for the complete
list.

[acme@jouet linux]$ 

At least this one:

	   ·   dcacheline: the cacheline the data address is on at the
time of the sample

There may be others, need to thoroughly check.

I.e. if "dcacheline" is in -s/--sort, this needs to fail and the user be
informed that it is not possible, perhaps we should add a --cacheline
option to allow the user to tell the tool what is the cacheline size?

What is the usual cacheline size for Android class devices? Perhaps we
should fallback to that and give a warning?

- Arnaldo