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

[Arnaldo Carvalho de Melo]

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?

> +		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 *))

> +	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 :-)

> +		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;
>  };
>  
>  /*
>