[PATCH 1/2] trace_events_filter: use rcu_assign_pointer() when setting ftrace_event_call->filter

[PATCH 1/2] trace_events_filter: use rcu_assign_pointer() when setting ftrace_event_call->filter

[Tejun Heo]

ftrace_event_call->filter is sched RCU protected but didn't use
rcu_assign_pointer().  Fix it.

TODO: Add proper __rcu annotation to call->filter and all its users.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 kernel/trace/trace_events_filter.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Index: work/kernel/trace/trace_events_filter.c
===================================================================
--- work.orig/kernel/trace/trace_events_filter.c
+++ work/kernel/trace/trace_events_filter.c
@@ -1686,7 +1686,7 @@ static int replace_system_preds(struct e
 		 * replace the filter for the call.
 		 */
 		filter = call->filter;
-		call->filter = filter_item->filter;
+		rcu_assign_pointer(call->filter, filter_item->filter);
 		filter_item->filter = filter;
 
 		fail = false;
@@ -1741,7 +1741,7 @@ int apply_event_filter(struct ftrace_eve
 		filter = call->filter;
 		if (!filter)
 			goto out_unlock;
-		call->filter = NULL;
+		rcu_assign_pointer(call->filter, NULL);
 		/* Make sure the filter is not being used */
 		synchronize_sched();
 		__free_filter(filter);
@@ -1782,7 +1782,7 @@ out:
 	 * string
 	 */
 	tmp = call->filter;
-	call->filter = filter;
+	rcu_assign_pointer(call->filter, filter);
 	if (tmp) {
 		/* Make sure the call is done with the filter */
 		synchronize_sched();

[PATCH 2/2] trace_event_filter: factorize filter creation

[Tejun Heo]

There are four places where new filter for a given filter string is
created, which involves several different steps.  This patch factors
those steps into create_[system_]filter() functions which in turn make
use of create_filter_{start|finish}() for common parts.

The only functional change is that if replace_filter_string() is
requested and fails, creation fails without any side effect instead of
being ignored.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
These two are on top of the current linus/master bbbc4791cd "Merge
branch 'staging-linus' of git://git.kernel.org/.../gregkh/staging" as
tip seems to be lagging behind for the moment.

Thank you.

 kernel/trace/trace_events_filter.c |  274 ++++++++++++++++++-------------------
 1 file changed, 137 insertions(+), 137 deletions(-)

Index: work/kernel/trace/trace_events_filter.c
===================================================================
--- work.orig/kernel/trace/trace_events_filter.c
+++ work/kernel/trace/trace_events_filter.c
@@ -1727,11 +1727,114 @@ static int replace_system_preds(struct e
 	return -ENOMEM;
 }
 
-int apply_event_filter(struct ftrace_event_call *call, char *filter_string)
+static int create_filter_start(char *filter_str, bool set_str,
+			       struct filter_parse_state **psp,
+			       struct event_filter **filterp)
 {
-	struct filter_parse_state *ps;
 	struct event_filter *filter;
-	struct event_filter *tmp;
+	struct filter_parse_state *ps = NULL;
+	int err = 0;
+
+	WARN_ON_ONCE(*psp || *filterp);
+
+	/* allocate everything, and if any fails, free all and fail */
+	filter = __alloc_filter();
+	if (filter && set_str)
+		err = replace_filter_string(filter, filter_str);
+
+	ps = kzalloc(sizeof(*ps), GFP_KERNEL);
+
+	if (!filter || !ps || err) {
+		kfree(ps);
+		__free_filter(filter);
+		return -ENOMEM;
+	}
+
+	/* we're committed to creating a new filter */
+	*filterp = filter;
+	*psp = ps;
+
+	parse_init(ps, filter_ops, filter_str);
+	err = filter_parse(ps);
+	if (err && set_str)
+		append_filter_err(ps, filter);
+	return err;
+}
+
+static void create_filter_finish(struct filter_parse_state *ps)
+{
+	if (ps) {
+		filter_opstack_clear(ps);
+		postfix_clear(ps);
+		kfree(ps);
+	}
+}
+
+/**
+ * create_filter - create a filter for a ftrace_event_call
+ * @call: ftrace_event_call to create a filter for
+ * @filter_str: filter string
+ * @set_str: remember @filter_str and enable detailed error in filter
+ * @filterp: out param for created filter (must be %NULL on entry, may not
+ *	     be %NULL after failure)
+ *
+ * Creates a filter for @call with @filter_str.  If @set_str is %true,
+ * @filter_str is copied and recorded in the new filter.
+ *
+ * On success, returns 0 and *@filterp points to the new filter.  On
+ * failure, returns -errno and *@filterp may point to %NULL or to a new
+ * filter.  In the latter case, the returned filter contains error
+ * information if @set_str is %true and the caller is responsible for
+ * freeing it.
+ */
+static int create_filter(struct ftrace_event_call *call,
+			 char *filter_str, bool set_str,
+			 struct event_filter **filterp)
+{
+	struct filter_parse_state *ps = NULL;
+	int err;
+
+	err = create_filter_start(filter_str, set_str, &ps, filterp);
+	if (!err) {
+		err = replace_preds(call, *filterp, ps, filter_str, false);
+		if (err && set_str)
+			append_filter_err(ps, *filterp);
+	}
+	create_filter_finish(ps);
+
+	return err;
+}
+
+/**
+ * create_system_filter - create a filter for an event_subsystem
+ * @system: event_subsystem to create a filter for
+ * @filter_str: filter string
+ * @filterp: out param for created filter (must be %NULL on entry, may not
+ *	     be %NULL after failure)
+ *
+ * Identical to create_filter() except that it creates a subsystem filter
+ * and always remembers @filter_str.
+ */
+static int create_system_filter(struct event_subsystem *system,
+				char *filter_str, struct event_filter **filterp)
+{
+	struct filter_parse_state *ps = NULL;
+	int err;
+
+	err = create_filter_start(filter_str, true, &ps, filterp);
+	if (!err) {
+		err = replace_system_preds(system, ps, filter_str);
+		if (err)
+			append_filter_err(ps, *filterp);
+	}
+	create_filter_finish(ps);
+
+	return err;
+}
+
+int apply_event_filter(struct ftrace_event_call *call, char *filter_string)
+{
+	struct event_filter *filter = NULL;
 	int err = 0;
 
 	mutex_lock(&event_mutex);
@@ -1748,49 +1851,30 @@ int apply_event_filter(struct ftrace_eve
 		goto out_unlock;
 	}
 
-	err = -ENOMEM;
-	ps = kzalloc(sizeof(*ps), GFP_KERNEL);
-	if (!ps)
-		goto out_unlock;
-
-	filter = __alloc_filter();
-	if (!filter) {
-		kfree(ps);
-		goto out_unlock;
-	}
-
-	replace_filter_string(filter, filter_string);
-
-	parse_init(ps, filter_ops, filter_string);
-	err = filter_parse(ps);
-	if (err) {
-		append_filter_err(ps, filter);
-		goto out;
-	}
+	err = create_filter(call, filter_string, true, &filter);
 
-	err = replace_preds(call, filter, ps, filter_string, false);
-	if (err) {
-		filter_disable(call);
-		append_filter_err(ps, filter);
-	} else
-		call->flags |= TRACE_EVENT_FL_FILTERED;
-out:
 	/*
 	 * Always swap the call filter with the new filter
 	 * even if there was an error. If there was an error
 	 * in the filter, we disable the filter and show the error
 	 * string
 	 */
-	tmp = call->filter;
-	rcu_assign_pointer(call->filter, filter);
-	if (tmp) {
-		/* Make sure the call is done with the filter */
-		synchronize_sched();
-		__free_filter(tmp);
+	if (filter) {
+		struct event_filter *tmp = call->filter;
+
+		if (!err)
+			call->flags |= TRACE_EVENT_FL_FILTERED;
+		else
+			filter_disable(call);
+
+		rcu_assign_pointer(call->filter, filter);
+
+		if (tmp) {
+			/* Make sure the call is done with the filter */
+			synchronize_sched();
+			__free_filter(tmp);
+		}
 	}
-	filter_opstack_clear(ps);
-	postfix_clear(ps);
-	kfree(ps);
 out_unlock:
 	mutex_unlock(&event_mutex);
 
@@ -1800,8 +1884,7 @@ out_unlock:
 int apply_subsystem_event_filter(struct event_subsystem *system,
 				 char *filter_string)
 {
-	struct filter_parse_state *ps;
-	struct event_filter *filter;
+	struct event_filter *filter = NULL;
 	int err = 0;
 
 	mutex_lock(&event_mutex);
@@ -1824,38 +1907,15 @@ int apply_subsystem_event_filter(struct 
 		goto out_unlock;
 	}
 
-	err = -ENOMEM;
-	ps = kzalloc(sizeof(*ps), GFP_KERNEL);
-	if (!ps)
-		goto out_unlock;
-
-	filter = __alloc_filter();
-	if (!filter)
-		goto out;
-
-	replace_filter_string(filter, filter_string);
-	/*
-	 * No event actually uses the system filter
-	 * we can free it without synchronize_sched().
-	 */
-	__free_filter(system->filter);
-	system->filter = filter;
-
-	parse_init(ps, filter_ops, filter_string);
-	err = filter_parse(ps);
-	if (err) {
-		append_filter_err(ps, system->filter);
-		goto out;
+	err = create_system_filter(system, filter_string, &filter);
+	if (filter) {
+		/*
+		 * No event actually uses the system filter
+		 * we can free it without synchronize_sched().
+		 */
+		__free_filter(system->filter);
+		system->filter = filter;
 	}
-
-	err = replace_system_preds(system, ps, filter_string);
-	if (err)
-		append_filter_err(ps, system->filter);
-
-out:
-	filter_opstack_clear(ps);
-	postfix_clear(ps);
-	kfree(ps);
 out_unlock:
 	mutex_unlock(&event_mutex);
 
@@ -1876,8 +1936,7 @@ int ftrace_profile_set_filter(struct per
 			      char *filter_str)
 {
 	int err;
-	struct event_filter *filter;
-	struct filter_parse_state *ps;
+	struct event_filter *filter = NULL;
 	struct ftrace_event_call *call;
 
 	mutex_lock(&event_mutex);
@@ -1892,33 +1951,10 @@ int ftrace_profile_set_filter(struct per
 	if (event->filter)
 		goto out_unlock;
 
-	filter = __alloc_filter();
-	if (!filter) {
-		err = PTR_ERR(filter);
-		goto out_unlock;
-	}
-
-	err = -ENOMEM;
-	ps = kzalloc(sizeof(*ps), GFP_KERNEL);
-	if (!ps)
-		goto free_filter;
-
-	parse_init(ps, filter_ops, filter_str);
-	err = filter_parse(ps);
-	if (err)
-		goto free_ps;
-
-	err = replace_preds(call, filter, ps, filter_str, false);
+	err = create_filter(call, filter_str, false, &filter);
 	if (!err)
 		event->filter = filter;
-
-free_ps:
-	filter_opstack_clear(ps);
-	postfix_clear(ps);
-	kfree(ps);
-
-free_filter:
-	if (err)
+	else
 		__free_filter(filter);
 
 out_unlock:
@@ -1937,43 +1973,6 @@ out_unlock:
 #define CREATE_TRACE_POINTS
 #include "trace_events_filter_test.h"
 
-static int test_get_filter(char *filter_str, struct ftrace_event_call *call,
-			   struct event_filter **pfilter)
-{
-	struct event_filter *filter;
-	struct filter_parse_state *ps;
-	int err = -ENOMEM;
-
-	filter = __alloc_filter();
-	if (!filter)
-		goto out;
-
-	ps = kzalloc(sizeof(*ps), GFP_KERNEL);
-	if (!ps)
-		goto free_filter;
-
-	parse_init(ps, filter_ops, filter_str);
-	err = filter_parse(ps);
-	if (err)
-		goto free_ps;
-
-	err = replace_preds(call, filter, ps, filter_str, false);
-	if (!err)
-		*pfilter = filter;
-
- free_ps:
-	filter_opstack_clear(ps);
-	postfix_clear(ps);
-	kfree(ps);
-
- free_filter:
-	if (err)
-		__free_filter(filter);
-
- out:
-	return err;
-}
-
 #define DATA_REC(m, va, vb, vc, vd, ve, vf, vg, vh, nvisit) \
 { \
 	.filter = FILTER, \
@@ -2092,12 +2091,13 @@ static __init int ftrace_test_event_filt
 		struct test_filter_data_t *d = &test_filter_data[i];
 		int err;
 
-		err = test_get_filter(d->filter, &event_ftrace_test_filter,
-				      &filter);
+		err = create_filter(&event_ftrace_test_filter, d->filter,
+				    false, &filter);
 		if (err) {
 			printk(KERN_INFO
 			       "Failed to get filter for '%s', err %d\n",
 			       d->filter, err);
+			__free_filter(filter);
 			break;
 		}
 

Re: [PATCH 1/2] trace_events_filter: use rcu_assign_pointer() when setting ftrace_event_call->filter

[Steven Rostedt]

[ Added Paul to Cc ]

On Tue, 2011-11-22 at 17:46 -0800, Tejun Heo wrote:
> ftrace_event_call->filter is sched RCU protected but didn't use
> rcu_assign_pointer().  Fix it.

Is it really needed? Maybe just for documentation but I'm not sure this
use is required because all use cases have synchronize_sched() used,
which is a big hammer compared to the rcu_assign_pointer().

> 
> TODO: Add proper __rcu annotation to call->filter and all its users.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
>  kernel/trace/trace_events_filter.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> Index: work/kernel/trace/trace_events_filter.c
> ===================================================================
> --- work.orig/kernel/trace/trace_events_filter.c
> +++ work/kernel/trace/trace_events_filter.c
> @@ -1686,7 +1686,7 @@ static int replace_system_preds(struct e
>  		 * replace the filter for the call.
>  		 */
>  		filter = call->filter;
> -		call->filter = filter_item->filter;
> +		rcu_assign_pointer(call->filter, filter_item->filter);

We update filter here, and then call synchronize_sched() before we free
the filter_item->filter.

>  		filter_item->filter = filter;
>  
>  		fail = false;
> @@ -1741,7 +1741,7 @@ int apply_event_filter(struct ftrace_eve
>  		filter = call->filter;
>  		if (!filter)
>  			goto out_unlock;
> -		call->filter = NULL;
> +		rcu_assign_pointer(call->filter, NULL);
>  		/* Make sure the filter is not being used */

Again you can see that synchronize_sched() is called here.

>  		synchronize_sched();
>  		__free_filter(filter);
> @@ -1782,7 +1782,7 @@ out:
>  	 * string
>  	 */
>  	tmp = call->filter;
> -	call->filter = filter;
> +	rcu_assign_pointer(call->filter, filter);

We only call synchronize_sched if call->filter wasn't NULL, because we
are going to free tmp. We need to make sure all users are done with tmp
before we free it.

>  	if (tmp) {
>  		/* Make sure the call is done with the filter */
>  		synchronize_sched();

Thus my question is, do we really need to add the rcu_assign_pointer().
I have no problem if we only do so to document that this is an rcu sched
protected variable. But it should be commented as such.

-- Steve

Re: [PATCH 2/2] trace_event_filter: factorize filter creation

[Steven Rostedt]

On Tue, 2011-11-22 at 18:00 -0800, Tejun Heo wrote:
> There are four places where new filter for a given filter string is
> created, which involves several different steps.  This patch factors
> those steps into create_[system_]filter() functions which in turn make
> use of create_filter_{start|finish}() for common parts.
> 
> The only functional change is that if replace_filter_string() is
> requested and fails, creation fails without any side effect instead of
> being ignored.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
> These two are on top of the current linus/master bbbc4791cd "Merge
> branch 'staging-linus' of git://git.kernel.org/.../gregkh/staging" as
> tip seems to be lagging behind for the moment.
> 
> Thank you.
> 
>  kernel/trace/trace_events_filter.c |  274 ++++++++++++++++++-------------------
>  1 file changed, 137 insertions(+), 137 deletions(-)

For such a change, I would have liked to see more deletions than
insertions. Although, if it removes duplicate code, I'm not against such
a change. I'll look at this more when I get some more time. But just in
case I forget about it (because I wont look at it before my turkey
dinner tomorrow, and will most likely forget about it come next Monday),
please feel free to ping me again next week.

-- Steve

Re: [PATCH 2/2] trace_event_filter: factorize filter creation

[Tejun Heo]

Hello,

On Wed, Nov 23, 2011 at 10:19:21AM -0500, Steven Rostedt wrote:
> For such a change, I would have liked to see more deletions than
> insertions. Although, if it removes duplicate code, I'm not against such
> a change.

Yeah, certainly.  If you count out the function comments, it reduces
~30 lines of code.  I was trying to add another user and with one more
user, the stats become more attractive.

> I'll look at this more when I get some more time. But just in
> case I forget about it (because I wont look at it before my turkey
> dinner tomorrow, and will most likely forget about it come next Monday),
> please feel free to ping me again next week.

Will do so.

Thank you.

-- 
tejun

Re: [PATCH 1/2] trace_events_filter: use rcu_assign_pointer() when setting ftrace_event_call->filter

[Tejun Heo]

On Wed, Nov 23, 2011 at 10:16:47AM -0500, Steven Rostedt wrote:
> [ Added Paul to Cc ]
> 
> On Tue, 2011-11-22 at 17:46 -0800, Tejun Heo wrote:
> > ftrace_event_call->filter is sched RCU protected but didn't use
> > rcu_assign_pointer().  Fix it.
> 
> Is it really needed? Maybe just for documentation but I'm not sure this
> use is required because all use cases have synchronize_sched() used,
> which is a big hammer compared to the rcu_assign_pointer().

Oh yeah, I think we do.  synchronize_sched() is to drain users of the
old pointer.  Whether synchronize_sched() or call_rcu() is used is
irrelevant to the synchronization of new pointer.

rcu_assign_pointer() is to synchronize against rcu_dereference()
dereferencing the new one.  More specifically, we need it for the
writer memory barrier, so that it matches the data dependency read
barrier in rcu_dereference() when it access the new pointer;
otherwise, it may fetch the old values from before the new area is
initialized on archs where data dependency barrier isn't noop.

> We update filter here, and then call synchronize_sched() before we free
> the filter_item->filter.
...
> Again you can see that synchronize_sched() is called here.

So, synchronized_sched() being called after isn't relevant.  We want
smp_wmb() between data structure initialization and assignment of the
new pointer.

Thank you.

-- 
tejun

Re: [PATCH 1/2] trace_events_filter: use rcu_assign_pointer() when setting ftrace_event_call->filter

[Tejun Heo]

On Wed, Nov 23, 2011 at 08:06:53AM -0800, Tejun Heo wrote:
> otherwise, it may fetch the old values from before the new area is
> initialized on archs where data dependency barrier isn't noop.

I think this can be a problem on archs where dd barrier is noop but
wmb is not.  dd barrier is implied on every read access but nothing
guarantees the updating CPU sends out write for the pointer assignment
before initialization of the new area.

Thanks.

-- 
tejun

Re: [PATCH 1/2] trace_events_filter: use rcu_assign_pointer() when setting ftrace_event_call->filter

[Eric Dumazet]

Le mardi 22 novembre 2011 à 17:46 -0800, Tejun Heo a écrit :
> ftrace_event_call->filter is sched RCU protected but didn't use
> rcu_assign_pointer().  Fix it.
> 
> TODO: Add proper __rcu annotation to call->filter and all its users.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
>  kernel/trace/trace_events_filter.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> Index: work/kernel/trace/trace_events_filter.c
> ===================================================================
> --- work.orig/kernel/trace/trace_events_filter.c
> +++ work/kernel/trace/trace_events_filter.c
> @@ -1686,7 +1686,7 @@ static int replace_system_preds(struct e
>  		 * replace the filter for the call.
>  		 */
>  		filter = call->filter;
> -		call->filter = filter_item->filter;
> +		rcu_assign_pointer(call->filter, filter_item->filter);
>  		filter_item->filter = filter;
>  
>  		fail = false;
> @@ -1741,7 +1741,7 @@ int apply_event_filter(struct ftrace_eve
>  		filter = call->filter;
>  		if (!filter)
>  			goto out_unlock;
> -		call->filter = NULL;
> +		rcu_assign_pointer(call->filter, NULL);

Its a minor point, but setting a pointer to NULL certainly doesnt need a
smp_wmb() barrier, so you can use RCU_INIT_POINTER(call->filter, NULL);

Re: [PATCH 1/2] trace_events_filter: use rcu_assign_pointer() when setting ftrace_event_call->filter

[Tejun Heo]

Hello,

On Wed, Nov 23, 2011 at 05:40:17PM +0100, Eric Dumazet wrote:
> > -		call->filter = NULL;
> > +		rcu_assign_pointer(call->filter, NULL);
> 
> Its a minor point, but setting a pointer to NULL certainly doesnt need a
> smp_wmb() barrier, so you can use RCU_INIT_POINTER(call->filter, NULL);

Yeap, we don't need it for NULL.  Did that anyway for consistency /
documentation.  Didn't know we had a macro for that.  Neat.  Updating
the patch....

Thanks.

-- 
tejun

[PATCH UPDATED 1/2] trace_events_filter: use rcu_assign_pointer() when setting ftrace_event_call->filter

[Tejun Heo]

ftrace_event_call->filter is sched RCU protected but didn't use
rcu_assign_pointer().  Use it.

TODO: Add proper __rcu annotation to call->filter and all its users.

-v2: Use RCU_INIT_POINTER() for %NULL clearing as suggested by Eric.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
---
 kernel/trace/trace_events_filter.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Index: work/kernel/trace/trace_events_filter.c
===================================================================
--- work.orig/kernel/trace/trace_events_filter.c
+++ work/kernel/trace/trace_events_filter.c
@@ -1686,7 +1686,7 @@ static int replace_system_preds(struct e
 		 * replace the filter for the call.
 		 */
 		filter = call->filter;
-		call->filter = filter_item->filter;
+		rcu_assign_pointer(call->filter, filter_item->filter);
 		filter_item->filter = filter;
 
 		fail = false;
@@ -1741,7 +1741,7 @@ int apply_event_filter(struct ftrace_eve
 		filter = call->filter;
 		if (!filter)
 			goto out_unlock;
-		call->filter = NULL;
+		RCU_INIT_POINTER(call->filter, NULL);
 		/* Make sure the filter is not being used */
 		synchronize_sched();
 		__free_filter(filter);
@@ -1782,7 +1782,7 @@ out:
 	 * string
 	 */
 	tmp = call->filter;
-	call->filter = filter;
+	rcu_assign_pointer(call->filter, filter);
 	if (tmp) {
 		/* Make sure the call is done with the filter */
 		synchronize_sched();

Re: [PATCH 1/2] trace_events_filter: use rcu_assign_pointer() when setting ftrace_event_call->filter

[Steven Rostedt]

On Wed, 2011-11-23 at 08:06 -0800, Tejun Heo wrote:

> So, synchronized_sched() being called after isn't relevant.  We want
> smp_wmb() between data structure initialization and assignment of the
> new pointer.

Ah, you're saying that we need to guarantee that the allocated filter is
seen before we update the call->filter to point to it. OK, fair enough,
this does look like a bug fix.

Is it big enough to be considered for stable?

-- Steve

Re: [PATCH 1/2] trace_events_filter: use rcu_assign_pointer() when setting ftrace_event_call->filter

[Tejun Heo]

Hello, Steven.

On Wed, Nov 23, 2011 at 12:06:40PM -0500, Steven Rostedt wrote:
> > So, synchronized_sched() being called after isn't relevant.  We want
> > smp_wmb() between data structure initialization and assignment of the
> > new pointer.
> 
> Ah, you're saying that we need to guarantee that the allocated filter is
> seen before we update the call->filter to point to it. OK, fair enough,
> this does look like a bug fix.
> 
> Is it big enough to be considered for stable?

It's unlikely to happen for x86 or arm and that probably is the
biggest reason there hasn't already been a bug report.  It isn't very
urgent but at the same time the change is almost trivial, so marking
it for stable is a pretty safe bet.  I lean toward marking it for
stable but the leaning isn't too strong. :)

Thanks.

-- 
tejun

Re: [PATCH 1/2] trace_events_filter: use rcu_assign_pointer() when setting ftrace_event_call->filter

[Paul E. McKenney]

On Wed, Nov 23, 2011 at 09:33:13AM -0800, Tejun Heo wrote:
> Hello, Steven.
> 
> On Wed, Nov 23, 2011 at 12:06:40PM -0500, Steven Rostedt wrote:
> > > So, synchronized_sched() being called after isn't relevant.  We want
> > > smp_wmb() between data structure initialization and assignment of the
> > > new pointer.
> > 
> > Ah, you're saying that we need to guarantee that the allocated filter is
> > seen before we update the call->filter to point to it. OK, fair enough,
> > this does look like a bug fix.
> > 
> > Is it big enough to be considered for stable?
> 
> It's unlikely to happen for x86 or arm and that probably is the
> biggest reason there hasn't already been a bug report.  It isn't very
> urgent but at the same time the change is almost trivial, so marking
> it for stable is a pretty safe bet.  I lean toward marking it for
> stable but the leaning isn't too strong. :)

I believe that marking it for stable makes sense, especially since
reasonably common compiler optimizations can rearrange the update side.
This can be especially painful for PREEMPT=y kernels, which could in
some cases preempt the task after the rearrangement, leaving readers
vulnerable to seeing uninitialized data for an extended time period.
Please note that this can happen even on arbitrarily strongly ordered
CPUs -- it is the compiler that is toying with us here.

							Thanx, Paul

Re: [PATCH 2/2] trace_event_filter: factorize filter creation

[Tejun Heo]

[OFFLIST]

Ping.

-- 
tejun

Re: [PATCH 2/2] trace_event_filter: factorize filter creation

[Steven Rostedt]

On Mon, 2011-11-28 at 12:49 -0800, Tejun Heo wrote:
> [OFFLIST]
> 
> Ping.
> 

Thanks! I actually remembered, just a bit slow today :)

-- Steve

[tip:perf/urgent] trace_events_filter: Use rcu_assign_pointer() when setting ftrace_event_call->filter

[tip-bot for Tejun Heo]

Commit-ID:  d3d9acf646679c1981032b0985b386d12fccc60c
Gitweb:     http://git.kernel.org/tip/d3d9acf646679c1981032b0985b386d12fccc60c
Author:     Tejun Heo <tj@kernel.org>
AuthorDate: Wed, 23 Nov 2011 08:49:49 -0800
Committer:  Steven Rostedt <rostedt@goodmis.org>
CommitDate: Thu, 1 Dec 2011 22:16:47 -0500

trace_events_filter: Use rcu_assign_pointer() when setting ftrace_event_call->filter

ftrace_event_call->filter is sched RCU protected but didn't use
rcu_assign_pointer().  Use it.

TODO: Add proper __rcu annotation to call->filter and all its users.

-v2: Use RCU_INIT_POINTER() for %NULL clearing as suggested by Eric.

Link: http://lkml.kernel.org/r/20111123164949.GA29639@google.com

Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: stable@kernel.org # (2.6.39+)
Signed-off-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace_events_filter.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 816d3d0..d6e7926 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -1686,7 +1686,7 @@ static int replace_system_preds(struct event_subsystem *system,
 		 * replace the filter for the call.
 		 */
 		filter = call->filter;
-		call->filter = filter_item->filter;
+		rcu_assign_pointer(call->filter, filter_item->filter);
 		filter_item->filter = filter;
 
 		fail = false;
@@ -1741,7 +1741,7 @@ int apply_event_filter(struct ftrace_event_call *call, char *filter_string)
 		filter = call->filter;
 		if (!filter)
 			goto out_unlock;
-		call->filter = NULL;
+		RCU_INIT_POINTER(call->filter, NULL);
 		/* Make sure the filter is not being used */
 		synchronize_sched();
 		__free_filter(filter);
@@ -1782,7 +1782,7 @@ out:
 	 * string
 	 */
 	tmp = call->filter;
-	call->filter = filter;
+	rcu_assign_pointer(call->filter, filter);
 	if (tmp) {
 		/* Make sure the call is done with the filter */
 		synchronize_sched();

Re: [PATCH 2/2] trace_event_filter: factorize filter creation

[Tejun Heo]

On Mon, Nov 28, 2011 at 05:15:12PM -0500, Steven Rostedt wrote:
> On Mon, 2011-11-28 at 12:49 -0800, Tejun Heo wrote:
> > [OFFLIST]
> > 
> > Ping.
> > 
> 
> Thanks! I actually remembered, just a bit slow today :)

Did you get around to it?

Thanks.

-- 
tejun

Re: [PATCH 2/2] trace_event_filter: factorize filter creation

[Steven Rostedt]

On Thu, 2011-12-08 at 15:32 -0800, Tejun Heo wrote:
> On Mon, Nov 28, 2011 at 05:15:12PM -0500, Steven Rostedt wrote:
> > On Mon, 2011-11-28 at 12:49 -0800, Tejun Heo wrote:
> > > [OFFLIST]
> > > 
> > > Ping.
> > > 
> > 
> > Thanks! I actually remembered, just a bit slow today :)
> 
> Did you get around to it?

Ah, I did patch 1 (as it was urgent) but forgot about this one. Let me
take a look at this too.

-- Steve

Re: [PATCH 2/2] trace_event_filter: factorize filter creation

[Steven Rostedt]

On Thu, 2011-12-08 at 15:32 -0800, Tejun Heo wrote:
> On Mon, Nov 28, 2011 at 05:15:12PM -0500, Steven Rostedt wrote:
> > On Mon, 2011-11-28 at 12:49 -0800, Tejun Heo wrote:
> > > [OFFLIST]
> > > 
> > > Ping.
> > > 
> > 
> > Thanks! I actually remembered, just a bit slow today :)
> 
> Did you get around to it?

This has some major conflicts with what is currently in tip/perf/core.
Could you rebase against it?

Note, 49aa29513ec995f201664cf6eee36e5326ed38bf does change how the
filter is displayed for system filters.

-- Steve

Re: [PATCH 2/2] trace_event_filter: factorize filter creation

[Tejun Heo]

On Thu, Dec 08, 2011 at 07:46:34PM -0500, Steven Rostedt wrote:
> This has some major conflicts with what is currently in tip/perf/core.
> Could you rebase against it?

I checked tip before sending the patch but it seemed to be lagging
behind mainline.  Just discovered my tree was tracking ingo/tip.git
instead of tip/tip.git.  Gah...

Will send updated patch soon.

Thanks.

-- 
tejun

[PATCH 2/2 UPDATED] trace_event_filter: factorize filter creation

[Tejun Heo]

There are four places where new filter for a given filter string is
created, which involves several different steps.  This patch factors
those steps into create_[system_]filter() functions which in turn make
use of create_filter_{start|finish}() for common parts.

The only functional change is that if replace_filter_string() is
requested and fails, creation fails without any side effect instead of
being ignored.

Note that system filter is now installed after the processing is
complete which makes freeing before and then restoring filter string
on error unncessary.

-v2: Rebased to resolve conflict with 49aa29513e and updated both
     create_filter() functions to always set *filterp instead of
     requiring the caller to clear it to %NULL on entry.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 kernel/trace/trace_events_filter.c |  283 ++++++++++++++++++-------------------
 1 file changed, 142 insertions(+), 141 deletions(-)

Index: work/kernel/trace/trace_events_filter.c
===================================================================
--- work.orig/kernel/trace/trace_events_filter.c
+++ work/kernel/trace/trace_events_filter.c
@@ -1738,11 +1738,121 @@ static int replace_system_preds(struct e
 	return -ENOMEM;
 }
 
+static int create_filter_start(char *filter_str, bool set_str,
+			       struct filter_parse_state **psp,
+			       struct event_filter **filterp)
+{
+	struct event_filter *filter;
+	struct filter_parse_state *ps = NULL;
+	int err = 0;
+
+	WARN_ON_ONCE(*psp || *filterp);
+
+	/* allocate everything, and if any fails, free all and fail */
+	filter = __alloc_filter();
+	if (filter && set_str)
+		err = replace_filter_string(filter, filter_str);
+
+	ps = kzalloc(sizeof(*ps), GFP_KERNEL);
+
+	if (!filter || !ps || err) {
+		kfree(ps);
+		__free_filter(filter);
+		return -ENOMEM;
+	}
+
+	/* we're committed to creating a new filter */
+	*filterp = filter;
+	*psp = ps;
+
+	parse_init(ps, filter_ops, filter_str);
+	err = filter_parse(ps);
+	if (err && set_str)
+		append_filter_err(ps, filter);
+	return err;
+}
+
+static void create_filter_finish(struct filter_parse_state *ps)
+{
+	if (ps) {
+		filter_opstack_clear(ps);
+		postfix_clear(ps);
+		kfree(ps);
+	}
+}
+
+/**
+ * create_filter - create a filter for a ftrace_event_call
+ * @call: ftrace_event_call to create a filter for
+ * @filter_str: filter string
+ * @set_str: remember @filter_str and enable detailed error in filter
+ * @filterp: out param for created filter (always updated on return)
+ *
+ * Creates a filter for @call with @filter_str.  If @set_str is %true,
+ * @filter_str is copied and recorded in the new filter.
+ *
+ * On success, returns 0 and *@filterp points to the new filter.  On
+ * failure, returns -errno and *@filterp may point to %NULL or to a new
+ * filter.  In the latter case, the returned filter contains error
+ * information if @set_str is %true and the caller is responsible for
+ * freeing it.
+ */
+static int create_filter(struct ftrace_event_call *call,
+			 char *filter_str, bool set_str,
+			 struct event_filter **filterp)
+{
+	struct event_filter *filter = NULL;
+	struct filter_parse_state *ps = NULL;
+	int err;
+
+	err = create_filter_start(filter_str, set_str, &ps, &filter);
+	if (!err) {
+		err = replace_preds(call, filter, ps, filter_str, false);
+		if (err && set_str)
+			append_filter_err(ps, filter);
+	}
+	create_filter_finish(ps);
+
+	*filterp = filter;
+	return err;
+}
+
+/**
+ * create_system_filter - create a filter for an event_subsystem
+ * @system: event_subsystem to create a filter for
+ * @filter_str: filter string
+ * @filterp: out param for created filter (always updated on return)
+ *
+ * Identical to create_filter() except that it creates a subsystem filter
+ * and always remembers @filter_str.
+ */
+static int create_system_filter(struct event_subsystem *system,
+				char *filter_str, struct event_filter **filterp)
+{
+	struct event_filter *filter = NULL;
+	struct filter_parse_state *ps = NULL;
+	int err;
+
+	err = create_filter_start(filter_str, true, &ps, &filter);
+	if (!err) {
+		err = replace_system_preds(system, ps, filter_str);
+		if (!err) {
+			/* System filters just show a default message */
+			kfree(filter->filter_string);
+			filter->filter_string = NULL;
+		} else {
+			append_filter_err(ps, filter);
+		}
+	}
+	create_filter_finish(ps);
+
+	*filterp = filter;
+	return err;
+}
+
 int apply_event_filter(struct ftrace_event_call *call, char *filter_string)
 {
-	struct filter_parse_state *ps;
 	struct event_filter *filter;
-	struct event_filter *tmp;
 	int err = 0;
 
 	mutex_lock(&event_mutex);
@@ -1759,49 +1869,30 @@ int apply_event_filter(struct ftrace_eve
 		goto out_unlock;
 	}
 
-	err = -ENOMEM;
-	ps = kzalloc(sizeof(*ps), GFP_KERNEL);
-	if (!ps)
-		goto out_unlock;
+	err = create_filter(call, filter_string, true, &filter);
 
-	filter = __alloc_filter();
-	if (!filter) {
-		kfree(ps);
-		goto out_unlock;
-	}
-
-	replace_filter_string(filter, filter_string);
-
-	parse_init(ps, filter_ops, filter_string);
-	err = filter_parse(ps);
-	if (err) {
-		append_filter_err(ps, filter);
-		goto out;
-	}
-
-	err = replace_preds(call, filter, ps, filter_string, false);
-	if (err) {
-		filter_disable(call);
-		append_filter_err(ps, filter);
-	} else
-		call->flags |= TRACE_EVENT_FL_FILTERED;
-out:
 	/*
 	 * Always swap the call filter with the new filter
 	 * even if there was an error. If there was an error
 	 * in the filter, we disable the filter and show the error
 	 * string
 	 */
-	tmp = call->filter;
-	rcu_assign_pointer(call->filter, filter);
-	if (tmp) {
-		/* Make sure the call is done with the filter */
-		synchronize_sched();
-		__free_filter(tmp);
+	if (filter) {
+		struct event_filter *tmp = call->filter;
+
+		if (!err)
+			call->flags |= TRACE_EVENT_FL_FILTERED;
+		else
+			filter_disable(call);
+
+		rcu_assign_pointer(call->filter, filter);
+
+		if (tmp) {
+			/* Make sure the call is done with the filter */
+			synchronize_sched();
+			__free_filter(tmp);
+		}
 	}
-	filter_opstack_clear(ps);
-	postfix_clear(ps);
-	kfree(ps);
 out_unlock:
 	mutex_unlock(&event_mutex);
 
@@ -1811,7 +1902,6 @@ out_unlock:
 int apply_subsystem_event_filter(struct event_subsystem *system,
 				 char *filter_string)
 {
-	struct filter_parse_state *ps;
 	struct event_filter *filter;
 	int err = 0;
 
@@ -1835,48 +1925,19 @@ int apply_subsystem_event_filter(struct 
 		goto out_unlock;
 	}
 
-	err = -ENOMEM;
-	ps = kzalloc(sizeof(*ps), GFP_KERNEL);
-	if (!ps)
-		goto out_unlock;
-
-	filter = __alloc_filter();
-	if (!filter)
-		goto out;
-
-	/* System filters just show a default message */
-	kfree(filter->filter_string);
-	filter->filter_string = NULL;
-
-	/*
-	 * No event actually uses the system filter
-	 * we can free it without synchronize_sched().
-	 */
-	__free_filter(system->filter);
-	system->filter = filter;
-
-	parse_init(ps, filter_ops, filter_string);
-	err = filter_parse(ps);
-	if (err)
-		goto err_filter;
-
-	err = replace_system_preds(system, ps, filter_string);
-	if (err)
-		goto err_filter;
-
-out:
-	filter_opstack_clear(ps);
-	postfix_clear(ps);
-	kfree(ps);
+	err = create_system_filter(system, filter_string, &filter);
+	if (filter) {
+		/*
+		 * No event actually uses the system filter
+		 * we can free it without synchronize_sched().
+		 */
+		__free_filter(system->filter);
+		system->filter = filter;
+	}
 out_unlock:
 	mutex_unlock(&event_mutex);
 
 	return err;
-
-err_filter:
-	replace_filter_string(filter, filter_string);
-	append_filter_err(ps, system->filter);
-	goto out;
 }
 
 #ifdef CONFIG_PERF_EVENTS
@@ -1894,7 +1955,6 @@ int ftrace_profile_set_filter(struct per
 {
 	int err;
 	struct event_filter *filter;
-	struct filter_parse_state *ps;
 	struct ftrace_event_call *call;
 
 	mutex_lock(&event_mutex);
@@ -1909,33 +1969,10 @@ int ftrace_profile_set_filter(struct per
 	if (event->filter)
 		goto out_unlock;
 
-	filter = __alloc_filter();
-	if (!filter) {
-		err = PTR_ERR(filter);
-		goto out_unlock;
-	}
-
-	err = -ENOMEM;
-	ps = kzalloc(sizeof(*ps), GFP_KERNEL);
-	if (!ps)
-		goto free_filter;
-
-	parse_init(ps, filter_ops, filter_str);
-	err = filter_parse(ps);
-	if (err)
-		goto free_ps;
-
-	err = replace_preds(call, filter, ps, filter_str, false);
+	err = create_filter(call, filter_str, false, &filter);
 	if (!err)
 		event->filter = filter;
-
-free_ps:
-	filter_opstack_clear(ps);
-	postfix_clear(ps);
-	kfree(ps);
-
-free_filter:
-	if (err)
+	else
 		__free_filter(filter);
 
 out_unlock:
@@ -1954,43 +1991,6 @@ out_unlock:
 #define CREATE_TRACE_POINTS
 #include "trace_events_filter_test.h"
 
-static int test_get_filter(char *filter_str, struct ftrace_event_call *call,
-			   struct event_filter **pfilter)
-{
-	struct event_filter *filter;
-	struct filter_parse_state *ps;
-	int err = -ENOMEM;
-
-	filter = __alloc_filter();
-	if (!filter)
-		goto out;
-
-	ps = kzalloc(sizeof(*ps), GFP_KERNEL);
-	if (!ps)
-		goto free_filter;
-
-	parse_init(ps, filter_ops, filter_str);
-	err = filter_parse(ps);
-	if (err)
-		goto free_ps;
-
-	err = replace_preds(call, filter, ps, filter_str, false);
-	if (!err)
-		*pfilter = filter;
-
- free_ps:
-	filter_opstack_clear(ps);
-	postfix_clear(ps);
-	kfree(ps);
-
- free_filter:
-	if (err)
-		__free_filter(filter);
-
- out:
-	return err;
-}
-
 #define DATA_REC(m, va, vb, vc, vd, ve, vf, vg, vh, nvisit) \
 { \
 	.filter = FILTER, \
@@ -2109,12 +2109,13 @@ static __init int ftrace_test_event_filt
 		struct test_filter_data_t *d = &test_filter_data[i];
 		int err;
 
-		err = test_get_filter(d->filter, &event_ftrace_test_filter,
-				      &filter);
+		err = create_filter(&event_ftrace_test_filter, d->filter,
+				    false, &filter);
 		if (err) {
 			printk(KERN_INFO
 			       "Failed to get filter for '%s', err %d\n",
 			       d->filter, err);
+			__free_filter(filter);
 			break;
 		}