Re: [PATCH 02/10] perf tool: fix: Don't discard prev in backward mode

[Wangnan (F)]

On 2017/10/12 20:49, Liang, Kan wrote:
> > >   From 8b058ea6977a97e5705aa2f64bdd014fd76d1247 Mon Sep 17
> > 00:00:00
> > > 2001
> > > From: Kan Liang <Kan.liang@xxxxxxxxx>
> > > Date: Wed, 11 Oct 2017 07:39:34 -0700
> > > Subject: [PATCH] perf tool: fix: Don't discard prev in backward mode
> > >
> > > Perf record can switch output. The new output should only store the
> > > data after switching. However, in overwrite backward mode, the new
> > > output still have the data from old output. That also brings extra overhead.
> > >
> > > At the end of mmap_read, the position of processed ring buffer is
> > > saved in md->prev. Next mmap_read should be end in md->prev if it is
> > > not overwriten. That avoids to process duplicate data.
> > > However, the md->prev is discarded. So next mmap_read has to process
> > > whole valid ring buffer, which probably include the old processed
> > > data.
> > >
> > > Introduce fast path for backward_rb_find_range. Stop searching when
> > > md->prev is detected.
> > >
> > > Signed-off-by: Kan Liang <Kan.liang@xxxxxxxxx>
> > > ---
> > >    tools/perf/util/mmap.c | 11 +++++++++--
> > >    1 file changed, 9 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tools/perf/util/mmap.c b/tools/perf/util/mmap.c index
> > > 9fe5f9c..36b459a 100644
> > > --- a/tools/perf/util/mmap.c
> > > +++ b/tools/perf/util/mmap.c
> > > @@ -254,7 +254,8 @@ int perf_mmap__mmap(struct perf_mmap *map,
> > struct mmap_params *mp, int fd)
> > >    	return 0;
> > >    }
> > >
> > > -static int backward_rb_find_range(void *buf, int mask, u64 head, u64
> > > *start, u64 *end)
> > > +static int backward_rb_find_range(void *buf, int mask, u64 head,
> > > +				  u64 old, u64 *start, u64 *end)
> > >    {
> > >    	struct perf_event_header *pheader;
> > >    	u64 evt_head = head;
> > > @@ -282,6 +283,12 @@ static int backward_rb_find_range(void *buf, int
> > > mask, u64 head, u64 *start, u64
> > >
> > >    		evt_head += pheader->size;
> > >    		pr_debug3("move evt_head: %"PRIx64"\n", evt_head);
> > > +
> > > +		/* fast path: avoid to process duplicate data */
> > > +		if (old == evt_head) {
> > > +			*end = evt_head;
> > > +			return 0;
> > > +		}
> > You still need to parse the whole ring buffer.
> I don't think so. I'm not using (old & mask).
> The upper bit of old is good enough to tell if there is overwriting.
>
> Here are some debugging logs.
> The start and end is the result from backward_rb_find_range.
> If there is overwriting, it finishes with 'rewind', not 'fast path'.
>
> sudo ./perf record -m1 -e cycles:P -C0 --overwrite --switch-output=1s
> backward_rb_find_range: buf=0x7f9ea7a93000, head=fffffffffffff550
> Finished reading backward ring buffer: fast path
> start   0xfffffffffffff550 end 0x0
>
> backward_rb_find_range: buf=0x7f9ea7a93000, head=ffffffffffffab30
> Finished reading backward ring buffer: rewind
> start   0xffffffffffffab30 end 0xffffffffffffbb10
>
> backward_rb_find_range: buf=0x7f9ea7a93000, head=ffffffffffff28b0
> Finished reading backward ring buffer: rewind
> start   0xffffffffffff28b0 end 0xffffffffffff3898
>
> backward_rb_find_range: buf=0x7f9ea7a93000, head=fffffffffffe3e40
> Finished reading backward ring buffer: rewind
> start   0xfffffffffffe3e40 end 0xfffffffffffe4e40
>
> backward_rb_find_range: buf=0x7f9ea7a93000, head=fffffffffffe3aa0
> Finished reading backward ring buffer: fast path
> start   0xfffffffffffe3aa0 end 0xfffffffffffe3e40
>
> backward_rb_find_range: buf=0x7f9ea7a93000, head=fffffffffffe3470
> Finished reading backward ring buffer: fast path
> start   0xfffffffffffe3470 end 0xfffffffffffe3aa0
>
> backward_rb_find_range: buf=0x7f9ea7a93000, head=fffffffffffe0610
> Finished reading backward ring buffer: rewind
> start   0xfffffffffffe0610 end 0xfffffffffffe1610
>
> backward_rb_find_range: buf=0x7f9ea7a93000, head=fffffffffffe00d0
> Finished reading backward ring buffer: fast path
> start   0xfffffffffffe00d0 end 0xfffffffffffe0610
>
> backward_rb_find_range: buf=0x7f9ea7a93000, head=fffffffffffdfe90
> Finished reading backward ring buffer: fast path
> start   0xfffffffffffdfe90 end 0xfffffffffffe00d0

I'm not saying your code is incorrect. What I mean is
you can avoid calling backward_rb_find_range() which
parses the ring buffer.

Please see rb_find_range. In case of forward ring buffer,
it never reads anything for positioning. Just passing start
and end pointers. Your and my implementation requires
reading the ring buffer to find the last record. It needs
a while loop.

When 'old - head' less than the size of the ring buffer,
even backward ring buffer can also avoid such reading.
backward_rb_find_range() should be called when 'old - head'
larger than the size of ring buffer only.

Thank you.