From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756472AbdJJSaS (ORCPT ); Tue, 10 Oct 2017 14:30:18 -0400 Received: from szxga04-in.huawei.com ([45.249.212.190]:7961 "EHLO szxga04-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755836AbdJJSaR (ORCPT ); Tue, 10 Oct 2017 14:30:17 -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> CC: , , , , , From: "Wangnan (F)" Message-ID: <37b6527e-cc03-db99-e30f-4652c06b5cb0@huawei.com> Date: Wed, 11 Oct 2017 02:23:54 +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: <1507656023-177125-3-git-send-email-kan.liang@intel.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.0A0B0204.59DD1184.007E,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: 9b499d084340699a7fbf0a13cc6d62de Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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; > + I don't understand this patch. What rb_find_range() wants to do is to find start and end pointer, so record__mmap_read() knows where to start reading and where to stop. In backward mode, 'start' pointer is clearly 'head' (because 'head' is the header of the last record kernel writes to the ring buffer), but the 'end' pointer is not very easy to find ('end' pointer is the last (the earliest) available record in the ring buffer). We have to parse the whole ring buffer to find the last available record and set its tail into 'end', this is what backward_rb_find_range() tries hard to do. However, your patch unconditionally overwrites *end (which is set by backward_rb_find_range()), makes backward_rb_find_range() meaningless. In my mind when you decide to use backward ring buffer you must make it overwrite because you want the kernel silently overwrite old records without waking up perf, while still able to parse the ring buffer even if overwriting happens. The use case should be: perf runs in background, kernel silently ignores old records in the ring buffer until something bad happens, then perf dumps 'events before the bad thing'. Similar to fight recorder. In this use case, each dumping is independent. Even if one record is appears in previous dumping, it is harmless (and useful) to make it appears in following dumping again. 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. > + return ret; > } > > /*