Messages in this thread | | | Subject | Re: [PATCH 02/10] perf tool: fix: Don't discard prev in backward mode | From | "Wangnan (F)" <> | Date | Wed, 11 Oct 2017 04:18:57 +0800 |
| |
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@intel.com wrote: >>>> From: Kan Liang <kan.liang@intel.com> >>>> >>>> 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@intel.com> >>>> --- >>>> 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.
| |