From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754577AbdKAMCh (ORCPT ); Wed, 1 Nov 2017 08:02:37 -0400 Received: from mail-pf0-f195.google.com ([209.85.192.195]:56100 "EHLO mail-pf0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754498AbdKAMCf (ORCPT ); Wed, 1 Nov 2017 08:02:35 -0400 X-Google-Smtp-Source: ABhQp+QDcjYCA9WhFBc7bRT+U6C8ciLKA0PEuFoA251VJ5D2O6PpHfSi/42b27ScjGbtY57+JXjRtA== Date: Wed, 1 Nov 2017 21:00:07 +0900 From: Namhyung Kim To: "Wangnan (F)" Cc: linux-kernel@vger.kernel.org, kan.liang@intel.com, acme@kernel.org, jolsa@redhat.com, kernel-team@lge.com Subject: Re: [PATCH 1/2] perf mmap: Fix perf backward recording Message-ID: <20171101120007.GA26623@danjae.aot.lge.com> References: <20171101055327.141281-1-wangnan0@huawei.com> <20171101055327.141281-2-wangnan0@huawei.com> <20171101094929.GB25146@danjae.aot.lge.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.1 (2017-09-22) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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/' > > Then why not removing mp->prot totally, and add a prot argument to > perf_mmap__mmap, > since prot setting is always decided independently? I'm ok with it. > > > 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. Right, and it's always "false" for recording in the current code. Thanks, Namhyung