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
Changelog for v2:
- do some cleanups to address Frederic's comments.
Inspied-by: Steven Rostedt <[email protected]>
Signed-off-by: Li Zefan <[email protected]>
---
include/linux/ftrace_event.h | 4 +-
kernel/trace/trace.h | 3 +-
kernel/trace/trace_events_filter.c | 124 ++++++++++++++++++++++++------------
3 files changed, 87 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 b9aae72..c7ed427 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -418,7 +418,14 @@ oom:
}
EXPORT_SYMBOL_GPL(init_preds);
-static void filter_free_subsystem_preds(struct event_subsystem *system)
+enum {
+ FILTER_DISABLE_ALL,
+ FILTER_INIT_NO_RESET,
+ FILTER_SKIP_NO_RESET,
+};
+
+static void filter_free_subsystem_preds(struct event_subsystem *system,
+ int flag)
{
struct ftrace_event_call *call;
@@ -426,6 +433,14 @@ static void filter_free_subsystem_preds(struct event_subsystem *system)
if (!call->define_fields)
continue;
+ if (flag == FILTER_INIT_NO_RESET) {
+ call->filter->no_reset = false;
+ continue;
+ }
+
+ if (flag == FILTER_SKIP_NO_RESET && call->filter->no_reset)
+ continue;
+
if (!strcmp(call->system, system->name)) {
filter_disable_preds(call);
remove_filter_string(call->filter);
@@ -527,7 +542,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 dry_run)
{
struct ftrace_event_field *field;
filter_pred_fn_t fn;
@@ -539,10 +555,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);
@@ -565,9 +583,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);
@@ -578,27 +593,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 (!dry_run)
+ 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 dry_run)
{
struct ftrace_event_call *call;
int err = 0;
+ bool fail = true;
list_for_each_entry(call, &ftrace_events, list) {
@@ -608,16 +629,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, dry_run);
+ if (err)
+ call->filter->no_reset = true;
+ else
+ fail = false;
+
+ if (!dry_run)
+ 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,
@@ -976,12 +1005,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 dry_run)
{
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)
@@ -1000,19 +1031,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) {
@@ -1021,11 +1047,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, false);
else
err = filter_add_subsystem_pred(ps, system, pred,
- filter_string);
+ filter_string, dry_run);
filter_free_pred(pred);
if (err)
return err;
@@ -1066,7 +1093,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, false);
if (err)
append_filter_err(ps, call->filter);
@@ -1090,7 +1117,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, FILTER_DISABLE_ALL);
remove_filter_string(system->filter);
mutex_unlock(&event_mutex);
return 0;
@@ -1101,7 +1128,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);
@@ -1111,9 +1137,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, FILTER_INIT_NO_RESET);
+
+ /* try to see the filter can be applied to which events */
+ err = replace_preds(system, NULL, ps, filter_string, true);
+ if (err) {
+ append_filter_err(ps, system->filter);
+ goto out;
+ }
+
+ filter_free_subsystem_preds(system, FILTER_SKIP_NO_RESET);
+
+ /* really apply the filter to the events */
+ err = replace_preds(system, NULL, ps, filter_string, false);
+ if (err) {
append_filter_err(ps, system->filter);
+ filter_free_subsystem_preds(system, 2);
+ }
out:
filter_opstack_clear(ps);
--
1.6.3
On Mon, 20 Jul 2009, 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
>
> Changelog for v2:
> - do some cleanups to address Frederic's comments.
>
> Inspied-by: Steven Rostedt <[email protected]>
> Signed-off-by: Li Zefan <[email protected]>
Nice work Li!
I'll pull it in and give it some tests. I believe Frederic is traveling
today, but if it works well I'll get it ready to push. Unless Tom has any
rejections to it.
Thanks!
-- Steve
> ---
> include/linux/ftrace_event.h | 4 +-
> kernel/trace/trace.h | 3 +-
> kernel/trace/trace_events_filter.c | 124 ++++++++++++++++++++++++------------
> 3 files changed, 87 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 b9aae72..c7ed427 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -418,7 +418,14 @@ oom:
> }
> EXPORT_SYMBOL_GPL(init_preds);
>
> -static void filter_free_subsystem_preds(struct event_subsystem *system)
> +enum {
> + FILTER_DISABLE_ALL,
> + FILTER_INIT_NO_RESET,
> + FILTER_SKIP_NO_RESET,
> +};
> +
> +static void filter_free_subsystem_preds(struct event_subsystem *system,
> + int flag)
> {
> struct ftrace_event_call *call;
>
> @@ -426,6 +433,14 @@ static void filter_free_subsystem_preds(struct event_subsystem *system)
> if (!call->define_fields)
> continue;
>
> + if (flag == FILTER_INIT_NO_RESET) {
> + call->filter->no_reset = false;
> + continue;
> + }
> +
> + if (flag == FILTER_SKIP_NO_RESET && call->filter->no_reset)
> + continue;
> +
> if (!strcmp(call->system, system->name)) {
> filter_disable_preds(call);
> remove_filter_string(call->filter);
> @@ -527,7 +542,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 dry_run)
> {
> struct ftrace_event_field *field;
> filter_pred_fn_t fn;
> @@ -539,10 +555,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);
> @@ -565,9 +583,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);
> @@ -578,27 +593,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 (!dry_run)
> + 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 dry_run)
> {
> struct ftrace_event_call *call;
> int err = 0;
> + bool fail = true;
>
> list_for_each_entry(call, &ftrace_events, list) {
>
> @@ -608,16 +629,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, dry_run);
> + if (err)
> + call->filter->no_reset = true;
> + else
> + fail = false;
> +
> + if (!dry_run)
> + 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,
> @@ -976,12 +1005,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 dry_run)
> {
> 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)
> @@ -1000,19 +1031,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) {
> @@ -1021,11 +1047,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, false);
> else
> err = filter_add_subsystem_pred(ps, system, pred,
> - filter_string);
> + filter_string, dry_run);
> filter_free_pred(pred);
> if (err)
> return err;
> @@ -1066,7 +1093,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, false);
> if (err)
> append_filter_err(ps, call->filter);
>
> @@ -1090,7 +1117,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, FILTER_DISABLE_ALL);
> remove_filter_string(system->filter);
> mutex_unlock(&event_mutex);
> return 0;
> @@ -1101,7 +1128,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);
> @@ -1111,9 +1137,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, FILTER_INIT_NO_RESET);
> +
> + /* try to see the filter can be applied to which events */
> + err = replace_preds(system, NULL, ps, filter_string, true);
> + if (err) {
> + append_filter_err(ps, system->filter);
> + goto out;
> + }
> +
> + filter_free_subsystem_preds(system, FILTER_SKIP_NO_RESET);
> +
> + /* really apply the filter to the events */
> + err = replace_preds(system, NULL, ps, filter_string, false);
> + if (err) {
> append_filter_err(ps, system->filter);
> + filter_free_subsystem_preds(system, 2);
> + }
>
> out:
> filter_opstack_clear(ps);
> --
> 1.6.3
>
On Mon, Jul 20, 2009 at 12:08:52PM -0400, Steven Rostedt wrote:
>
>
> On Mon, 20 Jul 2009, 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
> >
> > Changelog for v2:
> > - do some cleanups to address Frederic's comments.
> >
> > Inspied-by: Steven Rostedt <[email protected]>
> > Signed-off-by: Li Zefan <[email protected]>
>
> Nice work Li!
>
> I'll pull it in and give it some tests. I believe Frederic is traveling
> today, but if it works well I'll get it ready to push. Unless Tom has any
> rejections to it.
Not yet traveling but very soon :)
No problem for me anyway.
Acked-by: Frederic Weisbecker <[email protected]>
Thanks.
Frederic.
> Thanks!
>
> -- Steve
>
>
> > ---
> > include/linux/ftrace_event.h | 4 +-
> > kernel/trace/trace.h | 3 +-
> > kernel/trace/trace_events_filter.c | 124 ++++++++++++++++++++++++------------
> > 3 files changed, 87 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 b9aae72..c7ed427 100644
> > --- a/kernel/trace/trace_events_filter.c
> > +++ b/kernel/trace/trace_events_filter.c
> > @@ -418,7 +418,14 @@ oom:
> > }
> > EXPORT_SYMBOL_GPL(init_preds);
> >
> > -static void filter_free_subsystem_preds(struct event_subsystem *system)
> > +enum {
> > + FILTER_DISABLE_ALL,
> > + FILTER_INIT_NO_RESET,
> > + FILTER_SKIP_NO_RESET,
> > +};
> > +
> > +static void filter_free_subsystem_preds(struct event_subsystem *system,
> > + int flag)
> > {
> > struct ftrace_event_call *call;
> >
> > @@ -426,6 +433,14 @@ static void filter_free_subsystem_preds(struct event_subsystem *system)
> > if (!call->define_fields)
> > continue;
> >
> > + if (flag == FILTER_INIT_NO_RESET) {
> > + call->filter->no_reset = false;
> > + continue;
> > + }
> > +
> > + if (flag == FILTER_SKIP_NO_RESET && call->filter->no_reset)
> > + continue;
> > +
> > if (!strcmp(call->system, system->name)) {
> > filter_disable_preds(call);
> > remove_filter_string(call->filter);
> > @@ -527,7 +542,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 dry_run)
> > {
> > struct ftrace_event_field *field;
> > filter_pred_fn_t fn;
> > @@ -539,10 +555,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);
> > @@ -565,9 +583,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);
> > @@ -578,27 +593,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 (!dry_run)
> > + 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 dry_run)
> > {
> > struct ftrace_event_call *call;
> > int err = 0;
> > + bool fail = true;
> >
> > list_for_each_entry(call, &ftrace_events, list) {
> >
> > @@ -608,16 +629,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, dry_run);
> > + if (err)
> > + call->filter->no_reset = true;
> > + else
> > + fail = false;
> > +
> > + if (!dry_run)
> > + 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,
> > @@ -976,12 +1005,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 dry_run)
> > {
> > 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)
> > @@ -1000,19 +1031,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) {
> > @@ -1021,11 +1047,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, false);
> > else
> > err = filter_add_subsystem_pred(ps, system, pred,
> > - filter_string);
> > + filter_string, dry_run);
> > filter_free_pred(pred);
> > if (err)
> > return err;
> > @@ -1066,7 +1093,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, false);
> > if (err)
> > append_filter_err(ps, call->filter);
> >
> > @@ -1090,7 +1117,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, FILTER_DISABLE_ALL);
> > remove_filter_string(system->filter);
> > mutex_unlock(&event_mutex);
> > return 0;
> > @@ -1101,7 +1128,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);
> > @@ -1111,9 +1137,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, FILTER_INIT_NO_RESET);
> > +
> > + /* try to see the filter can be applied to which events */
> > + err = replace_preds(system, NULL, ps, filter_string, true);
> > + if (err) {
> > + append_filter_err(ps, system->filter);
> > + goto out;
> > + }
> > +
> > + filter_free_subsystem_preds(system, FILTER_SKIP_NO_RESET);
> > +
> > + /* really apply the filter to the events */
> > + err = replace_preds(system, NULL, ps, filter_string, false);
> > + if (err) {
> > append_filter_err(ps, system->filter);
> > + filter_free_subsystem_preds(system, 2);
> > + }
> >
> > out:
> > filter_opstack_clear(ps);
> > --
> > 1.6.3
> >
On Mon, 20 Jul 2009, Steven Rostedt wrote:
>
>
> On Mon, 20 Jul 2009, 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
OK, I tried this and it worked as you described, but it also does
something that I did not like. Basically this:
# echo 'vec == 1' > filter
# cat softirq_entry/filter
vec == 1
# cat irq_handler_entry/filter
none
# cat filter
vec == 1
# echo 'irq == 5' > filter
# cat irq_handler_entry/filter
irq == 5
# cat softirq_entry/filter
vec == 1
# cat filter
irq == 5
That last "cat filter" should show:
vec == 1
irq == 5
I can keep this version, since I do find it useful (I've now come to the
conclusion that having the filter work only for those filters that it
would work for is fine). But the top level filter should show all filters.
You can make a patch on top of this one, and I'll just keep this one in my
queue.
Thanks,
-- Steve
> >
> > Changelog for v2:
> > - do some cleanups to address Frederic's comments.
> >
> > Inspied-by: Steven Rostedt <[email protected]>
> > Signed-off-by: Li Zefan <[email protected]>
>
> Nice work Li!
>
> I'll pull it in and give it some tests. I believe Frederic is traveling
> today, but if it works well I'll get it ready to push. Unless Tom has any
> rejections to it.
>
> Thanks!
>
> -- Steve
>
>
> > ---
> > include/linux/ftrace_event.h | 4 +-
> > kernel/trace/trace.h | 3 +-
> > kernel/trace/trace_events_filter.c | 124 ++++++++++++++++++++++++------------
> > 3 files changed, 87 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 b9aae72..c7ed427 100644
> > --- a/kernel/trace/trace_events_filter.c
> > +++ b/kernel/trace/trace_events_filter.c
> > @@ -418,7 +418,14 @@ oom:
> > }
> > EXPORT_SYMBOL_GPL(init_preds);
> >
> > -static void filter_free_subsystem_preds(struct event_subsystem *system)
> > +enum {
> > + FILTER_DISABLE_ALL,
> > + FILTER_INIT_NO_RESET,
> > + FILTER_SKIP_NO_RESET,
> > +};
> > +
> > +static void filter_free_subsystem_preds(struct event_subsystem *system,
> > + int flag)
> > {
> > struct ftrace_event_call *call;
> >
> > @@ -426,6 +433,14 @@ static void filter_free_subsystem_preds(struct event_subsystem *system)
> > if (!call->define_fields)
> > continue;
> >
> > + if (flag == FILTER_INIT_NO_RESET) {
> > + call->filter->no_reset = false;
> > + continue;
> > + }
> > +
> > + if (flag == FILTER_SKIP_NO_RESET && call->filter->no_reset)
> > + continue;
> > +
> > if (!strcmp(call->system, system->name)) {
> > filter_disable_preds(call);
> > remove_filter_string(call->filter);
> > @@ -527,7 +542,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 dry_run)
> > {
> > struct ftrace_event_field *field;
> > filter_pred_fn_t fn;
> > @@ -539,10 +555,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);
> > @@ -565,9 +583,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);
> > @@ -578,27 +593,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 (!dry_run)
> > + 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 dry_run)
> > {
> > struct ftrace_event_call *call;
> > int err = 0;
> > + bool fail = true;
> >
> > list_for_each_entry(call, &ftrace_events, list) {
> >
> > @@ -608,16 +629,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, dry_run);
> > + if (err)
> > + call->filter->no_reset = true;
> > + else
> > + fail = false;
> > +
> > + if (!dry_run)
> > + 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,
> > @@ -976,12 +1005,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 dry_run)
> > {
> > 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)
> > @@ -1000,19 +1031,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) {
> > @@ -1021,11 +1047,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, false);
> > else
> > err = filter_add_subsystem_pred(ps, system, pred,
> > - filter_string);
> > + filter_string, dry_run);
> > filter_free_pred(pred);
> > if (err)
> > return err;
> > @@ -1066,7 +1093,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, false);
> > if (err)
> > append_filter_err(ps, call->filter);
> >
> > @@ -1090,7 +1117,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, FILTER_DISABLE_ALL);
> > remove_filter_string(system->filter);
> > mutex_unlock(&event_mutex);
> > return 0;
> > @@ -1101,7 +1128,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);
> > @@ -1111,9 +1137,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, FILTER_INIT_NO_RESET);
> > +
> > + /* try to see the filter can be applied to which events */
> > + err = replace_preds(system, NULL, ps, filter_string, true);
> > + if (err) {
> > + append_filter_err(ps, system->filter);
> > + goto out;
> > + }
> > +
> > + filter_free_subsystem_preds(system, FILTER_SKIP_NO_RESET);
> > +
> > + /* really apply the filter to the events */
> > + err = replace_preds(system, NULL, ps, filter_string, false);
> > + if (err) {
> > append_filter_err(ps, system->filter);
> > + filter_free_subsystem_preds(system, 2);
> > + }
> >
> > out:
> > filter_opstack_clear(ps);
> > --
> > 1.6.3
> >
>
Steven Rostedt wrote:
> On Mon, 20 Jul 2009, Steven Rostedt wrote:
>
>>
>> On Mon, 20 Jul 2009, 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
>
> OK, I tried this and it worked as you described, but it also does
> something that I did not like. Basically this:
>
> # echo 'vec == 1' > filter
> # cat softirq_entry/filter
> vec == 1
> # cat irq_handler_entry/filter
> none
> # cat filter
> vec == 1
> # echo 'irq == 5' > filter
> # cat irq_handler_entry/filter
> irq == 5
> # cat softirq_entry/filter
> vec == 1
> # cat filter
> irq == 5
>
>
> That last "cat filter" should show:
> vec == 1
> irq == 5
>
But I did not change the read semantic. For example, when
subsystem filter is not set at all:
# echo 'irq == 5' > irq_handler_entry/filter
# echo 'vec == 1' > softirq_entry/filter
# cat filter
none
It'll show "none".
> I can keep this version, since I do find it useful (I've now come to the
> conclusion that having the filter work only for those filters that it
> would work for is fine). But the top level filter should show all filters.
>
As I said in an old mail, I think the subsystem filter is much more
useful in "write" than in "read".
I have no strong option about its read semantic, but I can do what
you like.
Suppose:
irq_handler_entry/filter -> irq == 5
irq_handler_exit/filter -> none
softirq_entry/filter -> vec == 1
softirq_exit/filter -> vec == 2
Proposal 1:
# cat filter
irq == 5
vec == 1
vec == 2
Proposal 2:
# cat filter
irq_handler_entry: irq == 5
irq_handler_exit: none
softirq_entry: vec == 1
softirq_exit: vec == 2
Which do you think is better? Or do you have some better idea?
And in the failure case:
# echo 'irq == not_num' > filter
bash: echo: write error: Invalid argument
1:
# cat filter
(still shows filters in each event like above)
or shows error message (the current behavior)
2:
# cat filter
irq == not_num
^
parse_error: Couldn't find or set field in one of a subsystem's events
?
On Tue, 21 Jul 2009, Li Zefan wrote:
>
> But I did not change the read semantic. For example, when
> subsystem filter is not set at all:
>
> # echo 'irq == 5' > irq_handler_entry/filter
> # echo 'vec == 1' > softirq_entry/filter
> # cat filter
> none
>
> It'll show "none".
Ah, good point. Makes me feel even better about applying your patch ;-)
>
> > I can keep this version, since I do find it useful (I've now come to the
> > conclusion that having the filter work only for those filters that it
> > would work for is fine). But the top level filter should show all filters.
> >
>
> As I said in an old mail, I think the subsystem filter is much more
> useful in "write" than in "read".
>
> I have no strong option about its read semantic, but I can do what
> you like.
>
> Suppose:
> irq_handler_entry/filter -> irq == 5
> irq_handler_exit/filter -> none
> softirq_entry/filter -> vec == 1
> softirq_exit/filter -> vec == 2
>
> Proposal 1:
>
> # cat filter
> irq == 5
> vec == 1
> vec == 2
That's a bit confusing.
>
> Proposal 2:
>
> # cat filter
> irq_handler_entry: irq == 5
> irq_handler_exit: none
> softirq_entry: vec == 1
> softirq_exit: vec == 2
I like proposal 2, it is very intuitive.
>
> Which do you think is better? Or do you have some better idea?
>
> And in the failure case:
>
> # echo 'irq == not_num' > filter
> bash: echo: write error: Invalid argument
>
> 1:
> # cat filter
> (still shows filters in each event like above)
>
> or shows error message (the current behavior)
No need to show error messages of failed filter modifications in the
"filter" file.
>
> 2:
> # cat filter
> irq == not_num
> ^
> parse_error: Couldn't find or set field in one of a subsystem's events
This looks good, BUT, it is too much. If you want to implement an error
message like the above, it should probably be a "pr_info()" thing.
-- Steve
>> Proposal 2:
>>
>> # cat filter
>> irq_handler_entry: irq == 5
>> irq_handler_exit: none
>> softirq_entry: vec == 1
>> softirq_exit: vec == 2
>
> I like proposal 2, it is very intuitive.
>
Me too.
>> Which do you think is better? Or do you have some better idea?
>>
>> And in the failure case:
>>
>> # echo 'irq == not_num' > filter
>> bash: echo: write error: Invalid argument
>>
>> 1:
>> # cat filter
>> (still shows filters in each event like above)
>>
>> or shows error message (the current behavior)
>
> No need to show error messages of failed filter modifications in the
> "filter" file.
>
>> 2:
>> # cat filter
>> irq == not_num
>> ^
>> parse_error: Couldn't find or set field in one of a subsystem's events
>
> This looks good, BUT, it is too much. If you want to implement an error
> message like the above, it should probably be a "pr_info()" thing.
>
Yeah, I think it's too much too, but that's exactly what we have.
And I posted a patch to remove those error messages, but Tom and
Frederic didn't seem to like it:
http://lkml.org/lkml/2009/6/17/89
On Tue, 21 Jul 2009, Li Zefan wrote:
> >> 2:
> >> # cat filter
> >> irq == not_num
> >> ^
> >> parse_error: Couldn't find or set field in one of a subsystem's events
> >
> > This looks good, BUT, it is too much. If you want to implement an error
> > message like the above, it should probably be a "pr_info()" thing.
> >
>
> Yeah, I think it's too much too, but that's exactly what we have.
> And I posted a patch to remove those error messages, but Tom and
> Frederic didn't seem to like it:
>
> http://lkml.org/lkml/2009/6/17/89
>
Ah, I forgot about that thread. Well, Frederic did mention making a
"filter_error" file. That sounds like a good solution to me. But I have no
strong opinions on this one way or another.
-- Steve
Hi,
On Mon, 2009-07-20 at 10:20 +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
>
It looks like it accomplishes the goal, but I wonder if you could
simplify it by getting rid of the no_reset flag and all the state-saving
while keeping the dry_run param on filter_add_pred to use from
filter_add_subsysystem_pred() e.g. something like this:
filter_add_subsystem_pred(...)
{
...
list_for_each_entry(call, &ftrace_events, list) {
err = filter_add_pred(ps, call, pred, dry_run = 1);
if (!err)
filter_add_pred(ps, call, pred, dry_run = 0);
...
}
}
IIRC the state-saving was necessary if the idea was to back out of (or
avoid setting) all the filters if one failed, but the new model is to
set whichever apply while leaving the rest alone. I think the simpler
approach above accomplishes that, but I may be missing something or have
forgotten the original motivation for doing it this way.
Tom
> Changelog for v2:
> - do some cleanups to address Frederic's comments.
>
> Inspied-by: Steven Rostedt <[email protected]>
> Signed-off-by: Li Zefan <[email protected]>
> ---
> include/linux/ftrace_event.h | 4 +-
> kernel/trace/trace.h | 3 +-
> kernel/trace/trace_events_filter.c | 124 ++++++++++++++++++++++++------------
> 3 files changed, 87 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 b9aae72..c7ed427 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -418,7 +418,14 @@ oom:
> }
> EXPORT_SYMBOL_GPL(init_preds);
>
> -static void filter_free_subsystem_preds(struct event_subsystem *system)
> +enum {
> + FILTER_DISABLE_ALL,
> + FILTER_INIT_NO_RESET,
> + FILTER_SKIP_NO_RESET,
> +};
> +
> +static void filter_free_subsystem_preds(struct event_subsystem *system,
> + int flag)
> {
> struct ftrace_event_call *call;
>
> @@ -426,6 +433,14 @@ static void filter_free_subsystem_preds(struct event_subsystem *system)
> if (!call->define_fields)
> continue;
>
> + if (flag == FILTER_INIT_NO_RESET) {
> + call->filter->no_reset = false;
> + continue;
> + }
> +
> + if (flag == FILTER_SKIP_NO_RESET && call->filter->no_reset)
> + continue;
> +
> if (!strcmp(call->system, system->name)) {
> filter_disable_preds(call);
> remove_filter_string(call->filter);
> @@ -527,7 +542,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 dry_run)
> {
> struct ftrace_event_field *field;
> filter_pred_fn_t fn;
> @@ -539,10 +555,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);
> @@ -565,9 +583,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);
> @@ -578,27 +593,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 (!dry_run)
> + 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 dry_run)
> {
> struct ftrace_event_call *call;
> int err = 0;
> + bool fail = true;
>
> list_for_each_entry(call, &ftrace_events, list) {
>
> @@ -608,16 +629,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, dry_run);
> + if (err)
> + call->filter->no_reset = true;
> + else
> + fail = false;
> +
> + if (!dry_run)
> + 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,
> @@ -976,12 +1005,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 dry_run)
> {
> 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)
> @@ -1000,19 +1031,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) {
> @@ -1021,11 +1047,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, false);
> else
> err = filter_add_subsystem_pred(ps, system, pred,
> - filter_string);
> + filter_string, dry_run);
> filter_free_pred(pred);
> if (err)
> return err;
> @@ -1066,7 +1093,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, false);
> if (err)
> append_filter_err(ps, call->filter);
>
> @@ -1090,7 +1117,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, FILTER_DISABLE_ALL);
> remove_filter_string(system->filter);
> mutex_unlock(&event_mutex);
> return 0;
> @@ -1101,7 +1128,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);
> @@ -1111,9 +1137,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, FILTER_INIT_NO_RESET);
> +
> + /* try to see the filter can be applied to which events */
> + err = replace_preds(system, NULL, ps, filter_string, true);
> + if (err) {
> + append_filter_err(ps, system->filter);
> + goto out;
> + }
> +
> + filter_free_subsystem_preds(system, FILTER_SKIP_NO_RESET);
> +
> + /* really apply the filter to the events */
> + err = replace_preds(system, NULL, ps, filter_string, false);
> + if (err) {
> append_filter_err(ps, system->filter);
> + filter_free_subsystem_preds(system, 2);
> + }
>
> out:
> filter_opstack_clear(ps);
On Tue, 2009-07-21 at 09:46 +0800, Li Zefan wrote:
> >> Proposal 2:
> >>
> >> # cat filter
> >> irq_handler_entry: irq == 5
> >> irq_handler_exit: none
> >> softirq_entry: vec == 1
> >> softirq_exit: vec == 2
> >
> > I like proposal 2, it is very intuitive.
> >
>
> Me too.
Me three. It's nice to see all the filters for the subsystem in one
place without having to descend into the event subdirs. It might also
be nice to see which events are enabled by looking at the subsystem
'enable' file too. Or maybe the subsystem filter file should show only
filters for enabled events...
>
> >> Which do you think is better? Or do you have some better idea?
> >>
> >> And in the failure case:
> >>
> >> # echo 'irq == not_num' > filter
> >> bash: echo: write error: Invalid argument
> >>
> >> 1:
> >> # cat filter
> >> (still shows filters in each event like above)
> >>
> >> or shows error message (the current behavior)
> >
> > No need to show error messages of failed filter modifications in the
> > "filter" file.
> >
> >> 2:
> >> # cat filter
> >> irq == not_num
> >> ^
> >> parse_error: Couldn't find or set field in one of a subsystem's events
> >
> > This looks good, BUT, it is too much. If you want to implement an error
> > message like the above, it should probably be a "pr_info()" thing.
> >
>
> Yeah, I think it's too much too, but that's exactly what we have.
> And I posted a patch to remove those error messages, but Tom and
> Frederic didn't seem to like it:
>
> http://lkml.org/lkml/2009/6/17/89
It made sense to me to overload the filter file for individual events
this way since error messages (which I still think are useful) and valid
filters are mutually exclusive. But that's not the case for the
subsystem filter files, so for those maybe a filter_error file makes
sense. Maybe for consistency, it also makes sense for the individual
events too - I don't really have a strong opinion either way.
Tom
Tom Zanussi wrote:
> Hi,
>
> On Mon, 2009-07-20 at 10:20 +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
>>
>
> It looks like it accomplishes the goal, but I wonder if you could
> simplify it by getting rid of the no_reset flag and all the state-saving
> while keeping the dry_run param on filter_add_pred to use from
> filter_add_subsysystem_pred() e.g. something like this:
>
> filter_add_subsystem_pred(...)
> {
> ...
> list_for_each_entry(call, &ftrace_events, list) {
> err = filter_add_pred(ps, call, pred, dry_run = 1);
> if (!err)
> filter_add_pred(ps, call, pred, dry_run = 0);
This is the same with just call "filter_add_pred(ps, call, pred)"
> ...
> }
> }
>
> IIRC the state-saving was necessary if the idea was to back out of (or
> avoid setting) all the filters if one failed, but the new model is to
> set whichever apply while leaving the rest alone. I think the simpler
> approach above accomplishes that, but I may be missing something or have
> forgotten the original motivation for doing it this way.
>
This won't work as expected. The state-saving is necessary, because
if any pred is not appliable for an event, the whole operation should
fail for that event.
What this patch does is apply the filter to those events that the
filter is appliable, but not apply parts of the filter to each event.
Imaging the result for those filters with the above code:
# echo 'irq == 5 && vec == 1' > filter
# echo 'irq == 5 || vec == 1' > filter
# echo 'irq == 5 || foo == bar' > fitler
# echo '(irq == 5 && vec == 1) || irq == 2' > filter
On Tue, 2009-07-21 at 14:35 +0800, Li Zefan wrote:
> Tom Zanussi wrote:
> > Hi,
> >
> > On Mon, 2009-07-20 at 10:20 +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
> >>
> >
> > It looks like it accomplishes the goal, but I wonder if you could
> > simplify it by getting rid of the no_reset flag and all the state-saving
> > while keeping the dry_run param on filter_add_pred to use from
> > filter_add_subsysystem_pred() e.g. something like this:
> >
> > filter_add_subsystem_pred(...)
> > {
> > ...
> > list_for_each_entry(call, &ftrace_events, list) {
> > err = filter_add_pred(ps, call, pred, dry_run = 1);
> > if (!err)
> > filter_add_pred(ps, call, pred, dry_run = 0);
>
> This is the same with just call "filter_add_pred(ps, call, pred)"
>
> > ...
> > }
> > }
> >
> > IIRC the state-saving was necessary if the idea was to back out of (or
> > avoid setting) all the filters if one failed, but the new model is to
> > set whichever apply while leaving the rest alone. I think the simpler
> > approach above accomplishes that, but I may be missing something or have
> > forgotten the original motivation for doing it this way.
> >
>
> This won't work as expected. The state-saving is necessary, because
> if any pred is not appliable for an event, the whole operation should
> fail for that event.
>
> What this patch does is apply the filter to those events that the
> filter is appliable, but not apply parts of the filter to each event.
>
Yeah, you're right - I was confusing preds with filters when reading
this.
Acked-by: Tom Zanussi <[email protected]>
> Imaging the result for those filters with the above code:
>
> # echo 'irq == 5 && vec == 1' > filter
> # echo 'irq == 5 || vec == 1' > filter
> # echo 'irq == 5 || foo == bar' > fitler
> # echo '(irq == 5 && vec == 1) || irq == 2' > filter
>