| 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
| ||
|
Message-ID: <37D7C6CF3E00A74B8858931C1DB2F077537DC3EF@SHSMSX103.ccr.corp.intel.com>
Date: Wed, 1 Nov 2017 16:22:53 +0000
From: "Liang, Kan" <kan.liang@...el.com>
To: "Wangnan (F)" <wangnan0@...wei.com>,
Namhyung Kim <namhyung@...nel.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"acme@...nel.org" <acme@...nel.org>,
"jolsa@...hat.com" <jolsa@...hat.com>,
"kernel-team@....com" <kernel-team@....com>
Subject: RE: [PATCH 1/2] perf mmap: Fix perf backward recording
> On 2017/11/1 21:57, Liang, Kan wrote:
> >> On 2017/11/1 20:00, Namhyung Kim wrote:
> >>> On Wed, Nov 01, 2017 at 06:32:50PM +0800, Wangnan (F) wrote:
> >>>> On 2017/11/1 17:49, Namhyung Kim wrote:
> >>>>> Hi,
> >>>>>
> >>>>> On Wed, Nov 01, 2017 at 05:53:26AM +0000, Wang Nan wrote:
> >>>>>> perf record backward recording doesn't work as we expected: it
> >>>>>> never overwrite when ring buffer full.
> >>>>>>
> >>>>>> Test:
> >>>>>>
> >>>>>> (Run a busy printing python task background like this:
> >>>>>>
> >>>>>> while True:
> >>>>>> print 123
> >>>>>>
> >>>>>> send SIGUSR2 to perf to capture snapshot.)
> >>>> [SNIP]
> >>>>
> >>>>>> Signed-off-by: Wang Nan <wangnan0@...wei.com>
> >>>>>> ---
> >>>>>> tools/perf/util/evlist.c | 8 +++++++-
> >>>>>> 1 file changed, 7 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> >>>>>> index c6c891e..4c5daba 100644
> >>>>>> --- a/tools/perf/util/evlist.c
> >>>>>> +++ b/tools/perf/util/evlist.c
> >>>>>> @@ -799,22 +799,28 @@ 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;
> >>>>>> evlist__for_each_entry(evlist, evsel) {
> >>>>>> struct perf_mmap *maps = evlist->mmap;
> >>>>>> + struct mmap_params rdonly_mp;
> >>>>>> int *output = _output;
> >>>>>> int fd;
> >>>>>> int cpu;
> >>>>>> + mp = _mp;
> >>>>>> if (evsel->attr.write_backward) {
> >>>>>> output = _output_backward;
> >>>>>> maps = evlist->backward_mmap;
> >>>>>> + rdonly_mp = *_mp;
> >>>>>> + rdonly_mp.prot &= ~PROT_WRITE;
> >>>>>> + mp = &rdonly_mp;
> >>>>>> if (!maps) {
> >>>>>> maps =
> perf_evlist__alloc_mmap(evlist);
> >>>>>> --
> >>>>> What about this instead (not tested)?
> >>>>>
> >>>>>
> >>>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> >>>>> index c6c891e154a6..27ebe355e794 100644
> >>>>> --- a/tools/perf/util/evlist.c
> >>>>> +++ b/tools/perf/util/evlist.c
> >>>>> @@ -838,6 +838,11 @@ static int
> perf_evlist__mmap_per_evsel(struct
> >> perf_evlist *evlist, int idx,
> >>>>> if (*output == -1) {
> >>>>> *output = fd;
> >>>>> + if (evsel->attr.write_backward)
> >>>>> + mp->prot = PROT_READ;
> >>>>> + else
> >>>>> + mp->prot = PROT_READ | PROT_WRITE;
> >>>>> +
> >>>> If evlist->overwrite is true, PROT_WRITE should be unset even if
> >>>> write_backward is not set. If you want to delay the setting of
> >>>> mp->prot, you need to consider both evlist->overwrite and
> >>>> evsel->attr.write_backward.
> >>> I thought evsel->attr.write_backward should be set when
> >>> evlist->overwrite is set. Do you mean following case?
> >>>
> >>> perf record --overwrite -e 'cycles/no-overwrite/'
> >>>
> >> No. evlist->overwrite is unrelated to '--overwrite'. This is why I
> >> said the concept of 'overwrite' and 'backward' is ambiguous.
> >>
> > Yes, I think we should make it clear.
> >
> > As we discussed previously, there are four possible combinations to
> > access ring buffer , 'forward non-overwrite', 'forward overwrite',
> > 'backward non-overwrite' and 'backward overwrite'.
> >
> > Actually, not all of the combinations are necessary.
> > - 'forward overwrite' mode brings some problems which were mentioned
> > in commit ID 9ecda41acb97 ("perf/core: Add ::write_backward attribute
> > to perf event").
> > - 'backward non-overwrite' mode is very similar as 'forward non-overwrite'.
> > There is no extra advantage. Only keep one non-overwrite mode is
> enough.
> > So 'forward non-overwrite' and 'backward overwrite' are enough for all
> perf tools.
> >
> > Furthermore, 'forward' and 'backward' only indicate the direction of
> > the ring buffer. They don't impact the result and performance. It is
> > not important as the concept of overwrite/non-overwrite.
> >
> > To simplify the concept, only 'non-overwrite' mode and 'overwrite'
> > mode should be kept. 'non-overwrite' mode indicates the forward ring
> > buffer. 'overwrite' mode indicates the backward ring buffer.
> >
> >> perf record never sets 'evlist->overwrite'. What '--overwrite'
> >> actually does is setting write_backward. Some testcases needs overwrite
> evlist.
> >>
> > There are only four test cases which set overwrite,
> > sw-clock,task-exit, mmap-basic, backward-ring-buffer.
> > Only backward-ring-buffer is 'backward overwrite'.
> > The rest three are all 'forward overwrite'. We just need to set
> > write_backward to convert them to 'backward overwrite'.
> > I think it's not hard to clean up.
>
> If we add a new rule that overwrite ring buffers are always backward then it
> is not hard to cleanup. However, the support of forward overwrite ring
> buffer has a long history and the code is not written by me. I'd like to check if
> there is some reason to keep support this configuration?
>
As my observation, currently, there are no perf tools which support
'forward overwrite'.
There are only three test cases (sw-clock, task-exit, mmap-basic) which
is set to 'forward overwrite'. I don’t see any reason it cannot be changed to
'backward overwrite'
Arnaldo? Jirka? Kim?
What do you think?
Thanks,
Kan
Powered by blists - more mailing lists