Re: [PATCH] perf tools: fix: Force backward ring buffer mapped readonly
From: Wangnan (F)
Date: Thu Oct 12 2017 - 10:36:27 EST
On 2017/10/12 20:56, Liang, Kan wrote:
On 2017/10/11 21:16, Liang, Kan wrote:
perf record's --overwrite option doesn't work as we expect.
For example:
[SNIP]
In the above example we get same records from the backward ring
buffer all the time. Overwriting is not triggered.
This commit maps backward ring buffers readonly, make it overwritable.
It is safe because we assume backward ring buffer always overwritable
in other part of code.
After applying this patch:
$ ~/linux/tools/perf$ sudo ./perf record -m 4 -e raw_syscalls:*
-g -- overwrite \
--switch-output=1s --tail-synthesize dd
if=/dev/zero of=/dev/null
[SNIP]
Signed-off-by: Wang Nan <wangnan0@xxxxxxxxxx>
Cc: Liang Kan <kan.liang@xxxxxxxxx>
Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
Cc: Ingo Molnar <mingo@xxxxxxxxxx>
Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
Cc: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
Cc: Adrian Hunter <adrian.hunter@xxxxxxxxx>
Cc: Andi Kleen <ak@xxxxxxxxxxxxxxx>
Cc: Li Zefan <lizefan@xxxxxxxxxx>
---
tools/perf/util/evlist.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index c6c891e..a86b0d2 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -799,12 +799,14 @@ perf_evlist__should_poll(struct perf_evlist
*evlist __maybe_unused,
}
static int perf_evlist__mmap_per_evsel(struct perf_evlist *evlist, int idx,
- struct mmap_params *mp, int cpu_idx,
+ struct mmap_params *_mp, int cpu_idx,
int thread, int *_output, int
*_output_backward)
{
struct perf_evsel *evsel;
int revent;
int evlist_cpu = cpu_map__cpu(evlist->cpus, cpu_idx);
+ struct mmap_params *mp = _mp;
+ struct mmap_params backward_mp;
evlist__for_each_entry(evlist, evsel) {
struct perf_mmap *maps = evlist->mmap; @@ -815,6 +817,9
@@ static
int perf_evlist__mmap_per_evsel(struct
perf_evlist *evlist, int idx,
if (evsel->attr.write_backward) {
output = _output_backward;
maps = evlist->backward_mmap;
+ backward_mp = *mp;
+ backward_mp.prot &= ~PROT_WRITE;
+ mp = &backward_mp;
if (!maps) {
maps = perf_evlist__alloc_mmap(evlist);
So it's trying to support per-event overwrite.
How about the global --overwrite option?
Not only the per-event overwrite. See the example above. The overall --
overwrite option is also respected. In perf_evsel__config, per-event evsel
'backward' setting is set based on overall '--overwrite' and per-event
'/overwrite/' setting.
But how about evlist->overwrite? I think it still keeps the wrong setting.
The overwrite is implicitly applied. Some settings are inconsistent.
Is there any drawback if you use opts->overwrite for perf_evlist__mmap_ex?
We will always face such inconsistency, because we have
an /no-overwrite/ option which can be set per-evsel.
Setting evlist->overwrite won't make things more consistent,
because in a evlist, different evsel can have different
overwrite setting. A simple solution is making evlist
non-overwrite by default, and watch all overwrite evsels
a special cases. Then we have only 2 cases to consider:
1. overwrite evsel in a non-overwrite evlist.
2. non-overwrite evsel in a non-overwrite evlist.
If we reset evlist->overwrite according to --overwrite, we
will have 4 cases to consider:
1. overwrite evsel in a overwrite evlist.
2. non-overwrite evsel in a overwrite evlist.
3. overwrite evsel in a non-overwrite evlist.
4. non-overwrite evsel in a non-overwrite evlist.
The real problem is: there's 'overwrite' and 'backward'
concepts in our code, but these two concepts are neither
independent nor identical.
Thank you.
Thanks,
Kan
I think we should use opts->overwrite to replace the hard code 'false'
for perf_evlist__mmap_ex as well.
Thanks,
Kan