From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756574AbdJJT1q (ORCPT ); Tue, 10 Oct 2017 15:27:46 -0400 Received: from szxga05-in.huawei.com ([45.249.212.191]:7544 "EHLO szxga05-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751544AbdJJT1o (ORCPT ); Tue, 10 Oct 2017 15:27:44 -0400 Subject: Re: [PATCH 03/10] perf tool: new iterfaces to read event from ring buffer To: Arnaldo Carvalho de Melo References: <1507656023-177125-1-git-send-email-kan.liang@intel.com> <1507656023-177125-4-git-send-email-kan.liang@intel.com> <20171010181520.GH28623@kernel.org> <37D7C6CF3E00A74B8858931C1DB2F077537D1B11@SHSMSX103.ccr.corp.intel.com> <20171010183455.GK28623@kernel.org> <20171010183628.GL28623@kernel.org> <20171010190014.GM28623@kernel.org> <20171010191737.GQ28623@kernel.org> CC: "Liang, Kan" , "peterz@infradead.org" , "mingo@redhat.com" , "linux-kernel@vger.kernel.org" , "jolsa@kernel.org" , "hekuang@huawei.com" , "namhyung@kernel.org" , "alexander.shishkin@linux.intel.com" , "Hunter, Adrian" , "ak@linux.intel.com" From: "Wangnan (F)" Message-ID: <14740ebe-fd65-a6af-15c7-cd22331a2aaf@huawei.com> Date: Wed, 11 Oct 2017 03:22:30 +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: <20171010191737.GQ28623@kernel.org> 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.59DD1F0C.019F,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: 76fe318eb28660a6ec8eb5b91e932a4c Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2017/10/11 3:17, Arnaldo Carvalho de Melo wrote: > Em Wed, Oct 11, 2017 at 03:10:37AM +0800, Wangnan (F) escreveu: >> >> On 2017/10/11 3:00, Arnaldo Carvalho de Melo wrote: >>> Em Tue, Oct 10, 2017 at 03:36:28PM -0300, Arnaldo Carvalho de Melo escreveu: >>>> Em Tue, Oct 10, 2017 at 03:34:55PM -0300, Arnaldo Carvalho de Melo escreveu: >>>>> Em Tue, Oct 10, 2017 at 06:28:18PM +0000, Liang, Kan escreveu: >>>>>>> Em Tue, Oct 10, 2017 at 10:20:16AM -0700, kan.liang@intel.com escreveu: >>>>>>>> From: Kan Liang >>>>>>>> >>>>>>>> The perf_evlist__mmap_read only support forward mode. It needs a >>>>>>>> common function to support both forward and backward mode. >>>>>>>> The perf_evlist__mmap_read_backward is buggy. >>>>>>> So, what is the bug? You state that it is buggy, but don't spell out the bug, >>>>>>> please do so. >>>>>>> >>>>>> union perf_event *perf_evlist__mmap_read_backward(struct perf_evlist *evlist, int idx) >>>>>> { >>>>>> struct perf_mmap *md = &evlist->mmap[idx]; <--- it should be backward_mmap >>>>>> >>>>>>> If it fixes an existing bug, then it should go separate from this patchkit, right? >>>>>> There is no one use perf_evlist__mmap_read_backward. So it doesn't trigger any issue. >>>>> There is no one at the end of your patchkit? Or no user _right now_? If >>>>> there is a user now, lemme see... yeah, no user right now, so _that_ is >>>>> yet another bug, i.e. it should be used, no? If this is just a left >>>>> over, then we should just throw it away, now, its a cleanup. >>>> Wang, can you take a look at these two issues? >>> So it looks leftover that should've been removed by the following cset, right Wang? >>> commit a0c6f451f90204847ce5f91c3268d83a76bde1b6 >>> Author: Wang Nan >>> Date: Thu Jul 14 08:34:41 2016 +0000 >>> perf evlist: Drop evlist->backward >>> Now there's no real user of evlist->backward. Drop it. We are going to >>> use evlist->backward_mmap as a container for backward ring buffer. > >> Yes, it should be removed, but then there will be no corresponding >> function to perf_evlist__mmap_read(), which read an record from forward >> ring buffer. > >> I think Kan wants to become the first user of this function because >> he is trying to make 'perf top' utilizing backward ring buffer. It needs >> perf_evlist__mmap_read_backward(), and he triggers the bug use his >> unpublished patch set. > >> I think we can remove it now, let Kan fix and add it back in his 'perf top' >> patch set. > Well, if there will be a user, perhaps we should fix it, as it seems > interesting to have now for, as you said, a counterpart for the forward > ring buffer, and one that we have plans for using soon, right? Right if I understand Kan's patch 00/10 correctly. He said: ... But perf top need to switch to overwrite backward mode for good performance. ... Thank you.