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

["Wangnan (F)" <wangnan0@huawei.com>]

On 2017/10/12 22:46, Wangnan (F) wrote:
>
>
> 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@huawei.com>
>>>>>>> Cc: Liang Kan <kan.liang@intel.com>
>>>>>>> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
>>>>>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>>>>>> Cc: Ingo Molnar <mingo@kernel.org>
>>>>>>> Cc: Jiri Olsa <jolsa@kernel.org>
>>>>>>> Cc: Namhyung Kim <namhyung@kernel.org>
>>>>>>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>>>>>>> Cc: Adrian Hunter <adrian.hunter@intel.com>
>>>>>>> Cc: Andi Kleen <ak@linux.intel.com>
>>>>>>> Cc: Li Zefan <lizefan@huawei.com>
>>>>>>> ---
>>>>>>>     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.

Thank you.

> Thank you.