Re: [PATCH 1/2] perf mmap: Fix perf backward recording

[Wangnan (F)]

On 2017/11/1 20:39, Jiri Olsa wrote:

On Wed, Nov 01, 2017 at 08:10:49PM +0800, Wangnan (F) wrote:

SNIP

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index c6c891e154a6..27ebe355e794 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -838,6 +838,11 @@ static int perf_evlist__mmap_per_evsel(struct perf_evlist 
*evlist, int idx,
                   if (*output == -1) {
                           *output = fd;
+                       if (evsel->attr.write_backward)
+                               mp->prot = PROT_READ;
+                       else
+                               mp->prot = PROT_READ | PROT_WRITE;
+

If evlist->overwrite is true, PROT_WRITE should be unset even if
write_backward is
not set. If you want to delay the setting of mp->prot, you need to consider
both evlist->overwrite and evsel->attr.write_backward.

I thought evsel->attr.write_backward should be set when
evlist->overwrite is set.  Do you mean following case?

    perf record --overwrite -e 'cycles/no-overwrite/'

No. evlist->overwrite is unrelated to '--overwrite'. This is why I
said the concept of 'overwrite' and 'backward' is ambiguous.

perf record never sets 'evlist->overwrite'. What '--overwrite' actually
does is setting write_backward. Some testcases needs overwrite evlist.

did not check deeply, but so why can't we do the below?

jirka


---
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index f4d9fc54b382..4643c487bd29 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -300,7 +300,7 @@ static int record__mmap_evlist(struct record *rec,
        struct record_opts *opts = &rec->opts;
        char msg[512];

- if (perf_evlist__mmap_ex(evlist, opts->mmap_pages, false,

+       if (perf_evlist__mmap_ex(evlist, opts->mmap_pages, opts->overwrite,
                                 opts->auxtrace_mmap_pages,
                                 opts->auxtrace_snapshot_mode) < 0) {

perf_evlist__mmap_ex's overwrite argument is used to control evlist->mmap.

Consider Namhyung's example:

  perf record --overwrite -e 'cycles/no-overwrite/'

In this case both evlist->mmap and evlist->backward_mmap would be set to 
overwrite.
'cycles' will be put into evlist->mmap, so it will be set to overwrite 
incorrectly.

Patch 2/2 clarifies these concepts. What we want is the flight recorder mode,
not only a read only ring buffer. Even flight recorder mode is selected, we can
still have a normal ring buffer which keep output data.

Thank you.