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, [email protected] > > > > > > escreveu: > > > > > > > From: Kan Liang <[email protected]> > > > > > > > > > > > > > > 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 <[email protected]> > > 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? It doesn't need to go via perf/urgent as there is no current user, but I could just fix it so that we have more info about its history in the git commit logs. - Arnaldo
