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

From: Wangnan (F)
Date: Tue Oct 10 2017 - 16:19:56 EST




On 2017/10/11 3:50, Liang, Kan wrote:
On 2017/10/11 2:23, Wangnan (F) wrote:

On 2017/10/11 1:20, kan.liang@xxxxxxxxx wrote:
From: Kan Liang <kan.liang@xxxxxxxxx>

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.

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.
However, the md->prev is discarded. So next mmap_read has to process
whole valid ring buffer, which definitely include the old processed
data.

Set the prev as the end of the range in backward mode.

Signed-off-by: Kan Liang <kan.liang@xxxxxxxxx>
---
tools/perf/util/evlist.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 33b8837..7d23cf5 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -742,13 +742,25 @@ static int
rb_find_range(void *data, int mask, u64 head, u64 old,
u64 *start, u64 *end, bool backward)
{
+ int ret;
+
if (!backward) {
*start = old;
*end = head;
return 0;
}
- return backward_rb_find_range(data, mask, head, start, end);
+ ret = backward_rb_find_range(data, mask, head, start, end);
+
+ /*
+ * The start and end from backward_rb_find_range is the range
for all
+ * valid data in ring buffer.
+ * However, part of the data is processed previously.
+ * Reset the end to drop the processed data
+ */
+ *end = old;
+
[SNIP]

If you really want to avoid record duplication, you need to changes
record__mmap_read()'s
logic. Now it complains "failed to keep up with mmap data" and avoid
dumping data when
size of newly generated data is larger than the size of the ring
buffer. It is reasonable
for forward ring buffer because in this case you lost the head of the
first record, the
whole ring buffer is unparseable. However, it is wrong in backward
case. What you
should do in this case is dumping the whole ring buffer.

I think what you want should be something like this: (not tested)

No. That's not what I want.
My test code never trigger the WARN_ONCE.

The existing code never trigger that warning because the size computed
by rb_find_range is never larger than size of ring buffer. After applying
your patch, I believe it will trigger this WARN_ONCE and drop the whole
ring buffer. Please set a smaller ring buffer and try again.

I think you will see the problem, if you simply run the command as below.
sudo ./perf record -e cycles:P -C0 --overwrite --switch-output=1s

The output size keep increasing. Because the new output always include the old outputs.
What I want is the 'start' and 'end' for the increase, not everything.


This is my test result: add a '-m 1' for 'perf record' for shrinking ring buffer,
start a while loop on CPU 0 to increase data rate.

It stops increasing after the ring buffer is full:

$:~/linux/tools/perf$ sudo ./perf record -m1 -e cycles:P -C0 --overwrite --switch-output=1s
Warning: File /home/w00229757/.perfconfig not owned by current user or root, ignoring it.
[ perf record: dump data: Woken up 1 times ]
[ perf record: Dump perf.data.2017101212165072 ]
[ perf record: dump data: Woken up 1 times ]
[ perf record: Dump perf.data.2017101212165175 ]
[ perf record: dump data: Woken up 1 times ]
[ perf record: Dump perf.data.2017101212165278 ]
[ perf record: dump data: Woken up 1 times ]
[ perf record: Dump perf.data.2017101212165381 ]
[ perf record: dump data: Woken up 1 times ]
[ perf record: Dump perf.data.2017101212165484 ]
[ perf record: dump data: Woken up 1 times ]
[ perf record: Dump perf.data.2017101212165586 ]
^C[ perf record: Woken up 1 times to write data ]
[ perf record: Dump perf.data.2017101212165653 ]
[ perf record: Captured and wrote 1.013 MB perf.data.<timestamp> ]

$ ls -l ./perf.data*
-rw------- 1 root root 538988 Oct 12 12:16 ./perf.data.2017101212165072
-rw------- 1 root root 538988 Oct 12 12:16 ./perf.data.2017101212165175
-rw------- 1 root root 538988 Oct 12 12:16 ./perf.data.2017101212165278
-rw------- 1 root root 538988 Oct 12 12:16 ./perf.data.2017101212165381
-rw------- 1 root root 538988 Oct 12 12:16 ./perf.data.2017101212165484
-rw------- 1 root root 538988 Oct 12 12:16 ./perf.data.2017101212165586
-rw------- 1 root root 1067812 Oct 12 12:16 ./perf.data.2017101212165653

You see the result keep getting larger because the ring buffer
is never full in your case.

Thank you.