Re: [PATCH 3/5] perf tool: Introducing perf_mmap object

[Jiri Olsa <jolsa@redhat.com>]

On Fri, Nov 18, 2011 at 12:37:40PM -0200, Arnaldo Carvalho de Melo wrote:
> Em Fri, Nov 18, 2011 at 02:46:43PM +0100, Jiri Olsa escreveu:
> > +++ b/tools/perf/builtin-test.c
> > @@ -476,6 +477,7 @@ static int test__basic_mmap(void)
> >  		     expected_nr_events[nsyscalls], i, j;
> >  	struct perf_evsel *evsels[nsyscalls], *evsel;
> >  	int sample_size = __perf_evsel__sample_size(attr.sample_type);
> > +	struct perf_mmap *md;
> >  
> >  	for (i = 0; i < nsyscalls; ++i) {
> >  		char name[64];
> > @@ -551,7 +553,9 @@ static int test__basic_mmap(void)
> >  			++foo;
> >  		}
> >  
> > -	while ((event = perf_evlist__mmap_read(evlist, 0)) != NULL) {
> > +	md = &evlist->mmap[0];
> > +
> > +	while ((event = perf_mmap__read(md)) != NULL) {
> >  		struct perf_sample sample;
> >  
> >  		if (event->header.type != PERF_RECORD_SAMPLE) {
> 
> Please keep perf_evlist__mmap_read() as just a wrapper for the above
> operation, that way you reduce the patch size by not touching the
> functions that use perf_evlist__mmap_read().
> 
> Later, if we think this is the right thing to do, i.e. to open code it
> like you're doing above, we can elliminate it, but I think its better to
> keep it as perf_evlist__mmap_read(evlist, 0) as it uses just one line
> instead of the 4 above :-)

right, I'll sent v2 shortly

> 
> > diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> > index 8e02027..032f70d 100644
> > --- a/tools/perf/builtin-top.c
> > +++ b/tools/perf/builtin-top.c
> > @@ -38,6 +38,7 @@

SNIP

> >  void perf_evlist__munmap(struct perf_evlist *evlist)
> >  {
> >  	int i;
> >  
> >  	for (i = 0; i < evlist->nr_mmaps; i++) {
> > -		if (evlist->mmap[i].base != NULL) {
> > -			munmap(evlist->mmap[i].base, evlist->mmap_len);
> > -			evlist->mmap[i].base = NULL;
> > -		}
> > +		struct perf_mmap *m = &evlist->mmap[i];
> > +		if (m->base != NULL)
> > +			perf_mmap__close(m);
> 
> Wouldn't it be perf_mmap__munmap() ? 
> 
> >  	}
> >  
> >  	free(evlist->mmap);
> > @@ -292,20 +225,18 @@ int perf_evlist__alloc_mmap(struct perf_evlist *evlist)
> >  }
> >  
> >  static int __perf_evlist__mmap(struct perf_evlist *evlist,
> > -			       int idx, int prot, int mask, int fd)
> > +			       int idx, int fd)
> >  {
> > -	evlist->mmap[idx].prev = 0;
> > -	evlist->mmap[idx].mask = mask;
> > -	evlist->mmap[idx].base = mmap(NULL, evlist->mmap_len, prot,
> > -				      MAP_SHARED, fd, 0);
> > -	if (evlist->mmap[idx].base == MAP_FAILED)
> > +	struct perf_mmap *m = &evlist->mmap[idx];
> > +
> > +	if (perf_mmap__open(m, fd, evlist->overwrite, evlist->pages))
> 
> And here perf_mmap__mmap or at least perf_mmap__map and the other
> perf_mmap__unmap?

not sure, perf_mmap__open and perf_mmap__close actually does map/umap..
maybe we can rename them ;)