| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 12 Oct 2017 15:11:29 +0000
From: "Liang, Kan" <kan.liang@...el.com>
To: "Wangnan (F)" <wangnan0@...wei.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"acme@...nel.org" <acme@...nel.org>
CC: Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...nel.org>, Jiri Olsa <jolsa@...nel.org>,
Namhyung Kim <namhyung@...nel.org>,
"Alexander Shishkin" <alexander.shishkin@...ux.intel.com>,
"Hunter, Adrian" <adrian.hunter@...el.com>,
Andi Kleen <ak@...ux.intel.com>, Li Zefan <lizefan@...wei.com>
Subject: RE: [PATCH] perf tools: fix: Force backward ring buffer mapped
readonly
> > 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@...wei.com>
> >>>>>>> Cc: Liang Kan <kan.liang@...el.com>
> >>>>>>> Cc: Arnaldo Carvalho de Melo <acme@...nel.org>
> >>>>>>> Cc: Peter Zijlstra <peterz@...radead.org>
> >>>>>>> Cc: Ingo Molnar <mingo@...nel.org>
> >>>>>>> Cc: Jiri Olsa <jolsa@...nel.org>
> >>>>>>> Cc: Namhyung Kim <namhyung@...nel.org>
> >>>>>>> Cc: Alexander Shishkin <alexander.shishkin@...ux.intel.com>
> >>>>>>> Cc: Adrian Hunter <adrian.hunter@...el.com>
> >>>>>>> Cc: Andi Kleen <ak@...ux.intel.com>
> >>>>>>> Cc: Li Zefan <lizefan@...wei.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.
>
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.
>
Powered by blists - more mailing lists