From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756582AbdJJSvS (ORCPT ); Tue, 10 Oct 2017 14:51:18 -0400 Received: from szxga05-in.huawei.com ([45.249.212.191]:7543 "EHLO szxga05-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755836AbdJJSvQ (ORCPT ); Tue, 10 Oct 2017 14:51:16 -0400 Subject: Re: [PATCH 02/10] perf tool: fix: Don't discard prev in backward mode To: , , , , References: <1507656023-177125-1-git-send-email-kan.liang@intel.com> <1507656023-177125-3-git-send-email-kan.liang@intel.com> <37b6527e-cc03-db99-e30f-4652c06b5cb0@huawei.com> CC: , , , , , From: "Wangnan (F)" Message-ID: <37664610-cd3f-57e2-7a05-d0f3d81e8a9d@huawei.com> Date: Wed, 11 Oct 2017 02:50:15 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <37b6527e-cc03-db99-e30f-4652c06b5cb0@huawei.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.111.194.139] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A090202.59DD167B.02AB,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2014-11-16 11:51:01, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 1e9e906a7da20ed1681dcd6e21d0d7d2 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2017/10/11 2:23, Wangnan (F) wrote: > > > On 2017/10/11 1:20, kan.liang@intel.com wrote: >> From: Kan Liang >> >> 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 >> --- >> 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) diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c index ee7d0a8..f621a8e 100644 --- a/tools/perf/builtin-record.c +++ b/tools/perf/builtin-record.c @@ -173,7 +173,9 @@ rb_find_range(void *data, int mask, u64 head, u64 old, return 0; } - return backward_rb_find_range(data, mask, head, start, end); + *start = head; + *end = old; + return 0; } static int @@ -199,10 +201,15 @@ record__mmap_read(struct record *rec, struct perf_mmap *md, size = end - start; if (size > (unsigned long)(md->mask) + 1) { - WARN_ONCE(1, "failed to keep up with mmap data. (warn only once)\n"); + if (!backward) { + WARN_ONCE(1, "failed to keep up with mmap data. (warn only once)\n"); - md->prev = head; - perf_mmap__consume(md, overwrite || backward); + md->prev = head; + perf_mmap__consume(md, overwrite || backward); + } else { + backward_rb_find_range(data, md->mask, head, start, end); + /* FIXME: error processing */ + } return 0; } Use 'head' and 'old' to locate data position in ring buffer by default. If overwrite happen, use backward_rb_find_range() to fetch the last available record and dump the whole ring buffer. Thank you.