From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754081AbZEYPmn (ORCPT ); Mon, 25 May 2009 11:42:43 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752228AbZEYPmf (ORCPT ); Mon, 25 May 2009 11:42:35 -0400 Received: from mail-bw0-f174.google.com ([209.85.218.174]:49334 "EHLO mail-bw0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751600AbZEYPme (ORCPT ); Mon, 25 May 2009 11:42:34 -0400 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; b=IeTEfmt/ptxtSEpej+9aI8DF/cKDxOHAwMP2IvWMD92fSahFMilBjlwrEs8szXxGgV Z6JZVoxzk1j+MxNN0L0OQUtthB7Nv8Q7pR67ye30LH2xWllB/vPnTJTkyist+wu5hvza f+3iX2uQ689lBzl3Atf2PNIwmqwg+oy5HfpRQ= Date: Mon, 25 May 2009 17:42:32 +0200 From: Frederic Weisbecker To: Li Zefan Cc: Ingo Molnar , Steven Rostedt , LKML Subject: Re: [PATCH 1/3] tracing/stat: sort in ascending order Message-ID: <20090525154229.GA7121@nowhere> References: <4A1A5AD1.3070804@cn.fujitsu.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4A1A5AD1.3070804@cn.fujitsu.com> User-Agent: Mutt/1.5.18 (2008-05-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, May 25, 2009 at 04:46:09PM +0800, Li Zefan wrote: > Currently the output of trace_stat/workqueues is totally reversed: > > # cat /debug/tracing/trace_stat/workqueues > ... > 1 17 17 210 37 `-blk_unplug_work+0x0/0x57 > 1 3779 3779 181 11 |-cfq_kick_queue+0x0/0x2f > 1 3796 3796 kblockd/1:120 > ... > > The correct output should be: > > 1 3796 3796 kblockd/1:120 > 1 3779 3779 181 11 |-cfq_kick_queue+0x0/0x2f > 1 17 17 210 37 `-blk_unplug_work+0x0/0x57 > > It's caused by "tracing/stat: replace linked list by an rbtree for sorting" > (53059c9b67a62a3dc8c80204d3da42b9267ea5a0). > > Though we can simply change dummy_cmp() to return -1 instead of 1, IMO > it's better to always do ascending sorting in trace_stat.c, and leave each > stat tracer to decide whether to sort in descending or ascending order. > > [ Impact: fix the output of trace_stat/workqueue ] > > Signed-off-by: Li Zefan For now in stat tracing, the ascendent sorting is the most relevant. Especially because we always want to see the highest problems first. -1 (or < 0) usually means lower and 1 ( > 0) is higher. I wonder what would most confuse the developers of stat tracers: - to reverse these common sort values (-1 turn into "higher") - keep the default ascendent sorting, which is not natural because the default is often descendent. I don't know. Anyone else. Do you have a preference? Thanks, Frederic. > --- > kernel/trace/ftrace.c | 12 ++++++------ > kernel/trace/trace_branch.c | 5 +++-- > kernel/trace/trace_stat.c | 6 +----- > 3 files changed, 10 insertions(+), 13 deletions(-) > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > index 140699a..3dd16bd 100644 > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c > @@ -315,29 +315,29 @@ static void *function_stat_start(struct tracer_stat *trace) > } > > #ifdef CONFIG_FUNCTION_GRAPH_TRACER > -/* function graph compares on total time */ > +/* function graph compares on total time in reverse order */ > static int function_stat_cmp(void *p1, void *p2) > { > struct ftrace_profile *a = p1; > struct ftrace_profile *b = p2; > > - if (a->time < b->time) > - return -1; > if (a->time > b->time) > + return -1; > + if (a->time < b->time) > return 1; > else > return 0; > } > #else > -/* not function graph compares against hits */ > +/* not function graph compares against hits in reverse order */ > static int function_stat_cmp(void *p1, void *p2) > { > struct ftrace_profile *a = p1; > struct ftrace_profile *b = p2; > > - if (a->counter < b->counter) > - return -1; > if (a->counter > b->counter) > + return -1; > + if (a->counter < b->counter) > return 1; > else > return 0; > diff --git a/kernel/trace/trace_branch.c b/kernel/trace/trace_branch.c > index 7a7a9fd..df58411 100644 > --- a/kernel/trace/trace_branch.c > +++ b/kernel/trace/trace_branch.c > @@ -301,9 +301,10 @@ static int annotated_branch_stat_cmp(void *p1, void *p2) > percent_a = get_incorrect_percent(a); > percent_b = get_incorrect_percent(b); > > - if (percent_a < percent_b) > - return -1; > + /* sort in descending order */ > if (percent_a > percent_b) > + return -1; > + if (percent_a < percent_b) > return 1; > else > return 0; > diff --git a/kernel/trace/trace_stat.c b/kernel/trace/trace_stat.c > index 2e849b5..6efbcb4 100644 > --- a/kernel/trace/trace_stat.c > +++ b/kernel/trace/trace_stat.c > @@ -98,10 +98,6 @@ insert_stat(struct rb_root *root, struct stat_node *data, cmp_stat_t cmp) > { > struct rb_node **new = &(root->rb_node), *parent = NULL; > > - /* > - * Figure out where to put new node > - * This is a descendent sorting > - */ > while (*new) { > struct stat_node *this; > int result; > @@ -110,7 +106,7 @@ insert_stat(struct rb_root *root, struct stat_node *data, cmp_stat_t cmp) > result = cmp(data->stat, this->stat); > > parent = *new; > - if (result >= 0) > + if (result < 0) > new = &((*new)->rb_left); > else > new = &((*new)->rb_right); > -- > 1.5.4.rc3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/