[PATCH] tracing/filters: Improve subsystem filter

[PATCH] tracing/filters: Improve subsystem filter

[Li Zefan]

Currently a subsystem filter should be applicable to all events
under the subsystem, and if it failed, all the event filters
will be cleared. Those behaviors make subsys filter much less
useful:

  # echo 'vec == 1' > irq/softirq_entry/filter
  # echo 'irq == 5' > irq/filter
  bash: echo: write error: Invalid argument
  # cat irq/softirq_entry/filter
  none

I'd expect it set the filter for irq_handler_entry/exit, and
not touch softirq_entry/exit.

The basic idea is, try to see if the filter can be applied
to which events, and then just apply to the those events:

  # echo 'vec == 1' > softirq_entry/filter
  # echo 'irq == 5' > filter
  # cat irq_handler_entry/filter
  irq == 5
  # cat softirq_entry/filter
  vec == 1

Inspied-by: Steven Rostedt <srostedt@redhat.com>
Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 include/linux/ftrace_event.h       |    4 +-
 kernel/trace/trace.h               |    3 +-
 kernel/trace/trace_events_filter.c |  123 +++++++++++++++++++++++------------
 3 files changed, 86 insertions(+), 44 deletions(-)

diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index 5c093ff..26d3673 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -101,6 +101,8 @@ void trace_current_buffer_discard_commit(struct ring_buffer_event *event);
 
 void tracing_record_cmdline(struct task_struct *tsk);
 
