Messages in this thread | | | Subject | Re: [PATCH 03/10] perf tool: new iterfaces to read event from ring buffer | From | "Wangnan (F)" <> | Date | Wed, 11 Oct 2017 03:10:37 +0800 |
| |
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 <kan.liang@intel.com> >>>>>> >>>>>> 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? > > - Arnaldo > > commit a0c6f451f90204847ce5f91c3268d83a76bde1b6 > Author: Wang Nan <wangnan0@huawei.com> > 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.
Thank you.
| |