From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933069AbdKAQMo (ORCPT ); Wed, 1 Nov 2017 12:12:44 -0400 Received: from szxga06-in.huawei.com ([45.249.212.32]:39735 "EHLO huawei.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S932481AbdKAQMn (ORCPT ); Wed, 1 Nov 2017 12:12:43 -0400 Subject: Re: [PATCH 1/2] perf mmap: Fix perf backward recording To: "Liang, Kan" , Namhyung Kim References: <20171101055327.141281-1-wangnan0@huawei.com> <20171101055327.141281-2-wangnan0@huawei.com> <20171101094929.GB25146@danjae.aot.lge.com> <20171101120007.GA26623@danjae.aot.lge.com> <109f02ef-5dc2-94f9-e850-572c498781ee@huawei.com> <37D7C6CF3E00A74B8858931C1DB2F077537DC1FA@SHSMSX103.ccr.corp.intel.com> CC: "linux-kernel@vger.kernel.org" , "acme@kernel.org" , "jolsa@redhat.com" , "kernel-team@lge.com" From: "Wangnan (F)" Message-ID: <226626ec-4f96-d3f4-a6d5-62c17c897f32@huawei.com> Date: Thu, 2 Nov 2017 00:12:21 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <37D7C6CF3E00A74B8858931C1DB2F077537DC1FA@SHSMSX103.ccr.corp.intel.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.111.194.139] X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>>>>> --- >>>>>> 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? Thank you.