From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934154AbdJJUAw (ORCPT ); Tue, 10 Oct 2017 16:00:52 -0400 Received: from szxga05-in.huawei.com ([45.249.212.191]:7545 "EHLO szxga05-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934127AbdJJUAs (ORCPT ); Tue, 10 Oct 2017 16:00:48 -0400 Subject: Re: [PATCH 03/10] perf tool: new iterfaces to read event from ring buffer To: "Liang, Kan" , 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> <14740ebe-fd65-a6af-15c7-cd22331a2aaf@huawei.com> <37D7C6CF3E00A74B8858931C1DB2F077537D1BFC@SHSMSX103.ccr.corp.intel.com> CC: "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: <0c80f401-8a1b-b41b-7abc-386229e4ce80@huawei.com> Date: Wed, 11 Oct 2017 03:59:47 +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: <37D7C6CF3E00A74B8858931C1DB2F077537D1BFC@SHSMSX103.ccr.corp.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.0A090202.59DD26C8.0197,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:55, Liang, Kan wrote: >> 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. >> ... >> > Yes, it will be used for perf top optimization. > That will be great if you can fix it. You can fix it and post your fix together with your perf top patch set. I can write the code but I can't test it because there's no user now. > I still think it's a good idea to have common interfaces for both perf record > and other tools like perf top. > rb_find_range/backward_rb_find_range could be shared. > The mmap read codes are similar. Agree. Please keep going. > Thanks, > Kan