Re: [PATCH 03/10] perf tool: new iterfaces to read event from ring buffer

["Wangnan (F)" <wangnan0@huawei.com>]

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 <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?
>>>>> 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.
>>> 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