From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932128AbdKAKdG (ORCPT ); Wed, 1 Nov 2017 06:33:06 -0400 Received: from szxga04-in.huawei.com ([45.249.212.190]:9925 "EHLO szxga04-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752058AbdKAKdF (ORCPT ); Wed, 1 Nov 2017 06:33:05 -0400 Subject: Re: [PATCH 1/2] perf mmap: Fix perf backward recording To: Namhyung Kim References: <20171101055327.141281-1-wangnan0@huawei.com> <20171101055327.141281-2-wangnan0@huawei.com> <20171101094929.GB25146@danjae.aot.lge.com> CC: , , , , From: "Wangnan (F)" Message-ID: Date: Wed, 1 Nov 2017 18:32:50 +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: <20171101094929.GB25146@danjae.aot.lge.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 X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020202.59F9A2DF.00A8,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2014-11-16 11:51:01, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 0d161d64bdde9e4f8b274cbbea18d420 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. Then why not removing mp->prot totally, and add a prot argument to perf_mmap__mmap, since prot setting is always decided independently? > if (perf_mmap__mmap(&maps[idx], mp, *output) < 0) > return -1; > } else { > @@ -1058,9 +1063,7 @@ int perf_evlist__mmap_ex(struct perf_evlist *evlist, unsigned int pages, > struct perf_evsel *evsel; > const struct cpu_map *cpus = evlist->cpus; > const struct thread_map *threads = evlist->threads; > - struct mmap_params mp = { > - .prot = PROT_READ | (overwrite ? 0 : PROT_WRITE), > - }; > + struct mmap_params mp; > > if (!evlist->mmap) > evlist->mmap = perf_evlist__alloc_mmap(evlist); The 'overwrite' argument in perf_evlist__mmap_ex() controls evlist->overwrite. Thank you.