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

Arnaldo Carvalho de Melo
2018-08-23T14:30:11Z

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