Re: [PATCH 1/2] perf mmap: Fix perf backward recording

[Namhyung Kim <namhyung@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 <wangnan0@huawei.com>
> > > ---
> > >   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