From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933164AbdKAQW7 (ORCPT ); Wed, 1 Nov 2017 12:22:59 -0400 Received: from mga04.intel.com ([192.55.52.120]:11216 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932937AbdKAQW5 (ORCPT ); Wed, 1 Nov 2017 12:22:57 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.44,329,1505804400"; d="scan'208";a="330638719" From: "Liang, Kan" To: "Wangnan (F)" , Namhyung Kim CC: "linux-kernel@vger.kernel.org" , "acme@kernel.org" , "jolsa@redhat.com" , "kernel-team@lge.com" Subject: RE: [PATCH 1/2] perf mmap: Fix perf backward recording Thread-Topic: [PATCH 1/2] perf mmap: Fix perf backward recording Thread-Index: AQHTUtXZD2mToCc3eE2DfG6L0BK9oqL+waOAgAAMHQCAABhjgIAAAv2AgACe3VD//6SfgIAAhoEg Date: Wed, 1 Nov 2017 16:22:53 +0000 Message-ID: <37D7C6CF3E00A74B8858931C1DB2F077537DC3EF@SHSMSX103.ccr.corp.intel.com> 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> <226626ec-4f96-d3f4-a6d5-62c17c897f32@huawei.com> In-Reply-To: <226626ec-4f96-d3f4-a6d5-62c17c897f32@huawei.com> Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZWM1NDk2NmYtYzM3YS00NGRhLTkzYTEtMWExYjM3NDczNWQ3IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6InBoRlFnZUZNOUZyQVVBdk56QWlTSTB0Y2ZlVlBxQmhwWU5yN1wvVUw4SVNNPSJ9 x-ctpclassification: CTP_IC dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by nfs id vA1GN3m4011600 > 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? > 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