2017-11-01 12:12:28

by Wang Nan

[permalink] [raw]
Subject: Re: [PATCH 1/2] perf mmap: Fix perf backward recording



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 <[email protected]>
>>>> ---
>>>> 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.

perf record never sets 'evlist->overwrite'. What '--overwrite' actually
does is setting write_backward. Some testcases needs overwrite evlist.

Thank you.


From 1582865126844177912@xxx Wed Nov 01 12:03:37 +0000 2017
X-GM-THRID: 1582841972981207976
X-Gmail-Labels: Inbox,Category Forums