Re: [PATCH 01/10] perf record: new interfaces to read ring buffer to file

[Arnaldo Carvalho de Melo]

Em Wed, Oct 11, 2017 at 04:12:42AM +0000, Liang, Kan escreveu:
> > >  /* When check_messup is true, 'end' must points to a good entry */
> > > static union perf_event *  perf_mmap__read(struct perf_mmap *md, bool
> > > check_messup, u64 start, diff --git a/tools/perf/util/evlist.h
> > > b/tools/perf/util/evlist.h index b1c14f1..1ce4857 100644
> > > --- a/tools/perf/util/evlist.h
> > > +++ b/tools/perf/util/evlist.h
> > > @@ -39,6 +39,16 @@ struct perf_mmap {
> > >  	char		 event_copy[PERF_SAMPLE_MAX_SIZE] __aligned(8);
> > >  };
> > >
> > > +struct perf_mmap_read {
> > > +	struct perf_mmap	*md;
> > > +	u64			head;
> > > +	u64			start;
> > > +	u64			end;
> > 
> > So there will be always a one-on-one association of 'struct perf_mmap_read'
> > and 'struct perf_mmap', why not go on adding more fields to 'struct
> > perf_mmap' as we need
> 
> The fields in 'struct perf_mmap' needs to be recalculated before each reading.
> So I put them in a new struct.  

Ok, but I still think that if there is a one on one relatioship of
perf_mmap_read with perf_mmap, then we should just extend the one we
already have for per-mmap operations, i.e. 'struct perf_mmap', I'll try
and provide a patch on top of my perf/core branch to see how it looks.
 
> > but not doing it all at once (backward, snapshotting,
> > overwrite, etc) but first the simple part, make the most basic mode:
> > 
> > 	perf record -a
> > 
> > 	perf top
> > 
> > work, multithreaded, leaving the other more complicated modes fallbacking
> > to the old format, then when we have it solid, go on getting the other
> > features.
> 
> Agree. 
> When I did perf top optimization, I also tried Namhyung's perf top multi-thread patch.
> https://lwn.net/Articles/667469/
> I think it may be a good start point.

I have to read that to understand why we need those indexes :-\
 
> I didn't work on his patch. Because the root cause of bad perf top performance
> is non overwrite mode, which generate lots of samples shortly. It exceeds KNL's
> computational capability. Multi-threading doesn't help much on this case.
> So I tried to use overwrite mode then.

Right, work on the problem you have at hand, but all these efforts
should be considered to move forward.
 
> > In the end, having the two formats supported will be needed anyway, and
> > we can as well ask for processing with both perf.data file formats to compare
> > results, while we strenghten out the new code.
> >
> > I just think we should do this in a more fine grained way to avoid too much
> > code churn as well as having a fallback to the old code, that albeit non
> > scalable, is what we have been using and can help in certifying that the new
> > one works well, by comparing its outputs.
> 
> I already extended the multithreading support for event synthesization in perf
> record. 
> https://github.com/kliang2/perf.git perf_record_opt
> I will send it out for review shortly after rebasing on the latest perf/core.
> 
> In the patch series, I realloc buffer for each thread to temporarily keep the
> processing result, and write them to the perf.data at the end of event
> synthesization. The number of synthesized event is not big (hundreds of
> Kilobyte). So I think it should be OK to do that.

Ok, one thing I noticed was that with the snapshotting code we
synthesize events multiple times, once per each new perf.data file, I
haven't tested that with the multithreaded synthesizing code we recently
merged, have you?

- Arnaldo