Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 1/2] perf mmap: Fix perf backward recording
To:     "Liang, Kan" <kan.liang@intel.com>,
        Namhyung Kim <namhyung@kernel.org>
References: <20171101055327.141281-1-wangnan0@huawei.com>
 <20171101055327.141281-2-wangnan0@huawei.com>
 <20171101094929.GB25146@danjae.aot.lge.com>
 <e6079124-0171-adc4-742f-cd3f9b2d1df4@huawei.com>
 <20171101120007.GA26623@danjae.aot.lge.com>
 <109f02ef-5dc2-94f9-e850-572c498781ee@huawei.com>
 <37D7C6CF3E00A74B8858931C1DB2F077537DC1FA@SHSMSX103.ccr.corp.intel.com>
CC:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "acme@kernel.org" <acme@kernel.org>,
        "jolsa@redhat.com" <jolsa@redhat.com>,
        "kernel-team@lge.com" <kernel-team@lge.com>
From:   "Wangnan (F)" <wangnan0@huawei.com>
Message-ID: <226626ec-4f96-d3f4-a6d5-62c17c897f32@huawei.com>
Date:   Thu, 2 Nov 2017 00:12:21 +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: <37D7C6CF3E00A74B8858931C1DB2F077537DC1FA@SHSMSX103.ccr.corp.intel.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 2017/11/1 21:57, Liang, Kan wrote:
>> On 2017/11/1 20:00, Namhyung Kim wrote:
>>> On Wed, Nov 01, 2017 at 06:32:50PM +0800, Wangnan (F) wrote:
>>>> On 2017/11/1 17:49, Namhyung Kim wrote:
>>>>> Hi,
>>>>>
>>>>> On Wed, Nov 01, 2017 at 05:53:26AM +0000, Wang Nan wrote:
>>>>>> perf record backward recording doesn't work as we expected: it never
>>>>>> overwrite when ring buffer full.
>>>>>>
>>>>>> Test:
>>>>>>
>>>>>> (Run a busy printing python task background like this:
>>>>>>
>>>>>>     while True:
>>>>>>         print 123
>>>>>>
>>>>>> send SIGUSR2 to perf to capture snapshot.)
>>>> [SNIP]
>>>>
>>>>>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
>>>>>> ---
>>>>>>     tools/perf/util/evlist.c | 8 +++++++-
>>>>>>     1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>>>>>> index c6c891e..4c5daba 100644
>>>>>> --- a/tools/perf/util/evlist.c
>>>>>> +++ b/tools/perf/util/evlist.c
>>>>>> @@ -799,22 +799,28 @@ perf_evlist__should_poll(struct perf_evlist
>> *evlist __maybe_unused,
>>>>>>     }
>>>>>>     static int perf_evlist__mmap_per_evsel(struct perf_evlist *evlist, int
>> idx,
>>>>>> -				       struct mmap_params *mp, int cpu_idx,
>>>>>> +				       struct mmap_params *_mp, int cpu_idx,
>>>>>>     				       int thread, int *_output, int
>> *_output_backward)
>>>>>>     {
>>>>>>     	struct perf_evsel *evsel;
>>>>>>     	int revent;
>>>>>>     	int evlist_cpu = cpu_map__cpu(evlist->cpus, cpu_idx);
>>>>>> +	struct mmap_params *mp;
>>>>>>     	evlist__for_each_entry(evlist, evsel) {
>>>>>>     		struct perf_mmap *maps = evlist->mmap;
>>>>>> +		struct mmap_params rdonly_mp;
>>>>>>     		int *output = _output;
>>>>>>     		int fd;
>>>>>>     		int cpu;
>>>>>> +		mp = _mp;
>>>>>>     		if (evsel->attr.write_backward) {
>>>>>>     			output = _output_backward;
>>>>>>     			maps = evlist->backward_mmap;
>>>>>> +			rdonly_mp = *_mp;
>>>>>> +			rdonly_mp.prot &= ~PROT_WRITE;
>>>>>> +			mp = &rdonly_mp;
>>>>>>     			if (!maps) {
>>>>>>     				maps = perf_evlist__alloc_mmap(evlist);
>>>>>> --
>>>>> What about this instead (not tested)?
>>>>>
>>>>>
>>>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>>>>> index c6c891e154a6..27ebe355e794 100644
>>>>> --- a/tools/perf/util/evlist.c
>>>>> +++ b/tools/perf/util/evlist.c
>>>>> @@ -838,6 +838,11 @@ static int perf_evlist__mmap_per_evsel(struct
>> perf_evlist *evlist, int idx,
>>>>>                    if (*output == -1) {
>>>>>                            *output = fd;
>>>>> +                       if (evsel->attr.write_backward)
>>>>> +                               mp->prot = PROT_READ;
>>>>> +                       else
>>>>> +                               mp->prot = PROT_READ | PROT_WRITE;
>>>>> +
>>>> If evlist->overwrite is true, PROT_WRITE should be unset even if
>>>> write_backward is
>>>> not set. If you want to delay the setting of mp->prot, you need to consider
>>>> both evlist->overwrite and evsel->attr.write_backward.
>>> I thought evsel->attr.write_backward should be set when
>>> evlist->overwrite is set.  Do you mean following case?
>>>
>>>     perf record --overwrite -e 'cycles/no-overwrite/'
>>>
>> No. evlist->overwrite is unrelated to '--overwrite'. This is why I
>> said the concept of 'overwrite' and 'backward' is ambiguous.
>>
> Yes, I think we should make it clear.
>
> As we discussed previously, there are four possible combinations
> to access ring buffer , 'forward non-overwrite', 'forward overwrite',
> 'backward non-overwrite' and 'backward overwrite'.
>
> Actually, not all of the combinations are necessary.
> - 'forward overwrite' mode brings some problems which were mentioned
>    in commit ID 9ecda41acb97 ("perf/core: Add ::write_backward attribute
>    to perf event").
> - 'backward non-overwrite' mode is very similar as 'forward non-overwrite'.
>     There is no extra advantage. Only keep one non-overwrite mode is enough.
> So 'forward non-overwrite' and 'backward overwrite' are enough for all perf tools.
>
> Furthermore, 'forward' and 'backward' only indicate the direction of the
> ring buffer. They don't impact the result and performance. It is not
> important as the concept of overwrite/non-overwrite.
>
> To simplify the concept, only 'non-overwrite' mode and 'overwrite' mode should
> be kept. 'non-overwrite' mode indicates the forward ring buffer. 'overwrite' mode
> indicates the backward ring buffer.
>
>> perf record never sets 'evlist->overwrite'. What '--overwrite' actually
>> does is setting write_backward. Some testcases needs overwrite evlist.
>>
> There are only four test cases which set overwrite, sw-clock,task-exit,
> mmap-basic, backward-ring-buffer.
> Only backward-ring-buffer is 'backward overwrite'.
> The rest three are all 'forward overwrite'. We just need to set write_backward
> to convert them to 'backward overwrite'.
> I think it's not hard to clean up.

If we add a new rule that overwrite ring buffers are always backward
then it is not hard to cleanup. However, the support of forward
overwrite ring buffer has a long history and the code is not written
by me. I'd like to check if there is some reason to keep support this
configuration?

Thank you.


From 1582872369286961737@xxx Wed Nov 01 13:58:44 +0000 2017
X-GM-THRID: 1582841972981207976
X-Gmail-Labels: Inbox,Category Forums