From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933824AbdKBPMM (ORCPT ); Thu, 2 Nov 2017 11:12:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42668 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932724AbdKBPML (ORCPT ); Thu, 2 Nov 2017 11:12:11 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 3020CC0587E1 Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=jolsa@redhat.com Date: Thu, 2 Nov 2017 16:12:08 +0100 From: Jiri Olsa To: "Wangnan (F)" Cc: Namhyung Kim , linux-kernel@vger.kernel.org, kan.liang@intel.com, acme@kernel.org, kernel-team@lge.com Subject: Re: [PATCH 1/2] perf mmap: Fix perf backward recording Message-ID: <20171102151208.GB19184@krava> 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> <20171101123957.GA27215@krava> <4eeabf43-4de0-467c-04b0-85217040b036@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4eeabf43-4de0-467c-04b0-85217040b036@huawei.com> User-Agent: Mutt/1.9.1 (2017-09-22) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Thu, 02 Nov 2017 15:12:11 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Nov 01, 2017 at 08:56:32PM +0800, Wangnan (F) wrote: > > > On 2017/11/1 20:39, Jiri Olsa wrote: > > On Wed, Nov 01, 2017 at 08:10:49PM +0800, Wangnan (F) wrote: > > > > SNIP > > > > > > > > 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. > > > > > > perf record never sets 'evlist->overwrite'. What '--overwrite' actually > > > does is setting write_backward. Some testcases needs overwrite evlist. > > did not check deeply, but so why can't we do the below? > > > > jirka > > > > > > --- > > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c > > index f4d9fc54b382..4643c487bd29 100644 > > --- a/tools/perf/builtin-record.c > > +++ b/tools/perf/builtin-record.c > > @@ -300,7 +300,7 @@ static int record__mmap_evlist(struct record *rec, > > struct record_opts *opts = &rec->opts; > > char msg[512]; > > - if (perf_evlist__mmap_ex(evlist, opts->mmap_pages, false, > > + if (perf_evlist__mmap_ex(evlist, opts->mmap_pages, opts->overwrite, > > opts->auxtrace_mmap_pages, > > opts->auxtrace_snapshot_mode) < 0) { > > perf_evlist__mmap_ex's overwrite argument is used to control evlist->mmap. > > Consider Namhyung's example: > > perf record --overwrite -e 'cycles/no-overwrite/' > > In this case both evlist->mmap and evlist->backward_mmap would be set to overwrite. > 'cycles' will be put into evlist->mmap, so it will be set to overwrite incorrectly. right, missed the separate mmaps.. so we have some code that uses evlist->overwrite, which is always set to 'false' in perf record.. but in the crucial checks like for perf_mmap__consume we use the 'backward' bool to save the day that might need some consolidation as well.. we could keep the overwrite flag in the struct perf_mmap.. that could simplify the code jirka