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

[Liang, Kan]

> >  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

Thanks,
Kan

> You can use 'old - head' to check how many bytes are written into the ring
> buffer since you last read. If its size is less than the ring buffer size, there's no
> overwriting happen. In this case a simple copy should be enough.
> 
> Thank you.