lkml.org 
[lkml]   [2011]   [Nov]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 3/5] perf tool: Introducing perf_mmap object
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 ;)


\
 
 \ /
  Last update: 2011-11-19 19:43    [from the cache]
©2003-2011 Jasper Spaans