lkml.org 
[lkml]   [2011]   [Aug]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 06/10] tracing/filter: Change count_leafs function to use walk_pred_tree
    From
    Date
    I really like these two patches (5 and 6) as I hated the duplicate code
    of the two tree walks. But... see below.


    On Thu, 2011-08-04 at 12:08 +0200, Jiri Olsa wrote:
    > Changing count_leafs function to use unified predicates tree
    > processing.
    >
    > Signed-off-by: Jiri Olsa <jolsa@redhat.com>
    > ---
    > kernel/trace/trace_events_filter.c | 45 +++++++++--------------------------
    > 1 files changed, 12 insertions(+), 33 deletions(-)
    >
    > diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
    > index 5b889d4..14a9dad 100644
    > --- a/kernel/trace/trace_events_filter.c
    > +++ b/kernel/trace/trace_events_filter.c
    > @@ -1418,43 +1418,22 @@ static int check_pred_tree(struct event_filter *filter,
    > check_pred_tree_cb, &data);
    > }
    >
    > -static int count_leafs(struct filter_pred *preds, struct filter_pred *root)
    > +static int count_leafs_cb(enum move_type move, struct filter_pred *pred,
    > + int *err, void *data)
    > {
    > - struct filter_pred *pred;
    > - enum move_type move = MOVE_DOWN;
    > - int count = 0;
    > - int done = 0;
    > + int *count = data;
    >
    > - pred = root;
    > + if ((move == MOVE_DOWN) &&
    > + (pred->left == FILTER_PRED_INVALID))
    > + (*count)++;
    >
    > - do {
    > - switch (move) {
    > - case MOVE_DOWN:
    > - if (pred->left != FILTER_PRED_INVALID) {
    > - pred = &preds[pred->left];
    > - continue;
    > - }
    > - /* A leaf at the root is just a leaf in the tree */
    > - if (pred == root)
    > - return 1;
    > - count++;
    > - pred = get_pred_parent(pred, preds,
    > - pred->parent, &move);
    > - continue;
    > - case MOVE_UP_FROM_LEFT:
    > - pred = &preds[pred->right];
    > - move = MOVE_DOWN;
    > - continue;
    > - case MOVE_UP_FROM_RIGHT:
    > - if (pred == root)
    > - break;
    > - pred = get_pred_parent(pred, preds,
    > - pred->parent, &move);
    > - continue;
    > - }
    > - done = 1;
    > - } while (!done);
    > + return WALK_PRED_DEFAULT;
    > +}
    >
    > +static int count_leafs(struct filter_pred *preds, struct filter_pred *root)
    > +{
    > + int count = 0;
    > + WARN_ON(walk_pred_tree(preds, root, count_leafs_cb, &count));

    Please do not put functionality in a WARN_ON(). Some people have
    WARN_ON() turn into nops.

    Make this a:

    ret = walk_pred_tree(preds, root, count_leafs_cb, &count);
    WARN_ON(ret);

    -- Steve

    > return count;
    > }
    >




    \
     
     \ /
      Last update: 2011-08-10 23:09    [W:0.028 / U:0.848 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site