From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754557AbdKAN5w (ORCPT ); Wed, 1 Nov 2017 09:57:52 -0400 Received: from mga02.intel.com ([134.134.136.20]:5966 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752435AbdKAN5u (ORCPT ); Wed, 1 Nov 2017 09:57:50 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.44,329,1505804400"; d="scan'208";a="1212837627" 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+waOAgAAMHQCAABhjgIAAAv2AgACe3VA= Date: Wed, 1 Nov 2017 13:57:46 +0000 Message-ID: <37D7C6CF3E00A74B8858931C1DB2F077537DC1FA@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> In-Reply-To: <109f02ef-5dc2-94f9-e850-572c498781ee@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 vA1Dw7lO003573 > 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. Thanks, Kan