Message-ID: <20110421152038.GA1772@nowhere>
Date:	Thu, 21 Apr 2011 17:20:42 +0200
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Jiri Olsa <jolsa@...hat.com>
Cc:	rostedt@...dmis.org, mingo@...hat.com, a.p.zijlstra@...llo.nl,
	linux-kernel@...r.kernel.org,
	Arnaldo Carvalho de Melo <acme@...hat.com>,
	Tom Zanussi <tzanussi@...il.com>,
	Li Zefan <lizf@...fujitsu.com>
Subject: Re: [RFC,PATCH 3/3] trace,perf: add filter support for
 ftrace/function tracepoint event

On Thu, Apr 21, 2011 at 12:40:58PM +0200, Jiri Olsa wrote:
> Added support to be able to pass "ip == glob" filter for the ftrace/function
> tracepoint.
> 
> Respective functions are hot-patched/updated when the filter is set,
> and the filter predicate is set to be allways true.

Ideally, if the filter is set to "ip", we want to apply your hook
and not make the filter appearing in the tracing path. ie, Instead of
setting a filter with filter_pred_all callback, we should have no filter
to process from ftrace_function_call_*() things.

We probably need to make an exception when we filter parent_ip,
which can't be processed using hotpatching, so we need a real
filter for it.


> Probably missing
> several cases.. not sure this is the way.
> 
> wbr,
> jirka
> 
> ---
>  include/linux/ftrace_event.h       |    3 ++
>  kernel/trace/ftrace.c              |   35 ++++++++++++++++++++++++++++
>  kernel/trace/trace.h               |    2 +
>  kernel/trace/trace_events_filter.c |   45 ++++++++++++++++++++++++++++++++---
>  kernel/trace/trace_export.c        |   26 ++++++++++++++++----
>  5 files changed, 102 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
> index 22b32af..bbf7cc6 100644
> --- a/include/linux/ftrace_event.h
> +++ b/include/linux/ftrace_event.h
> @@ -144,6 +144,8 @@ struct ftrace_event_class {
>  	struct list_head	*(*get_fields)(struct ftrace_event_call *);
>  	struct list_head	fields;
>  	int			(*raw_init)(struct ftrace_event_call *);
> +	void			(*filter)(struct ftrace_event_call *call,
> +					  int enable);

Good, but this should take the parsed filter as a parameter.
But that's also a problem of un-globalizing the function tracer. Several
perf events, and ftrace, should be able to set different filters. Thus
you need to compute a kind of union of all these filters and keep
track of who needs which function.

perf event A may want to trace func1()
perf event B may want to trace func2()

So you need to enable func1() and func2(), although they both
have set different filters for that, but trigger an event on A
only when func1() is called, and trigger an event on B only when func2()
is called.

So we probably need a kind of function hlist to keep track of that.

I started something a while ago:

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
	tracing/hlist

The thing is very outdated now I think, and it needed some rework.

>  };
>  
>  extern int ftrace_event_reg(struct ftrace_event_call *event,
> @@ -221,6 +223,7 @@ enum {
>  	FILTER_STATIC_STRING,
>  	FILTER_DYN_STRING,
>  	FILTER_PTR_STRING,
> +	FILTER_TRACE_FN,

But yeah that kind of interface is cool. Although I would rather
call that FILTER_CUSTOM or FILTER_OVERRIDEN, to avoid confusion
with TRACE_FN, as this is deemed to express the fact we override
the interpretation of the filter. ftrace should be only a specific
user of that. More are perhaps coming.

Also, we may need to override the effects of the filtering endpoint
to express every capabilities of set_ftrace_filter, like
trace_on/trace_off commands. So that we can get rid of that global file in
the end.

One idea was to have a "triggers" directory in each event directories,
and have one intepretation of filters per file:

$ls triggers
filter
trace_on
trace_off

Then, when the user tries:

$ echo "ip == lambda" > trace_off

This simply disables tracing when we hit the lambda function.
Echoing the same in filter would filter when we hit that function.

The filter file in that triggers directory would refer to the same
inode of the filter file in the event directory. We would just keep
the old one for compatibility.

>  };
>  
>  #define EVENT_STORAGE_SIZE 128
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 49fbc8c..b52db6e 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -3285,6 +3285,41 @@ int ftrace_event_class_register(struct ftrace_event_call *call,
>  	return -EINVAL;
>  }
>  
> +void ftrace_event_tracefn_filter(int enable)
> +{
> +	char *filter = event_function.data;
> +
> +	if (enable) {
> +		WARN_ON(!filter);
> +		if (filter)
> +			ftrace_match_records(filter, strlen(filter), 1);
> +	} else {
> +		ftrace_filter_reset(1);
> +		if (filter) {
> +			kfree(filter);
> +			event_function.data = NULL;
> +		}
> +	}
> +
> +	mutex_lock(&ftrace_lock);
> +	if (ftrace_enabled)
> +		ftrace_run_update_code(FTRACE_ENABLE_CALLS);
> +	mutex_unlock(&ftrace_lock);
> +}

Note we may have complicated expressions linked with either && or ||.
Thus you need to be able to compute unions and intersections or
functions sets. The ftrace_match_record() won't be sufficient enough
for that. Best would be to evaluate the whole tree of the parsed expression.

This can be done gradually though, we can start with simple expressions
and reject complicated ones to begin with.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists