RE: [PATCH] perf tools: fix: Force backward ring buffer mapped readonly

[Liang, Kan]

> > On 2017/10/12 22:45, Liang, Kan wrote:
> >>> 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 evlist->overwrite is always non-overwrite, why not remove it?
> >
> > Some testcases require it.
> >
> 
> Sorry. I think removing it is reasonable now, but we need to solve
> the relationship between overwrite and backward first. I suggest remove
> the whole 'backward' concept, and makes evsels backward if it is
> overwrite. Is there any usecases that:
> 1. overwrite but not backward ring buffer: it will be unparsable after
> ring buffer full.
> 2. backward but not overwrite ring buffer: I don't see any advantage.
>

I agree.
I think we care more about the overwrite or non-overwrite.
It doesn't matter it is backward or forward unless it brings issues.

Thanks,
Kan
 
> Thank you.
> 
> > Thank you.
>