Re: [PATCH v1 1/2]: perf util: map data buffer for preserving collected data

[Alexey Budankov]

Hi Arnaldo,

On 23.08.2018 17:30, Arnaldo Carvalho de Melo wrote:
> Em Thu, Aug 23, 2018 at 01:30:47PM +0300, Alexey Budankov escreveu:
>> +++ b/tools/perf/util/evlist.c
>> @@ -718,6 +718,8 @@ static void perf_evlist__munmap_nofree(struct perf_evlist *evlist)
>>  void perf_evlist__munmap(struct perf_evlist *evlist)
>>  {
>>  	perf_evlist__munmap_nofree(evlist);
>> +	if (&evlist->mmap_aio)
> 
> Is the above test for evlist->mmap_aio being NULL? I think that '&' is
> not needed here, with it this test will always be true, right?

Right, looks like a typo.

> 
>> +		zfree(&evlist->mmap_aio);
> 
>>  	zfree(&evlist->mmap);
>>  	zfree(&evlist->overwrite_mmap);
>>  }
>> @@ -749,6 +751,13 @@ static struct perf_mmap *perf_evlist__alloc_mmap(struct perf_evlist *evlist,
>>  		 */
>>  		refcount_set(&map[i].refcnt, 0);
>>  	}
>> +
>> +	evlist->mmap_aio = zalloc(evlist->nr_mmaps * sizeof(struct aiocb *));
> 
> Right, and here you could have used calloc(evlist->nr_mmaps, sizeof(struct aiocb *))

yep, make sense.

> 
>> +	if (!evlist->mmap_aio) {
>> +		zfree(&map);
> 
> If you use zfree(&map); then map becomes NULL and you do not return NULL
> in the next line, if you insist in using the following 'return NULL;',
> then you could as well use just 'free(map);', as 'map' is a local
> variable and thus we need not set it to NULL :-)

yeah, I see. Let me resend. Thanks!

> 
>> +		return NULL;
>> +	}
>> +
>>  	return map;
>>  }
>>  
>> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
>> index dc66436add98..f98b949561fd 100644
>> --- a/tools/perf/util/evlist.h
>> +++ b/tools/perf/util/evlist.h
>> @@ -15,6 +15,7 @@
>>  #include "util.h"
>>  #include <signal.h>
>>  #include <unistd.h>
>> +#include <aio.h>
>>  
>>  struct pollfd;
>>  struct thread_map;
>> @@ -43,6 +44,7 @@ struct perf_evlist {
>>  	} workload;
>>  	struct fdarray	 pollfd;
>>  	struct perf_mmap *mmap;
>> +	struct aiocb	 **mmap_aio;
>>  	struct perf_mmap *overwrite_mmap;
>>  	struct thread_map *threads;
>>  	struct cpu_map	  *cpus;
>> diff --git a/tools/perf/util/mmap.c b/tools/perf/util/mmap.c
>> index fc832676a798..e71d46cb01cc 100644
>> --- a/tools/perf/util/mmap.c
>> +++ b/tools/perf/util/mmap.c
>> @@ -155,6 +155,10 @@ void __weak auxtrace_mmap_params__set_idx(struct auxtrace_mmap_params *mp __mayb
>>  
>>  void perf_mmap__munmap(struct perf_mmap *map)
>>  {
>> +	if (map->data != NULL) {
>> +		munmap(map->data, perf_mmap__mmap_len(map));
>> +		map->data = NULL;
>> +	}
>>  	if (map->base != NULL) {
>>  		munmap(map->base, perf_mmap__mmap_len(map));
>>  		map->base = NULL;
>> @@ -190,6 +194,14 @@ int perf_mmap__mmap(struct perf_mmap *map, struct mmap_params *mp, int fd)
>>  		map->base = NULL;
>>  		return -1;
>>  	}
>> +	map->data = mmap(NULL, perf_mmap__mmap_len(map), PROT_READ | PROT_WRITE,
>> +			MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>> +	if (map->data == MAP_FAILED) {
>> +		pr_debug2("failed to mmap perf event data buffer, error %d\n",
>> +				errno);
>> +		map->data = NULL;
>> +		return -1;
>> +	}
>>  	map->fd = fd;
>>  
>>  	if (auxtrace_mmap__mmap(&map->auxtrace_mmap,
>> diff --git a/tools/perf/util/mmap.h b/tools/perf/util/mmap.h
>> index d82294db1295..1974e621e36b 100644
>> --- a/tools/perf/util/mmap.h
>> +++ b/tools/perf/util/mmap.h
>> @@ -6,6 +6,7 @@
>>  #include <linux/types.h>
>>  #include <asm/barrier.h>
>>  #include <stdbool.h>
>> +#include <aio.h>
>>  #include "auxtrace.h"
>>  #include "event.h"
>>  
>> @@ -25,6 +26,8 @@ struct perf_mmap {
>>  	bool		 overwrite;
>>  	struct auxtrace_mmap auxtrace_mmap;
>>  	char		 event_copy[PERF_SAMPLE_MAX_SIZE] __aligned(8);
>> +	void 		 *data;
>> +	struct aiocb	 cblock;
>>  };
>>  
>>  /*
>>  
>