+struct event_filter;
+
 struct ftrace_event_call {
 	struct list_head	list;
 	char			*name;
@@ -116,7 +118,7 @@ struct ftrace_event_call {
 	int			(*define_fields)(void);
 	struct list_head	fields;
 	int			filter_active;
-	void			*filter;
+	struct event_filter	*filter;
 	void			*mod;
 
 #ifdef CONFIG_EVENT_PROFILE
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 06886a0..3a87d46 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -768,13 +768,14 @@ struct event_filter {
 	int			n_preds;
 	struct filter_pred	**preds;
 	char			*filter_string;
+	bool			no_reset;
 };
 
 struct event_subsystem {
 	struct list_head	list;
 	const char		*name;
 	struct dentry		*entry;
-	void			*filter;
+	struct event_filter	*filter;
 	int			nr_events;
 };
 
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 1c80ef7..0f510c2 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -420,7 +420,13 @@ oom:
 }
 EXPORT_SYMBOL_GPL(init_preds);
 
-static void filter_free_subsystem_preds(struct event_subsystem *system)
+/*
+ * flag == 0: remove all events' filter
+ * flag == 1: clear filter->no_reset
+ * flag == 2: remove all preds with no_reset == false
+ */
+static void filter_free_subsystem_preds(struct event_subsystem *system,
+					int flag)
 {
 	struct ftrace_event_call *call;
 
@@ -428,6 +434,14 @@ static void filter_free_subsystem_preds(struct event_subsystem *system)
 		if (!call->define_fields)
 			continue;
 
+		if (flag == 1) {
+			call->filter->no_reset = false;
+			continue;
+		}
+
+		if (flag == 2 && call->filter->no_reset == true)
+			continue;
+
 		if (!strcmp(call->system, system->name)) {
 			filter_disable_preds(call);
 			remove_filter_string(call->filter);
@@ -529,7 +543,8 @@ static filter_pred_fn_t select_comparison_fn(int op, int field_size,
 
 static int filter_add_pred(struct filter_parse_state *ps,
 			   struct ftrace_event_call *call,
-			   struct filter_pred *pred)
+			   struct filter_pred *pred,
+			   bool apply)
 {
 	struct ftrace_event_field *field;
 	filter_pred_fn_t fn;
@@ -541,10 +556,12 @@ static int filter_add_pred(struct filter_parse_state *ps,
 
 	if (pred->op == OP_AND) {
 		pred->pop_n = 2;
-		return filter_add_pred_fn(ps, call, pred, filter_pred_and);
+		fn = filter_pred_and;
+		goto add_pred_fn;
 	} else if (pred->op == OP_OR) {
 		pred->pop_n = 2;
-		return filter_add_pred_fn(ps, call, pred, filter_pred_or);
+		fn = filter_pred_or;
+		goto add_pred_fn;
 	}
 
 	field = find_event_field(call, pred->field_name);
@@ -567,9 +584,6 @@ static int filter_add_pred(struct filter_parse_state *ps,
 		else
 			fn = filter_pred_strloc;
 		pred->str_len = field->size;
-		if (pred->op == OP_NE)
-			pred->not = 1;
-		return filter_add_pred_fn(ps, call, pred, fn);
 	} else {
 		if (field->is_signed)
 			ret = strict_strtoll(pred->str_val, 0, &val);
@@ -580,27 +594,33 @@ static int filter_add_pred(struct filter_parse_state *ps,
 			return -EINVAL;
 		}
 		pred->val = val;
-	}
 
-	fn = select_comparison_fn(pred->op, field->size, field->is_signed);
-	if (!fn) {
-		parse_error(ps, FILT_ERR_INVALID_OP, 0);
-		return -EINVAL;
+		fn = select_comparison_fn(pred->op, field->size,
+					  field->is_signed);
+		if (!fn) {
+			parse_error(ps, FILT_ERR_INVALID_OP, 0);
+			return -EINVAL;
+		}
 	}
 
 	if (pred->op == OP_NE)
 		pred->not = 1;
 
-	return filter_add_pred_fn(ps, call, pred, fn);
+add_pred_fn:
+	if (apply)
+		return filter_add_pred_fn(ps, call, pred, fn);
+	return 0;
 }
 
 static int filter_add_subsystem_pred(struct filter_parse_state *ps,
 				     struct event_subsystem *system,
 				     struct filter_pred *pred,
-				     char *filter_string)
+				     char *filter_string,
+				     bool apply)
 {
 	struct ftrace_event_call *call;
 	int err = 0;
+	bool fail = true;
 
 	list_for_each_entry(call, &ftrace_events, list) {
 
@@ -610,16 +630,24 @@ static int filter_add_subsystem_pred(struct filter_parse_state *ps,
 		if (strcmp(call->system, system->name))
 			continue;
 
-		err = filter_add_pred(ps, call, pred);
-		if (err) {
-			filter_free_subsystem_preds(system);
-			parse_error(ps, FILT_ERR_BAD_SUBSYS_FILTER, 0);
-			goto out;
-		}
-		replace_filter_string(call->filter, filter_string);
+		if (call->filter->no_reset)
+			continue;
+
+		err = filter_add_pred(ps, call, pred, apply);
+		if (err)
+			call->filter->no_reset = true;
+		else
+			fail = false;
+
+		if (apply)
+			replace_filter_string(call->filter, filter_string);
 	}
-out:
-	return err;
+
+	if (fail) {
+		parse_error(ps, FILT_ERR_BAD_SUBSYS_FILTER, 0);
+		return err;
+	}
+	return 0;
 }
 
 static void parse_init(struct filter_parse_state *ps,
@@ -978,12 +1006,14 @@ static int check_preds(struct filter_parse_state *ps)
 static int replace_preds(struct event_subsystem *system,
 			 struct ftrace_event_call *call,
 			 struct filter_parse_state *ps,
-			 char *filter_string)
+			 char *filter_string,
+			 bool apply)
 {
 	char *operand1 = NULL, *operand2 = NULL;
 	struct filter_pred *pred;
 	struct postfix_elt *elt;
 	int err;
+	int n_preds = 0;
 
 	err = check_preds(ps);
 	if (err)
@@ -1002,19 +1032,14 @@ static int replace_preds(struct event_subsystem *system,
 			continue;
 		}
 
+		if (n_preds++ == MAX_FILTER_PRED) {
+			parse_error(ps, FILT_ERR_TOO_MANY_PREDS, 0);
+			return -ENOSPC;
+		}
+
 		if (elt->op == OP_AND || elt->op == OP_OR) {
 			pred = create_logical_pred(elt->op);
-			if (call)
-				err = filter_add_pred(ps, call, pred);
-			else
-				err = filter_add_subsystem_pred(ps, system,
-							pred, filter_string);
-			filter_free_pred(pred);
-			if (err)
-				return err;
-
-			operand1 = operand2 = NULL;
-			continue;
+			goto add_pred;
 		}
 
 		if (!operand1 || !operand2) {
@@ -1023,11 +1048,12 @@ static int replace_preds(struct event_subsystem *system,
 		}
 
 		pred = create_pred(elt->op, operand1, operand2);
+add_pred:
 		if (call)
-			err = filter_add_pred(ps, call, pred);
+			err = filter_add_pred(ps, call, pred, true);
 		else
 			err = filter_add_subsystem_pred(ps, system, pred,
-							filter_string);
+						filter_string, apply);
 		filter_free_pred(pred);
 		if (err)
 			return err;
@@ -1068,7 +1094,7 @@ int apply_event_filter(struct ftrace_event_call *call, char *filter_string)
 		goto out;
 	}
 
-	err = replace_preds(NULL, call, ps, filter_string);
+	err = replace_preds(NULL, call, ps, filter_string, true);
 	if (err)
 		append_filter_err(ps, call->filter);
 
@@ -1092,7 +1118,7 @@ int apply_subsystem_event_filter(struct event_subsystem *system,
 	mutex_lock(&event_mutex);
 
 	if (!strcmp(strstrip(filter_string), "0")) {
-		filter_free_subsystem_preds(system);
+		filter_free_subsystem_preds(system, 0);
 		remove_filter_string(system->filter);
 		mutex_unlock(&event_mutex);
 		return 0;
@@ -1103,7 +1129,6 @@ int apply_subsystem_event_filter(struct event_subsystem *system,
 	if (!ps)
 		goto out_unlock;
 
-	filter_free_subsystem_preds(system);
 	replace_filter_string(system->filter, filter_string);
 
 	parse_init(ps, filter_ops, filter_string);
@@ -1113,9 +1138,23 @@ int apply_subsystem_event_filter(struct event_subsystem *system,
 		goto out;
 	}
 
-	err = replace_preds(system, NULL, ps, filter_string);
-	if (err)
+	filter_free_subsystem_preds(system, 1);
+
+	/* try to see the filter can be applied to which events */
+	err = replace_preds(system, NULL, ps, filter_string, false);
+	if (err) {
 		append_filter_err(ps, system->filter);
+		goto out;
+	}
+
+	filter_free_subsystem_preds(system, 2);
+
+	/* really apply the filter to the events */
+	err = replace_preds(system, NULL, ps, filter_string, true);
+	if (err) {
+		append_filter_err(ps, system->filter);
+		filter_free_subsystem_preds(system, 2);
+	}
 
 out:
 	filter_opstack_clear(ps);
-- 
1.6.3

Re: [PATCH] tracing/filters: Improve subsystem filter

[Frederic Weisbecker]

On Thu, Jul 16, 2009 at 05:03:24PM +0800, Li Zefan wrote:
> Currently a subsystem filter should be applicable to all events
> under the subsystem, and if it failed, all the event filters
> will be cleared. Those behaviors make subsys filter much less
> useful:
> 
>   # echo 'vec == 1' > irq/softirq_entry/filter
>   # echo 'irq == 5' > irq/filter
>   bash: echo: write error: Invalid argument
>   # cat irq/softirq_entry/filter
>   none
> 
> I'd expect it set the filter for irq_handler_entry/exit, and
> not touch softirq_entry/exit.
> 
> The basic idea is, try to see if the filter can be applied
> to which events, and then just apply to the those events:
> 
>   # echo 'vec == 1' > softirq_entry/filter
>   # echo 'irq == 5' > filter
>   # cat irq_handler_entry/filter
>   irq == 5
>   # cat softirq_entry/filter
>   vec == 1
> 
> Inspied-by: Steven Rostedt <srostedt@redhat.com>
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  include/linux/ftrace_event.h       |    4 +-
>  kernel/trace/trace.h               |    3 +-
>  kernel/trace/trace_events_filter.c |  123 +++++++++++++++++++++++------------
>  3 files changed, 86 insertions(+), 44 deletions(-)
> 
> diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
> index 5c093ff..26d3673 100644
> --- a/include/linux/ftrace_event.h
> +++ b/include/linux/ftrace_event.h
> @@ -101,6 +101,8 @@ void trace_current_buffer_discard_commit(struct ring_buffer_event *event);
>  
>  void tracing_record_cmdline(struct task_struct *tsk);
>  
> +struct event_filter;
> +
>  struct ftrace_event_call {
>  	struct list_head	list;
>  	char			*name;
> @@ -116,7 +118,7 @@ struct ftrace_event_call {
>  	int			(*define_fields)(void);
>  	struct list_head	fields;
>  	int			filter_active;
> -	void			*filter;
> +	struct event_filter	*filter;
>  	void			*mod;
>  
>  #ifdef CONFIG_EVENT_PROFILE
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 06886a0..3a87d46 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -768,13 +768,14 @@ struct event_filter {
>  	int			n_preds;
>  	struct filter_pred	**preds;
>  	char			*filter_string;
> +	bool			no_reset;
>  };
>  
>  struct event_subsystem {
>  	struct list_head	list;
>  	const char		*name;
>  	struct dentry		*entry;
> -	void			*filter;
> +	struct event_filter	*filter;
>  	int			nr_events;
>  };
>  
> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index 1c80ef7..0f510c2 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -420,7 +420,13 @@ oom:
>  }
>  EXPORT_SYMBOL_GPL(init_preds);
>  
> -static void filter_free_subsystem_preds(struct event_subsystem *system)
> +/*
> + * flag == 0: remove all events' filter
> + * flag == 1: clear filter->no_reset
> + * flag == 2: remove all preds with no_reset == false
> + */



Once an option overlap the boolean binary range, it's better
to start thinking about an enum type, for better self-explaining code.


> +static void filter_free_subsystem_preds(struct event_subsystem *system,
> +					int flag)
>  {
>  	struct ftrace_event_call *call;
>  
> @@ -428,6 +434,14 @@ static void filter_free_subsystem_preds(struct event_subsystem *system)
>  		if (!call->define_fields)
>  			continue;
>  
> +		if (flag == 1) {
> +			call->filter->no_reset = false;
> +			continue;
> +		}
> +
> +		if (flag == 2 && call->filter->no_reset == true)



Or simply if (flag == 2 && call->filter->no_reset)



> +			continue;
> +
>  		if (!strcmp(call->system, system->name)) {
>  			filter_disable_preds(call);
>  			remove_filter_string(call->filter);
> @@ -529,7 +543,8 @@ static filter_pred_fn_t select_comparison_fn(int op, int field_size,
>  
>  static int filter_add_pred(struct filter_parse_state *ps,
>  			   struct ftrace_event_call *call,
> -			   struct filter_pred *pred)
> +			   struct filter_pred *pred,
> +			   bool apply)


bool dry_run sounds perhaps more intuitive for what is happening there.
Because "apply" is surprising in a "add_thing" function, it's like
a field to confirm that we know what what we are doing :)

(May be I start to become a PITA with my naming worries, especially
since I'm usually not good at it in my patches :)

Otherwise, the rest looks good.

Tom, any comment?



>  {
>  	struct ftrace_event_field *field;
>  	filter_pred_fn_t fn;
> @@ -541,10 +556,12 @@ static int filter_add_pred(struct filter_parse_state *ps,
>  
>  	if (pred->op == OP_AND) {
>  		pred->pop_n = 2;
> -		return filter_add_pred_fn(ps, call, pred, filter_pred_and);
> +		fn = filter_pred_and;
> +		goto add_pred_fn;
>  	} else if (pred->op == OP_OR) {
>  		pred->pop_n = 2;
> -		return filter_add_pred_fn(ps, call, pred, filter_pred_or);
> +		fn = filter_pred_or;
> +		goto add_pred_fn;
>  	}
>  
>  	field = find_event_field(call, pred->field_name);
> @@ -567,9 +584,6 @@ static int filter_add_pred(struct filter_parse_state *ps,
>  		else
>  			fn = filter_pred_strloc;
>  		pred->str_len = field->size;
> -		if (pred->op == OP_NE)
> -			pred->not = 1;
> -		return filter_add_pred_fn(ps, call, pred, fn);
>  	} else {
>  		if (field->is_signed)
>  			ret = strict_strtoll(pred->str_val, 0, &val);
> @@ -580,27 +594,33 @@ static int filter_add_pred(struct filter_parse_state *ps,
>  			return -EINVAL;
>  		}
>  		pred->val = val;
> -	}
>  
> -	fn = select_comparison_fn(pred->op, field->size, field->is_signed);
> -	if (!fn) {
> -		parse_error(ps, FILT_ERR_INVALID_OP, 0);
> -		return -EINVAL;
> +		fn = select_comparison_fn(pred->op, field->size,
> +					  field->is_signed);
> +		if (!fn) {
> +			parse_error(ps, FILT_ERR_INVALID_OP, 0);
> +			return -EINVAL;
> +		}
>  	}
>  
>  	if (pred->op == OP_NE)
>  		pred->not = 1;
>  
> -	return filter_add_pred_fn(ps, call, pred, fn);
> +add_pred_fn:
> +	if (apply)
> +		return filter_add_pred_fn(ps, call, pred, fn);
> +	return 0;
>  }
>  
>  static int filter_add_subsystem_pred(struct filter_parse_state *ps,
>  				     struct event_subsystem *system,
>  				     struct filter_pred *pred,
> -				     char *filter_string)
> +				     char *filter_string,
> +				     bool apply)
>  {
>  	struct ftrace_event_call *call;
>  	int err = 0;
> +	bool fail = true;
>  
>  	list_for_each_entry(call, &ftrace_events, list) {
>  
> @@ -610,16 +630,24 @@ static int filter_add_subsystem_pred(struct filter_parse_state *ps,
>  		if (strcmp(call->system, system->name))
>  			continue;
>  
> -		err = filter_add_pred(ps, call, pred);
> -		if (err) {
> -			filter_free_subsystem_preds(system);
> -			parse_error(ps, FILT_ERR_BAD_SUBSYS_FILTER, 0);
> -			goto out;
> -		}
> -		replace_filter_string(call->filter, filter_string);
> +		if (call->filter->no_reset)
> +			continue;
> +
> +		err = filter_add_pred(ps, call, pred, apply);
> +		if (err)
> +			call->filter->no_reset = true;
> +		else
> +			fail = false;
> +
> +		if (apply)
> +			replace_filter_string(call->filter, filter_string);
>  	}
> -out:
> -	return err;
> +
> +	if (fail) {
> +		parse_error(ps, FILT_ERR_BAD_SUBSYS_FILTER, 0);
> +		return err;
> +	}
> +	return 0;
>  }
>  
>  static void parse_init(struct filter_parse_state *ps,
> @@ -978,12 +1006,14 @@ static int check_preds(struct filter_parse_state *ps)
>  static int replace_preds(struct event_subsystem *system,
>  			 struct ftrace_event_call *call,
>  			 struct filter_parse_state *ps,
> -			 char *filter_string)
> +			 char *filter_string,
> +			 bool apply)
>  {
>  	char *operand1 = NULL, *operand2 = NULL;
>  	struct filter_pred *pred;
>  	struct postfix_elt *elt;
>  	int err;
> +	int n_preds = 0;
>  
>  	err = check_preds(ps);
>  	if (err)
> @@ -1002,19 +1032,14 @@ static int replace_preds(struct event_subsystem *system,
>  			continue;
>  		}
>  
> +		if (n_preds++ == MAX_FILTER_PRED) {
> +			parse_error(ps, FILT_ERR_TOO_MANY_PREDS, 0);
> +			return -ENOSPC;
> +		}
> +
>  		if (elt->op == OP_AND || elt->op == OP_OR) {
>  			pred = create_logical_pred(elt->op);
> -			if (call)
> -				err = filter_add_pred(ps, call, pred);
> -			else
> -				err = filter_add_subsystem_pred(ps, system,
> -							pred, filter_string);
> -			filter_free_pred(pred);
> -			if (err)
> -				return err;
> -
> -			operand1 = operand2 = NULL;
> -			continue;
> +			goto add_pred;
>  		}
>  
>  		if (!operand1 || !operand2) {
> @@ -1023,11 +1048,12 @@ static int replace_preds(struct event_subsystem *system,
>  		}
>  
>  		pred = create_pred(elt->op, operand1, operand2);
> +add_pred:
>  		if (call)
> -			err = filter_add_pred(ps, call, pred);
> +			err = filter_add_pred(ps, call, pred, true);
>  		else
>  			err = filter_add_subsystem_pred(ps, system, pred,
> -							filter_string);
> +						filter_string, apply);
>  		filter_free_pred(pred);
>  		if (err)
>  			return err;
> @@ -1068,7 +1094,7 @@ int apply_event_filter(struct ftrace_event_call *call, char *filter_string)
>  		goto out;
>  	}
>  
> -	err = replace_preds(NULL, call, ps, filter_string);
> +	err = replace_preds(NULL, call, ps, filter_string, true);
>  	if (err)
>  		append_filter_err(ps, call->filter);
>  
> @@ -1092,7 +1118,7 @@ int apply_subsystem_event_filter(struct event_subsystem *system,
>  	mutex_lock(&event_mutex);
>  
>  	if (!strcmp(strstrip(filter_string), "0")) {
> -		filter_free_subsystem_preds(system);
> +		filter_free_subsystem_preds(system, 0);
>  		remove_filter_string(system->filter);
>  		mutex_unlock(&event_mutex);
>  		return 0;
> @@ -1103,7 +1129,6 @@ int apply_subsystem_event_filter(struct event_subsystem *system,
>  	if (!ps)
>  		goto out_unlock;
>  
> -	filter_free_subsystem_preds(system);
>  	replace_filter_string(system->filter, filter_string);
>  
>  	parse_init(ps, filter_ops, filter_string);
> @@ -1113,9 +1138,23 @@ int apply_subsystem_event_filter(struct event_subsystem *system,
>  		goto out;
>  	}
>  
> -	err = replace_preds(system, NULL, ps, filter_string);
> -	if (err)
> +	filter_free_subsystem_preds(system, 1);
> +
> +	/* try to see the filter can be applied to which events */
> +	err = replace_preds(system, NULL, ps, filter_string, false);
> +	if (err) {
>  		append_filter_err(ps, system->filter);
> +		goto out;
> +	}
> +
> +	filter_free_subsystem_preds(system, 2);
> +
> +	/* really apply the filter to the events */
> +	err = replace_preds(system, NULL, ps, filter_string, true);
> +	if (err) {
> +		append_filter_err(ps, system->filter);
> +		filter_free_subsystem_preds(system, 2);
> +	}
>  
>  out:
>  	filter_opstack_clear(ps);
> -- 
> 1.6.3
> 
> 
> 
> 

Re: [PATCH] tracing/filters: Improve subsystem filter

[Li Zefan]

>> -static void filter_free_subsystem_preds(struct event_subsystem *system)
>> +/*
>> + * flag == 0: remove all events' filter
>> + * flag == 1: clear filter->no_reset
>> + * flag == 2: remove all preds with no_reset == false
>> + */
> 
> Once an option overlap the boolean binary range, it's better
> to start thinking about an enum type, for better self-explaining code.
> 

Ok.

I was lazy to think of proper names for those enums..

>> +static void filter_free_subsystem_preds(struct event_subsystem *system,
>> +					int flag)
>>  {
>>  	struct ftrace_event_call *call;
>>  
>> @@ -428,6 +434,14 @@ static void filter_free_subsystem_preds(struct event_subsystem *system)
>>  		if (!call->define_fields)
>>  			continue;
>>  
>> +		if (flag == 1) {
>> +			call->filter->no_reset = false;
>> +			continue;
>> +		}
>> +
>> +		if (flag == 2 && call->filter->no_reset == true)
> 
> Or simply if (flag == 2 && call->filter->no_reset)
> 

Sure.

>> +			continue;
>> +
>>  		if (!strcmp(call->system, system->name)) {
>>  			filter_disable_preds(call);
>>  			remove_filter_string(call->filter);
>> @@ -529,7 +543,8 @@ static filter_pred_fn_t select_comparison_fn(int op, int field_size,
>>  
>>  static int filter_add_pred(struct filter_parse_state *ps,
>>  			   struct ftrace_event_call *call,
>> -			   struct filter_pred *pred)
>> +			   struct filter_pred *pred,
>> +			   bool apply)
> 
> 
> bool dry_run sounds perhaps more intuitive for what is happening there.

I guess you "try_run". ;)

> Because "apply" is surprising in a "add_thing" function, it's like
> a field to confirm that we know what what we are doing :)
> 
> (May be I start to become a PITA with my naming worries, especially
> since I'm usually not good at it in my patches :)
> 

I'll take "try_run". Naming is often hard for me.

> Otherwise, the rest looks good.
> 

Thanks!

Re: [PATCH] tracing/filters: Improve subsystem filter

[Frederic Weisbecker]

On Mon, Jul 20, 2009 at 08:59:18AM +0800, Li Zefan wrote:
> >> -static void filter_free_subsystem_preds(struct event_subsystem *system)
> >> +/*
> >> + * flag == 0: remove all events' filter
> >> + * flag == 1: clear filter->no_reset
> >> + * flag == 2: remove all preds with no_reset == false
> >> + */
> > 
> > Once an option overlap the boolean binary range, it's better
> > to start thinking about an enum type, for better self-explaining code.
> > 
> 
> Ok.
> 
> I was lazy to think of proper names for those enums..
> 
> >> +static void filter_free_subsystem_preds(struct event_subsystem *system,
> >> +					int flag)
> >>  {
> >>  	struct ftrace_event_call *call;
> >>  
> >> @@ -428,6 +434,14 @@ static void filter_free_subsystem_preds(struct event_subsystem *system)
> >>  		if (!call->define_fields)
> >>  			continue;
> >>  
> >> +		if (flag == 1) {
> >> +			call->filter->no_reset = false;
> >> +			continue;
> >> +		}
> >> +
> >> +		if (flag == 2 && call->filter->no_reset == true)
> > 
> > Or simply if (flag == 2 && call->filter->no_reset)
> > 
> 
> Sure.
> 
> >> +			continue;
> >> +
> >>  		if (!strcmp(call->system, system->name)) {
> >>  			filter_disable_preds(call);
> >>  			remove_filter_string(call->filter);
> >> @@ -529,7 +543,8 @@ static filter_pred_fn_t select_comparison_fn(int op, int field_size,
> >>  
> >>  static int filter_add_pred(struct filter_parse_state *ps,
> >>  			   struct ftrace_event_call *call,
> >> -			   struct filter_pred *pred)
> >> +			   struct filter_pred *pred,
> >> +			   bool apply)
> > 
> > 
> > bool dry_run sounds perhaps more intuitive for what is happening there.
> 
> I guess you "try_run". ;)
> 
> > Because "apply" is surprising in a "add_thing" function, it's like
> > a field to confirm that we know what what we are doing :)
> > 
> > (May be I start to become a PITA with my naming worries, especially
> > since I'm usually not good at it in my patches :)
> > 
> 
> I'll take "try_run". Naming is often hard for me.


No I really meant "dry run"
"Try run" is already a function that would implicitly fit into
every function :)

Ie, dry run means:

"Behave as if you were truly doing it, just to see if it works.
But don't really do it"


> 
> > Otherwise, the rest looks good.
> > 
> 
> Thanks!

Re: [PATCH] tracing/filters: Improve subsystem filter

[Li Zefan]

>>> bool dry_run sounds perhaps more intuitive for what is happening there.
>> I guess you "try_run". ;)
>>
>>> Because "apply" is surprising in a "add_thing" function, it's like
>>> a field to confirm that we know what what we are doing :)
>>>
>>> (May be I start to become a PITA with my naming worries, especially
>>> since I'm usually not good at it in my patches :)
>>>
>> I'll take "try_run". Naming is often hard for me.
> 
> 
> No I really meant "dry run"
> "Try run" is already a function that would implicitly fit into
> every function :)
> 
> Ie, dry run means:
> 
> "Behave as if you were truly doing it, just to see if it works.
> But don't really do it"
> 

Got it. Thanks. :)