[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <56CF398A.8020905@fb.com>
Date: Thu, 25 Feb 2016 09:27:38 -0800
From: Alexei Starovoitov <ast@...com>
To: Peter Zijlstra <peterz@...radead.org>
CC: "David S. Miller" <davem@...emloft.net>,
Ingo Molnar <mingo@...nel.org>,
Steven Rostedt <rostedt@...dmis.org>,
Wang Nan <wangnan0@...wei.com>,
Daniel Borkmann <daniel@...earbox.net>,
Brendan Gregg <brendan.d.gregg@...il.com>,
<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next 1/3] perf: generalize perf_callchain
On 2/25/16 8:47 AM, Peter Zijlstra wrote:
> On Wed, Feb 17, 2016 at 07:58:57PM -0800, Alexei Starovoitov wrote:
>> +static inline int perf_callchain_store(struct perf_callchain_entry *entry, u64 ip)
>> {
>> + if (entry->nr < PERF_MAX_STACK_DEPTH) {
>> entry->ip[entry->nr++] = ip;
>> + return 0;
>> + } else {
>> + return -1; /* no more room, stop walking the stack */
>> + }
>> }
>
> Why 0 and -1 ?
because that's the interface you had for callbacks in
'struct stacktrace_ops' including a comment there:
/* On negative return stop dumping */
> What's wrong with something like:
>
> static inline bool perf_callchain_store(struct perf_callchain_entry *entry, u64 ip)
> {
> if (entry->nr < PERF_MAX_STACK_DEPTH) {
> entry->ip[entry->nr++] = ip;
> return true;
> }
> return false;
> }
I would prefer something like this as well, but it would look
inconsistent with what is already there. To make it bool
one would need to change struct stacktrace_ops for all archs
and touch a lot of files all over the tree.
Way more than 51 insertions(+), 29 deletions(-) as this patch did
for no real gain.
It's better to be consistent with existing code.
Powered by blists - more mailing lists