Subject: Re: [PATCH 4/4] perf core: Add backward attribute to perf event
To: Peter Zijlstra <peterz@infradead.org>
References: <1459147292-239310-1-git-send-email-wangnan0@huawei.com>
 <1459147292-239310-5-git-send-email-wangnan0@huawei.com>
 <20160329140439.GK3408@twins.programming.kicks-ass.net>
 <56FB39E9.7020006@huawei.com>
CC: Alexei Starovoitov <ast@kernel.org>,
        Arnaldo Carvalho de Melo <acme@redhat.com>,
        <linux-kernel@vger.kernel.org>,
        Brendan Gregg <brendan.d.gregg@gmail.com>,
        He Kuang <hekuang@huawei.com>, Jiri Olsa <jolsa@kernel.org>,
        Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>,
        Namhyung Kim <namhyung@kernel.org>, <pi3orama@163.com>,
        Zefan Li <lizefan@huawei.com>
From: "Wangnan (F)" <wangnan0@huawei.com>
Message-ID: <56FB3C3D.3050400@huawei.com>
Date: Wed, 30 Mar 2016 10:38:53 +0800
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101
 Thunderbird/38.5.0
MIME-Version: 1.0
In-Reply-To: <56FB39E9.7020006@huawei.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2611
Lines: 75


On 2016/3/30 10:28, Wangnan (F) wrote:
>
>
> On 2016/3/29 22:04, Peter Zijlstra wrote:
>> On Mon, Mar 28, 2016 at 06:41:32AM +0000, Wang Nan wrote:
>>
>> Could you maybe write a perf/tests thingy for this so that _some_
>> userspace exists that exercises this new code?
>>
>>
>>>   int perf_output_begin(struct perf_output_handle *handle,
>>>                 struct perf_event *event, unsigned int size)
>>>   {
>>> +    if (unlikely(is_write_backward(event)))
>>> +        return __perf_output_begin(handle, event, size, true);
>>>       return __perf_output_begin(handle, event, size, false);
>>>   }
>> Would something like:
>>
>> int perf_output_begin(...)
>> {
>>     if (unlikely(is_write_backward(event))
>>         return perf_output_begin_backward(...);
>>     return perf_output_begin_forward(...);
>> }
>>
>> make sense; I'm not sure how much is still using this, but it seems
>> somewhat excessive to inline two copies of that thing into a single
>> function.
>
> perf_output_begin is useful:
>
> $ grep perf_output_begin ./kernel -r
> ./kernel/events/ring_buffer.c:     * See perf_output_begin().
> ./kernel/events/ring_buffer.c:int perf_output_begin(struct 
> perf_output_handle *handle,
> ./kernel/events/ring_buffer.c:     * perf_output_begin() only checks 
> rb->paused, therefore
> ./kernel/events/core.c:    if (perf_output_begin(&handle, event, 
> header.size))
> ./kernel/events/core.c:    ret = perf_output_begin(&handle, event, 
> read_event.header.size);
> ./kernel/events/core.c:    ret = perf_output_begin(&handle, event,
> ./kernel/events/core.c:    ret = perf_output_begin(&handle, event,
> ./kernel/events/core.c:    ret = perf_output_begin(&handle, event,
> ./kernel/events/core.c:    ret = perf_output_begin(&handle, event, 
> rec.header.size);
> ./kernel/events/core.c:    ret = perf_output_begin(&handle, event,
> ./kernel/events/core.c:    ret = perf_output_begin(&handle, event, 
> se->event_id.header.size);
> ./kernel/events/core.c:    ret = perf_output_begin(&handle, event,
> ./kernel/events/core.c:    ret = perf_output_begin(&handle, event, 
> rec.header.size);
>
> Events like PERF_RECORD_MMAP2 uses this function, so we still need to 
> consider its overhead.
>
> So I will use your first suggestion.
>

Sorry. Your second suggestion seems also good:

My implementation makes a big perf_output_begin(), but introduces only 
one load and one branch.

Your first suggestion introduces one load, one branch and one function call.

Your second suggestion introduces one load, and at least one (and at 
most three) branches.

I need some benchmarking result.

Thank you.