lkml.org 
[lkml]   [2017]   [Oct]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] perf tools: fix: Force backward ring buffer mapped readonly
From
Date


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.


\
 
 \ /
  Last update: 2017-10-12 16:58    [W:0.174 / U:0.180 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site