Re: [RFC/PATCH 02/38] perf tools: Save mmap_param.len instead of mask

[Namhyung Kim]

Hi Arnaldo,

On Fri, Oct 02, 2015 at 03:44:33PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Oct 02, 2015 at 02:18:43PM +0900, Namhyung Kim escreveu:
> > It is more convenient saving mmap length rather than (bit) mask.  With
> > this patch, we can eliminate dependency to perf_evlist other than
> > getting mmap_desc for dealing with mmaps.  The mask and length can be
> > converted using perf_evlist__mmap_mask/len().
> > 
> > Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> > Cc: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> > Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> > ---
> >  tools/perf/util/evlist.c | 31 +++++++++++++++++++++++++------
> >  1 file changed, 25 insertions(+), 6 deletions(-)
> > 
> > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> > index c5180a29db1b..e46adcd5b408 100644
> > --- a/tools/perf/util/evlist.c
> > +++ b/tools/perf/util/evlist.c
> > @@ -29,6 +29,8 @@
> >  
> >  static void perf_evlist__mmap_put(struct perf_evlist *evlist, int idx);
> >  static void __perf_evlist__munmap(struct perf_evlist *evlist, int idx);
> > +static size_t perf_evlist__mmap_mask(size_t len);
> > +static size_t perf_evlist__mmap_len(size_t mask);
> 
> Are these "perf_evlist" methods? I don't think so, those are related to
> "perf_mmap".

Agreed.

> 
> >  #define FD(e, x, y) (*(int *)xyarray__entry(e->fd, x, y))
> >  #define SID(e, x, y) xyarray__entry(e->sample_id, x, y)
> > @@ -871,7 +873,9 @@ void __weak auxtrace_mmap_params__set_idx(
> >  static void __perf_evlist__munmap(struct perf_evlist *evlist, int idx)
> >  {
> >  	if (evlist->mmap[idx].base != NULL) {
> > -		munmap(evlist->mmap[idx].base, evlist->mmap_len);
> > +		size_t mmap_len = perf_evlist__mmap_len(evlist->mmap[idx].mask);
> 
> I.e. here you could have it as:
> 
> 		size_t mmap_len = perf_mmap__len(evlist->mmap[idx]);

OK.

> 
> > +
> > +		munmap(evlist->mmap[idx].base, mmap_len);
> >  		evlist->mmap[idx].base = NULL;
> >  		atomic_set(&evlist->mmap[idx].refcnt, 0);
> >  	}
> > @@ -901,8 +905,8 @@ static int perf_evlist__alloc_mmap(struct perf_evlist *evlist)
> >  }
> >  
> >  struct mmap_params {
> > -	int prot;
> > -	int mask;
> > +	int	prot;
> > +	size_t	len;
> >  	struct auxtrace_mmap_params auxtrace_mp;
> >  };
> >  
> > @@ -924,8 +928,8 @@ static int __perf_evlist__mmap(struct perf_evlist *evlist, int idx,
> >  	 */
> >  	atomic_set(&evlist->mmap[idx].refcnt, 2);
> >  	evlist->mmap[idx].prev = 0;
> > -	evlist->mmap[idx].mask = mp->mask;
> > -	evlist->mmap[idx].base = mmap(NULL, evlist->mmap_len, mp->prot,
> > +	evlist->mmap[idx].mask = perf_evlist__mmap_mask(mp->len);
> 
> Here, since you're not using a perf_mmap instance, but the calculation
> is relative to a perf_mmap property, we would use:
> 
> 	evlist->mmap[idx].mask = __perf_mmap__mask(mp->len);

Will change.

Thanks,
Namhyung


> 	
> 
> > +	evlist->mmap[idx].base = mmap(NULL, mp->len, mp->prot,
> >  				      MAP_SHARED, fd, 0);
> >  	if (evlist->mmap[idx].base == MAP_FAILED) {
> >  		pr_debug2("failed to mmap perf event ring buffer, error %d\n",
> > @@ -1071,6 +1075,21 @@ static size_t perf_evlist__mmap_size(unsigned long pages)
> >  	return (pages + 1) * page_size;
> >  }
> >  
> > +static size_t perf_evlist__mmap_mask(size_t len)
> > +{
> > +	BUG_ON(len <= page_size);
> > +	BUG_ON((len % page_size) != 0);
> > +
> > +	return len - page_size - 1;
> > +}
> > +
> > +static size_t perf_evlist__mmap_len(size_t mask)
> > +{
> > +	BUG_ON(((mask + 1) % page_size) != 0);
> > +
> > +	return mask + 1 + page_size;
> > +}
> > +
> >  static long parse_pages_arg(const char *str, unsigned long min,
> >  			    unsigned long max)
> >  {
> > @@ -1176,7 +1195,7 @@ int perf_evlist__mmap_ex(struct perf_evlist *evlist, unsigned int pages,
> >  	evlist->overwrite = overwrite;
> >  	evlist->mmap_len = perf_evlist__mmap_size(pages);
> >  	pr_debug("mmap size %zuB\n", evlist->mmap_len);
> > -	mp.mask = evlist->mmap_len - page_size - 1;
> > +	mp.len = evlist->mmap_len;
> >  
> >  	auxtrace_mmap_params__init(&mp.auxtrace_mp, evlist->mmap_len,
> >  				   auxtrace_pages, auxtrace_overwrite);
> > -- 
> > 2.6.0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/