From: Jiri Olsa <jolsa@redhat.com>
To: "Wangnan (F)" <wangnan0@huawei.com>
Cc: Namhyung Kim <namhyung@kernel.org>,
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
Date: Thu, 2 Nov 2017 16:12:08 +0100 [thread overview]
Message-ID: <20171102151208.GB19184@krava> (raw)
In-Reply-To: <4eeabf43-4de0-467c-04b0-85217040b036@huawei.com>
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
next prev parent reply other threads:[~2017-11-02 15:12 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-01 5:53 [PATCH 0/2] perf record: Fix --overwrite and clarify concepts Wang Nan
2017-11-01 5:53 ` [PATCH 1/2] perf mmap: Fix perf backward recording Wang Nan
2017-11-01 9:49 ` Namhyung Kim
2017-11-01 10:32 ` Wangnan (F)
2017-11-01 12:00 ` Namhyung Kim
2017-11-01 12:10 ` Wangnan (F)
2017-11-01 12:39 ` Jiri Olsa
2017-11-01 12:56 ` Wangnan (F)
2017-11-02 15:12 ` Jiri Olsa [this message]
2017-11-01 13:57 ` Liang, Kan
2017-11-01 16:12 ` Wangnan (F)
2017-11-01 16:22 ` Liang, Kan
2017-11-02 5:34 ` Namhyung Kim
2017-11-02 13:25 ` Liang, Kan
2017-11-02 14:59 ` Jiri Olsa
2017-11-01 5:53 ` [PATCH 2/2] perf record: Replace 'overwrite' by 'flightrecorder' for better naming Wang Nan
2017-11-01 10:03 ` Namhyung Kim
2017-11-01 10:17 ` Wangnan (F)
2017-11-01 12:03 ` Namhyung Kim
2017-11-01 13:26 ` Liang, Kan
2017-11-01 14:05 ` Wangnan (F)
2017-11-01 14:22 ` Liang, Kan
2017-11-01 14:44 ` Wangnan (F)
2017-11-01 15:04 ` Liang, Kan
2017-11-01 16:00 ` Wangnan (F)
2017-11-01 16:13 ` Liang, Kan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20171102151208.GB19184@krava \
--to=jolsa@redhat.com \
--cc=acme@kernel.org \
--cc=kan.liang@intel.com \
--cc=kernel-team@lge.com \
--cc=linux-kernel@vger.kernel.org \
--cc=namhyung@kernel.org \
--cc=wangnan0@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).