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    [from the cache]
©2003-2011 Jasper Spaans