lkml.org 
[lkml]   [2009]   [May]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 1/3] tracing/stat: sort in ascending order
    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 <lizf@cn.fujitsu.com>


    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/



    \
     
     \ /
      Last update: 2009-05-25 17:45    [W:0.033 / U:0.396 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site