All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH -tip] tracing: trigger: Replace unneeded RCU-list traversals
@ 2019-12-20  2:31 Masami Hiramatsu
  2020-01-07  9:15 ` Masami Hiramatsu
  2020-01-07 16:37 ` Tom Zanussi
  0 siblings, 2 replies; 3+ messages in thread
From: Masami Hiramatsu @ 2019-12-20  2:31 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: linux-kernel, mhiramat, Tom Zanussi, Joel Fernandes, Ingo Molnar,
	Paul E . McKenney

With CONFIG_PROVE_RCU_LIST, I had many suspicious RCU warnings
when I ran ftracetest trigger testcases.

-----
  # dmesg -c > /dev/null
  # ./ftracetest test.d/trigger
  ...
  # dmesg | grep "RCU-list traversed" | cut -f 2 -d ] | cut -f 2 -d " "
  kernel/trace/trace_events_hist.c:6070
  kernel/trace/trace_events_hist.c:1760
  kernel/trace/trace_events_hist.c:5911
  kernel/trace/trace_events_trigger.c:504
  kernel/trace/trace_events_hist.c:1810
  kernel/trace/trace_events_hist.c:3158
  kernel/trace/trace_events_hist.c:3105
  kernel/trace/trace_events_hist.c:5518
  kernel/trace/trace_events_hist.c:5998
  kernel/trace/trace_events_hist.c:6019
  kernel/trace/trace_events_hist.c:6044
  kernel/trace/trace_events_trigger.c:1500
  kernel/trace/trace_events_trigger.c:1540
  kernel/trace/trace_events_trigger.c:539
  kernel/trace/trace_events_trigger.c:584
-----

I investigated those warnings and found that the RCU-list
traversals in event trigger and hist didn't need to use
RCU version because those were called only under event_mutex.

I also checked other RCU-list traversals related to event
trigger list, and found that most of them were called from
event_hist_trigger_func() or hist_unregister_trigger() or
register/unregister functions except for a few cases.

Replace these unneeded RCU-list traversals with normal list
traversal macro and lockdep_assert_held() to check the
event_mutex is held.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/trace/trace_events_hist.c    |   41 ++++++++++++++++++++++++++---------
 kernel/trace/trace_events_trigger.c |   20 +++++++++++++----
 2 files changed, 45 insertions(+), 16 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 23b0195ac977..844f8325077f 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -1753,11 +1753,13 @@ static struct hist_field *find_var(struct hist_trigger_data *hist_data,
 	struct event_trigger_data *test;
 	struct hist_field *hist_field;
 
+	lockdep_assert_held(&event_mutex);
+
 	hist_field = find_var_field(hist_data, var_name);
 	if (hist_field)
 		return hist_field;
 
-	list_for_each_entry_rcu(test, &file->triggers, list) {
+	list_for_each_entry(test, &file->triggers, list) {
 		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
 			test_data = test->private_data;
 			hist_field = find_var_field(test_data, var_name);
@@ -1807,7 +1809,9 @@ static struct hist_field *find_file_var(struct trace_event_file *file,
 	struct event_trigger_data *test;
 	struct hist_field *hist_field;
 
-	list_for_each_entry_rcu(test, &file->triggers, list) {
+	lockdep_assert_held(&event_mutex);
+
+	list_for_each_entry(test, &file->triggers, list) {
 		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
 			test_data = test->private_data;
 			hist_field = find_var_field(test_data, var_name);
@@ -3102,7 +3106,9 @@ static char *find_trigger_filter(struct hist_trigger_data *hist_data,
 {
 	struct event_trigger_data *test;
 
-	list_for_each_entry_rcu(test, &file->triggers, list) {
+	lockdep_assert_held(&event_mutex);
+
+	list_for_each_entry(test, &file->triggers, list) {
 		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
 			if (test->private_data == hist_data)
 				return test->filter_str;
@@ -3153,9 +3159,11 @@ find_compatible_hist(struct hist_trigger_data *target_hist_data,
 	struct event_trigger_data *test;
 	unsigned int n_keys;
 
+	lockdep_assert_held(&event_mutex);
+
 	n_keys = target_hist_data->n_fields - target_hist_data->n_vals;
 
-	list_for_each_entry_rcu(test, &file->triggers, list) {
+	list_for_each_entry(test, &file->triggers, list) {
 		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
 			hist_data = test->private_data;
 
@@ -5515,7 +5523,7 @@ static int hist_show(struct seq_file *m, void *v)
 		goto out_unlock;
 	}
 
-	list_for_each_entry_rcu(data, &event_file->triggers, list) {
+	list_for_each_entry(data, &event_file->triggers, list) {
 		if (data->cmd_ops->trigger_type == ETT_EVENT_HIST)
 			hist_trigger_show(m, data, n++);
 	}
@@ -5908,7 +5916,9 @@ static int hist_register_trigger(char *glob, struct event_trigger_ops *ops,
 	if (hist_data->attrs->name && !named_data)
 		goto new;
 
-	list_for_each_entry_rcu(test, &file->triggers, list) {
+	lockdep_assert_held(&event_mutex);
+
+	list_for_each_entry(test, &file->triggers, list) {
 		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
 			if (!hist_trigger_match(data, test, named_data, false))
 				continue;
@@ -5992,10 +6002,12 @@ static bool have_hist_trigger_match(struct event_trigger_data *data,
 	struct event_trigger_data *test, *named_data = NULL;
 	bool match = false;
 
+	lockdep_assert_held(&event_mutex);
+
 	if (hist_data->attrs->name)
 		named_data = find_named_trigger(hist_data->attrs->name);
 
-	list_for_each_entry_rcu(test, &file->triggers, list) {
+	list_for_each_entry(test, &file->triggers, list) {
 		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
 			if (hist_trigger_match(data, test, named_data, false)) {
 				match = true;
@@ -6013,10 +6025,12 @@ static bool hist_trigger_check_refs(struct event_trigger_data *data,
 	struct hist_trigger_data *hist_data = data->private_data;
 	struct event_trigger_data *test, *named_data = NULL;
 
+	lockdep_assert_held(&event_mutex);
+
 	if (hist_data->attrs->name)
 		named_data = find_named_trigger(hist_data->attrs->name);
 
-	list_for_each_entry_rcu(test, &file->triggers, list) {
+	list_for_each_entry(test, &file->triggers, list) {
 		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
 			if (!hist_trigger_match(data, test, named_data, false))
 				continue;
@@ -6038,10 +6052,12 @@ static void hist_unregister_trigger(char *glob, struct event_trigger_ops *ops,
 	struct event_trigger_data *test, *named_data = NULL;
 	bool unregistered = false;
 
+	lockdep_assert_held(&event_mutex);
+
 	if (hist_data->attrs->name)
 		named_data = find_named_trigger(hist_data->attrs->name);
 
-	list_for_each_entry_rcu(test, &file->triggers, list) {
+	list_for_each_entry(test, &file->triggers, list) {
 		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
 			if (!hist_trigger_match(data, test, named_data, false))
 				continue;
@@ -6067,7 +6083,9 @@ static bool hist_file_check_refs(struct trace_event_file *file)
 	struct hist_trigger_data *hist_data;
 	struct event_trigger_data *test;
 
-	list_for_each_entry_rcu(test, &file->triggers, list) {
+	lockdep_assert_held(&event_mutex);
+
+	list_for_each_entry(test, &file->triggers, list) {
 		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
 			hist_data = test->private_data;
 			if (check_var_refs(hist_data))
@@ -6310,7 +6328,8 @@ hist_enable_trigger(struct event_trigger_data *data, void *rec,
 	struct enable_trigger_data *enable_data = data->private_data;
 	struct event_trigger_data *test;
 
-	list_for_each_entry_rcu(test, &enable_data->file->triggers, list) {
+	list_for_each_entry_rcu(test, &enable_data->file->triggers, list,
+				lockdep_is_held(&event_mutex)) {
 		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
 			if (enable_data->enable)
 				test->paused = false;
diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c
index 2cd53ca21b51..40106fff06a4 100644
--- a/kernel/trace/trace_events_trigger.c
+++ b/kernel/trace/trace_events_trigger.c
@@ -501,7 +501,9 @@ void update_cond_flag(struct trace_event_file *file)
 	struct event_trigger_data *data;
 	bool set_cond = false;
 
-	list_for_each_entry_rcu(data, &file->triggers, list) {
+	lockdep_assert_held(&event_mutex);
+
+	list_for_each_entry(data, &file->triggers, list) {
 		if (data->filter || event_command_post_trigger(data->cmd_ops) ||
 		    event_command_needs_rec(data->cmd_ops)) {
 			set_cond = true;
@@ -536,7 +538,9 @@ static int register_trigger(char *glob, struct event_trigger_ops *ops,
 	struct event_trigger_data *test;
 	int ret = 0;
 
-	list_for_each_entry_rcu(test, &file->triggers, list) {
+	lockdep_assert_held(&event_mutex);
+
+	list_for_each_entry(test, &file->triggers, list) {
 		if (test->cmd_ops->trigger_type == data->cmd_ops->trigger_type) {
 			ret = -EEXIST;
 			goto out;
@@ -581,7 +585,9 @@ static void unregister_trigger(char *glob, struct event_trigger_ops *ops,
 	struct event_trigger_data *data;
 	bool unregistered = false;
 
-	list_for_each_entry_rcu(data, &file->triggers, list) {
+	lockdep_assert_held(&event_mutex);
+
+	list_for_each_entry(data, &file->triggers, list) {
 		if (data->cmd_ops->trigger_type == test->cmd_ops->trigger_type) {
 			unregistered = true;
 			list_del_rcu(&data->list);
@@ -1497,7 +1503,9 @@ int event_enable_register_trigger(char *glob,
 	struct event_trigger_data *test;
 	int ret = 0;
 
-	list_for_each_entry_rcu(test, &file->triggers, list) {
+	lockdep_assert_held(&event_mutex);
+
+	list_for_each_entry(test, &file->triggers, list) {
 		test_enable_data = test->private_data;
 		if (test_enable_data &&
 		    (test->cmd_ops->trigger_type ==
@@ -1537,7 +1545,9 @@ void event_enable_unregister_trigger(char *glob,
 	struct event_trigger_data *data;
 	bool unregistered = false;
 
-	list_for_each_entry_rcu(data, &file->triggers, list) {
+	lockdep_assert_held(&event_mutex);
+
+	list_for_each_entry(data, &file->triggers, list) {
 		enable_data = data->private_data;
 		if (enable_data &&
 		    (data->cmd_ops->trigger_type ==


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH -tip] tracing: trigger: Replace unneeded RCU-list traversals
  2019-12-20  2:31 [PATCH -tip] tracing: trigger: Replace unneeded RCU-list traversals Masami Hiramatsu
@ 2020-01-07  9:15 ` Masami Hiramatsu
  2020-01-07 16:37 ` Tom Zanussi
  1 sibling, 0 replies; 3+ messages in thread
From: Masami Hiramatsu @ 2020-01-07  9:15 UTC (permalink / raw)
  To: Tom Zanussi
  Cc: Steven Rostedt, linux-kernel, Masami Hiramatsu, Joel Fernandes,
	Ingo Molnar, Paul E . McKenney

Hi Tom, 

Could you review this fix? 

On Fri, 20 Dec 2019 11:31:43 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> With CONFIG_PROVE_RCU_LIST, I had many suspicious RCU warnings
> when I ran ftracetest trigger testcases.
> 
> -----
>   # dmesg -c > /dev/null
>   # ./ftracetest test.d/trigger
>   ...
>   # dmesg | grep "RCU-list traversed" | cut -f 2 -d ] | cut -f 2 -d " "
>   kernel/trace/trace_events_hist.c:6070
>   kernel/trace/trace_events_hist.c:1760
>   kernel/trace/trace_events_hist.c:5911
>   kernel/trace/trace_events_trigger.c:504
>   kernel/trace/trace_events_hist.c:1810
>   kernel/trace/trace_events_hist.c:3158
>   kernel/trace/trace_events_hist.c:3105
>   kernel/trace/trace_events_hist.c:5518
>   kernel/trace/trace_events_hist.c:5998
>   kernel/trace/trace_events_hist.c:6019
>   kernel/trace/trace_events_hist.c:6044
>   kernel/trace/trace_events_trigger.c:1500
>   kernel/trace/trace_events_trigger.c:1540
>   kernel/trace/trace_events_trigger.c:539
>   kernel/trace/trace_events_trigger.c:584
> -----
> 
> I investigated those warnings and found that the RCU-list
> traversals in event trigger and hist didn't need to use
> RCU version because those were called only under event_mutex.
> 
> I also checked other RCU-list traversals related to event
> trigger list, and found that most of them were called from
> event_hist_trigger_func() or hist_unregister_trigger() or
> register/unregister functions except for a few cases.
> 
> Replace these unneeded RCU-list traversals with normal list
> traversal macro and lockdep_assert_held() to check the
> event_mutex is held.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  kernel/trace/trace_events_hist.c    |   41 ++++++++++++++++++++++++++---------
>  kernel/trace/trace_events_trigger.c |   20 +++++++++++++----
>  2 files changed, 45 insertions(+), 16 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 23b0195ac977..844f8325077f 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -1753,11 +1753,13 @@ static struct hist_field *find_var(struct hist_trigger_data *hist_data,
>  	struct event_trigger_data *test;
>  	struct hist_field *hist_field;
>  
> +	lockdep_assert_held(&event_mutex);
> +
>  	hist_field = find_var_field(hist_data, var_name);
>  	if (hist_field)
>  		return hist_field;
>  
> -	list_for_each_entry_rcu(test, &file->triggers, list) {
> +	list_for_each_entry(test, &file->triggers, list) {
>  		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
>  			test_data = test->private_data;
>  			hist_field = find_var_field(test_data, var_name);
> @@ -1807,7 +1809,9 @@ static struct hist_field *find_file_var(struct trace_event_file *file,
>  	struct event_trigger_data *test;
>  	struct hist_field *hist_field;
>  
> -	list_for_each_entry_rcu(test, &file->triggers, list) {
> +	lockdep_assert_held(&event_mutex);
> +
> +	list_for_each_entry(test, &file->triggers, list) {
>  		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
>  			test_data = test->private_data;
>  			hist_field = find_var_field(test_data, var_name);
> @@ -3102,7 +3106,9 @@ static char *find_trigger_filter(struct hist_trigger_data *hist_data,
>  {
>  	struct event_trigger_data *test;
>  
> -	list_for_each_entry_rcu(test, &file->triggers, list) {
> +	lockdep_assert_held(&event_mutex);
> +
> +	list_for_each_entry(test, &file->triggers, list) {
>  		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
>  			if (test->private_data == hist_data)
>  				return test->filter_str;
> @@ -3153,9 +3159,11 @@ find_compatible_hist(struct hist_trigger_data *target_hist_data,
>  	struct event_trigger_data *test;
>  	unsigned int n_keys;
>  
> +	lockdep_assert_held(&event_mutex);
> +
>  	n_keys = target_hist_data->n_fields - target_hist_data->n_vals;
>  
> -	list_for_each_entry_rcu(test, &file->triggers, list) {
> +	list_for_each_entry(test, &file->triggers, list) {
>  		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
>  			hist_data = test->private_data;
>  
> @@ -5515,7 +5523,7 @@ static int hist_show(struct seq_file *m, void *v)
>  		goto out_unlock;
>  	}
>  
> -	list_for_each_entry_rcu(data, &event_file->triggers, list) {
> +	list_for_each_entry(data, &event_file->triggers, list) {
>  		if (data->cmd_ops->trigger_type == ETT_EVENT_HIST)
>  			hist_trigger_show(m, data, n++);
>  	}
> @@ -5908,7 +5916,9 @@ static int hist_register_trigger(char *glob, struct event_trigger_ops *ops,
>  	if (hist_data->attrs->name && !named_data)
>  		goto new;
>  
> -	list_for_each_entry_rcu(test, &file->triggers, list) {
> +	lockdep_assert_held(&event_mutex);
> +
> +	list_for_each_entry(test, &file->triggers, list) {
>  		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
>  			if (!hist_trigger_match(data, test, named_data, false))
>  				continue;
> @@ -5992,10 +6002,12 @@ static bool have_hist_trigger_match(struct event_trigger_data *data,
>  	struct event_trigger_data *test, *named_data = NULL;
>  	bool match = false;
>  
> +	lockdep_assert_held(&event_mutex);
> +
>  	if (hist_data->attrs->name)
>  		named_data = find_named_trigger(hist_data->attrs->name);
>  
> -	list_for_each_entry_rcu(test, &file->triggers, list) {
> +	list_for_each_entry(test, &file->triggers, list) {
>  		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
>  			if (hist_trigger_match(data, test, named_data, false)) {
>  				match = true;
> @@ -6013,10 +6025,12 @@ static bool hist_trigger_check_refs(struct event_trigger_data *data,
>  	struct hist_trigger_data *hist_data = data->private_data;
>  	struct event_trigger_data *test, *named_data = NULL;
>  
> +	lockdep_assert_held(&event_mutex);
> +
>  	if (hist_data->attrs->name)
>  		named_data = find_named_trigger(hist_data->attrs->name);
>  
> -	list_for_each_entry_rcu(test, &file->triggers, list) {
> +	list_for_each_entry(test, &file->triggers, list) {
>  		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
>  			if (!hist_trigger_match(data, test, named_data, false))
>  				continue;
> @@ -6038,10 +6052,12 @@ static void hist_unregister_trigger(char *glob, struct event_trigger_ops *ops,
>  	struct event_trigger_data *test, *named_data = NULL;
>  	bool unregistered = false;
>  
> +	lockdep_assert_held(&event_mutex);
> +
>  	if (hist_data->attrs->name)
>  		named_data = find_named_trigger(hist_data->attrs->name);
>  
> -	list_for_each_entry_rcu(test, &file->triggers, list) {
> +	list_for_each_entry(test, &file->triggers, list) {
>  		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
>  			if (!hist_trigger_match(data, test, named_data, false))
>  				continue;
> @@ -6067,7 +6083,9 @@ static bool hist_file_check_refs(struct trace_event_file *file)
>  	struct hist_trigger_data *hist_data;
>  	struct event_trigger_data *test;
>  
> -	list_for_each_entry_rcu(test, &file->triggers, list) {
> +	lockdep_assert_held(&event_mutex);
> +
> +	list_for_each_entry(test, &file->triggers, list) {
>  		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
>  			hist_data = test->private_data;
>  			if (check_var_refs(hist_data))
> @@ -6310,7 +6328,8 @@ hist_enable_trigger(struct event_trigger_data *data, void *rec,
>  	struct enable_trigger_data *enable_data = data->private_data;
>  	struct event_trigger_data *test;
>  
> -	list_for_each_entry_rcu(test, &enable_data->file->triggers, list) {
> +	list_for_each_entry_rcu(test, &enable_data->file->triggers, list,
> +				lockdep_is_held(&event_mutex)) {
>  		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
>  			if (enable_data->enable)
>  				test->paused = false;
> diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c
> index 2cd53ca21b51..40106fff06a4 100644
> --- a/kernel/trace/trace_events_trigger.c
> +++ b/kernel/trace/trace_events_trigger.c
> @@ -501,7 +501,9 @@ void update_cond_flag(struct trace_event_file *file)
>  	struct event_trigger_data *data;
>  	bool set_cond = false;
>  
> -	list_for_each_entry_rcu(data, &file->triggers, list) {
> +	lockdep_assert_held(&event_mutex);
> +
> +	list_for_each_entry(data, &file->triggers, list) {
>  		if (data->filter || event_command_post_trigger(data->cmd_ops) ||
>  		    event_command_needs_rec(data->cmd_ops)) {
>  			set_cond = true;
> @@ -536,7 +538,9 @@ static int register_trigger(char *glob, struct event_trigger_ops *ops,
>  	struct event_trigger_data *test;
>  	int ret = 0;
>  
> -	list_for_each_entry_rcu(test, &file->triggers, list) {
> +	lockdep_assert_held(&event_mutex);
> +
> +	list_for_each_entry(test, &file->triggers, list) {
>  		if (test->cmd_ops->trigger_type == data->cmd_ops->trigger_type) {
>  			ret = -EEXIST;
>  			goto out;
> @@ -581,7 +585,9 @@ static void unregister_trigger(char *glob, struct event_trigger_ops *ops,
>  	struct event_trigger_data *data;
>  	bool unregistered = false;
>  
> -	list_for_each_entry_rcu(data, &file->triggers, list) {
> +	lockdep_assert_held(&event_mutex);
> +
> +	list_for_each_entry(data, &file->triggers, list) {
>  		if (data->cmd_ops->trigger_type == test->cmd_ops->trigger_type) {
>  			unregistered = true;
>  			list_del_rcu(&data->list);
> @@ -1497,7 +1503,9 @@ int event_enable_register_trigger(char *glob,
>  	struct event_trigger_data *test;
>  	int ret = 0;
>  
> -	list_for_each_entry_rcu(test, &file->triggers, list) {
> +	lockdep_assert_held(&event_mutex);
> +
> +	list_for_each_entry(test, &file->triggers, list) {
>  		test_enable_data = test->private_data;
>  		if (test_enable_data &&
>  		    (test->cmd_ops->trigger_type ==
> @@ -1537,7 +1545,9 @@ void event_enable_unregister_trigger(char *glob,
>  	struct event_trigger_data *data;
>  	bool unregistered = false;
>  
> -	list_for_each_entry_rcu(data, &file->triggers, list) {
> +	lockdep_assert_held(&event_mutex);
> +
> +	list_for_each_entry(data, &file->triggers, list) {
>  		enable_data = data->private_data;
>  		if (enable_data &&
>  		    (data->cmd_ops->trigger_type ==
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH -tip] tracing: trigger: Replace unneeded RCU-list traversals
  2019-12-20  2:31 [PATCH -tip] tracing: trigger: Replace unneeded RCU-list traversals Masami Hiramatsu
  2020-01-07  9:15 ` Masami Hiramatsu
@ 2020-01-07 16:37 ` Tom Zanussi
  1 sibling, 0 replies; 3+ messages in thread
From: Tom Zanussi @ 2020-01-07 16:37 UTC (permalink / raw)
  To: Masami Hiramatsu, Steven Rostedt
  Cc: linux-kernel, Joel Fernandes, Ingo Molnar, Paul E . McKenney

Hi Masami,

On Fri, 2019-12-20 at 11:31 +0900, Masami Hiramatsu wrote:
> With CONFIG_PROVE_RCU_LIST, I had many suspicious RCU warnings
> when I ran ftracetest trigger testcases.
> 
> -----
>   # dmesg -c > /dev/null
>   # ./ftracetest test.d/trigger
>   ...
>   # dmesg | grep "RCU-list traversed" | cut -f 2 -d ] | cut -f 2 -d "
> "
>   kernel/trace/trace_events_hist.c:6070
>   kernel/trace/trace_events_hist.c:1760
>   kernel/trace/trace_events_hist.c:5911
>   kernel/trace/trace_events_trigger.c:504
>   kernel/trace/trace_events_hist.c:1810
>   kernel/trace/trace_events_hist.c:3158
>   kernel/trace/trace_events_hist.c:3105
>   kernel/trace/trace_events_hist.c:5518
>   kernel/trace/trace_events_hist.c:5998
>   kernel/trace/trace_events_hist.c:6019
>   kernel/trace/trace_events_hist.c:6044
>   kernel/trace/trace_events_trigger.c:1500
>   kernel/trace/trace_events_trigger.c:1540
>   kernel/trace/trace_events_trigger.c:539
>   kernel/trace/trace_events_trigger.c:584
> -----
> 
> I investigated those warnings and found that the RCU-list
> traversals in event trigger and hist didn't need to use
> RCU version because those were called only under event_mutex.
> 
> I also checked other RCU-list traversals related to event
> trigger list, and found that most of them were called from
> event_hist_trigger_func() or hist_unregister_trigger() or
> register/unregister functions except for a few cases.
> 
> Replace these unneeded RCU-list traversals with normal list
> traversal macro and lockdep_assert_held() to check the
> event_mutex is held.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>

This patch looks fine to me, thanks for the fixes.

Reviewed-by: Tom Zanussi <zanussi@kernel.org>

> ---
>  kernel/trace/trace_events_hist.c    |   41
> ++++++++++++++++++++++++++---------
>  kernel/trace/trace_events_trigger.c |   20 +++++++++++++----
>  2 files changed, 45 insertions(+), 16 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_hist.c
> b/kernel/trace/trace_events_hist.c
> index 23b0195ac977..844f8325077f 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -1753,11 +1753,13 @@ static struct hist_field *find_var(struct
> hist_trigger_data *hist_data,
>  	struct event_trigger_data *test;
>  	struct hist_field *hist_field;
>  
> +	lockdep_assert_held(&event_mutex);
> +
>  	hist_field = find_var_field(hist_data, var_name);
>  	if (hist_field)
>  		return hist_field;
>  
> -	list_for_each_entry_rcu(test, &file->triggers, list) {
> +	list_for_each_entry(test, &file->triggers, list) {
>  		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
>  			test_data = test->private_data;
>  			hist_field = find_var_field(test_data,
> var_name);
> @@ -1807,7 +1809,9 @@ static struct hist_field *find_file_var(struct
> trace_event_file *file,
>  	struct event_trigger_data *test;
>  	struct hist_field *hist_field;
>  
> -	list_for_each_entry_rcu(test, &file->triggers, list) {
> +	lockdep_assert_held(&event_mutex);
> +
> +	list_for_each_entry(test, &file->triggers, list) {
>  		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
>  			test_data = test->private_data;
>  			hist_field = find_var_field(test_data,
> var_name);
> @@ -3102,7 +3106,9 @@ static char *find_trigger_filter(struct
> hist_trigger_data *hist_data,
>  {
>  	struct event_trigger_data *test;
>  
> -	list_for_each_entry_rcu(test, &file->triggers, list) {
> +	lockdep_assert_held(&event_mutex);
> +
> +	list_for_each_entry(test, &file->triggers, list) {
>  		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
>  			if (test->private_data == hist_data)
>  				return test->filter_str;
> @@ -3153,9 +3159,11 @@ find_compatible_hist(struct hist_trigger_data
> *target_hist_data,
>  	struct event_trigger_data *test;
>  	unsigned int n_keys;
>  
> +	lockdep_assert_held(&event_mutex);
> +
>  	n_keys = target_hist_data->n_fields - target_hist_data-
> >n_vals;
>  
> -	list_for_each_entry_rcu(test, &file->triggers, list) {
> +	list_for_each_entry(test, &file->triggers, list) {
>  		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
>  			hist_data = test->private_data;
>  
> @@ -5515,7 +5523,7 @@ static int hist_show(struct seq_file *m, void
> *v)
>  		goto out_unlock;
>  	}
>  
> -	list_for_each_entry_rcu(data, &event_file->triggers, list) {
> +	list_for_each_entry(data, &event_file->triggers, list) {
>  		if (data->cmd_ops->trigger_type == ETT_EVENT_HIST)
>  			hist_trigger_show(m, data, n++);
>  	}
> @@ -5908,7 +5916,9 @@ static int hist_register_trigger(char *glob,
> struct event_trigger_ops *ops,
>  	if (hist_data->attrs->name && !named_data)
>  		goto new;
>  
> -	list_for_each_entry_rcu(test, &file->triggers, list) {
> +	lockdep_assert_held(&event_mutex);
> +
> +	list_for_each_entry(test, &file->triggers, list) {
>  		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
>  			if (!hist_trigger_match(data, test,
> named_data, false))
>  				continue;
> @@ -5992,10 +6002,12 @@ static bool have_hist_trigger_match(struct
> event_trigger_data *data,
>  	struct event_trigger_data *test, *named_data = NULL;
>  	bool match = false;
>  
> +	lockdep_assert_held(&event_mutex);
> +
>  	if (hist_data->attrs->name)
>  		named_data = find_named_trigger(hist_data->attrs-
> >name);
>  
> -	list_for_each_entry_rcu(test, &file->triggers, list) {
> +	list_for_each_entry(test, &file->triggers, list) {
>  		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
>  			if (hist_trigger_match(data, test,
> named_data, false)) {
>  				match = true;
> @@ -6013,10 +6025,12 @@ static bool hist_trigger_check_refs(struct
> event_trigger_data *data,
>  	struct hist_trigger_data *hist_data = data->private_data;
>  	struct event_trigger_data *test, *named_data = NULL;
>  
> +	lockdep_assert_held(&event_mutex);
> +
>  	if (hist_data->attrs->name)
>  		named_data = find_named_trigger(hist_data->attrs-
> >name);
>  
> -	list_for_each_entry_rcu(test, &file->triggers, list) {
> +	list_for_each_entry(test, &file->triggers, list) {
>  		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
>  			if (!hist_trigger_match(data, test,
> named_data, false))
>  				continue;
> @@ -6038,10 +6052,12 @@ static void hist_unregister_trigger(char
> *glob, struct event_trigger_ops *ops,
>  	struct event_trigger_data *test, *named_data = NULL;
>  	bool unregistered = false;
>  
> +	lockdep_assert_held(&event_mutex);
> +
>  	if (hist_data->attrs->name)
>  		named_data = find_named_trigger(hist_data->attrs-
> >name);
>  
> -	list_for_each_entry_rcu(test, &file->triggers, list) {
> +	list_for_each_entry(test, &file->triggers, list) {
>  		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
>  			if (!hist_trigger_match(data, test,
> named_data, false))
>  				continue;
> @@ -6067,7 +6083,9 @@ static bool hist_file_check_refs(struct
> trace_event_file *file)
>  	struct hist_trigger_data *hist_data;
>  	struct event_trigger_data *test;
>  
> -	list_for_each_entry_rcu(test, &file->triggers, list) {
> +	lockdep_assert_held(&event_mutex);
> +
> +	list_for_each_entry(test, &file->triggers, list) {
>  		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
>  			hist_data = test->private_data;
>  			if (check_var_refs(hist_data))
> @@ -6310,7 +6328,8 @@ hist_enable_trigger(struct event_trigger_data
> *data, void *rec,
>  	struct enable_trigger_data *enable_data = data-
> >private_data;
>  	struct event_trigger_data *test;
>  
> -	list_for_each_entry_rcu(test, &enable_data->file->triggers,
> list) {
> +	list_for_each_entry_rcu(test, &enable_data->file->triggers,
> list,
> +				lockdep_is_held(&event_mutex)) {
>  		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
>  			if (enable_data->enable)
>  				test->paused = false;
> diff --git a/kernel/trace/trace_events_trigger.c
> b/kernel/trace/trace_events_trigger.c
> index 2cd53ca21b51..40106fff06a4 100644
> --- a/kernel/trace/trace_events_trigger.c
> +++ b/kernel/trace/trace_events_trigger.c
> @@ -501,7 +501,9 @@ void update_cond_flag(struct trace_event_file
> *file)
>  	struct event_trigger_data *data;
>  	bool set_cond = false;
>  
> -	list_for_each_entry_rcu(data, &file->triggers, list) {
> +	lockdep_assert_held(&event_mutex);
> +
> +	list_for_each_entry(data, &file->triggers, list) {
>  		if (data->filter || event_command_post_trigger(data-
> >cmd_ops) ||
>  		    event_command_needs_rec(data->cmd_ops)) {
>  			set_cond = true;
> @@ -536,7 +538,9 @@ static int register_trigger(char *glob, struct
> event_trigger_ops *ops,
>  	struct event_trigger_data *test;
>  	int ret = 0;
>  
> -	list_for_each_entry_rcu(test, &file->triggers, list) {
> +	lockdep_assert_held(&event_mutex);
> +
> +	list_for_each_entry(test, &file->triggers, list) {
>  		if (test->cmd_ops->trigger_type == data->cmd_ops-
> >trigger_type) {
>  			ret = -EEXIST;
>  			goto out;
> @@ -581,7 +585,9 @@ static void unregister_trigger(char *glob, struct
> event_trigger_ops *ops,
>  	struct event_trigger_data *data;
>  	bool unregistered = false;
>  
> -	list_for_each_entry_rcu(data, &file->triggers, list) {
> +	lockdep_assert_held(&event_mutex);
> +
> +	list_for_each_entry(data, &file->triggers, list) {
>  		if (data->cmd_ops->trigger_type == test->cmd_ops-
> >trigger_type) {
>  			unregistered = true;
>  			list_del_rcu(&data->list);
> @@ -1497,7 +1503,9 @@ int event_enable_register_trigger(char *glob,
>  	struct event_trigger_data *test;
>  	int ret = 0;
>  
> -	list_for_each_entry_rcu(test, &file->triggers, list) {
> +	lockdep_assert_held(&event_mutex);
> +
> +	list_for_each_entry(test, &file->triggers, list) {
>  		test_enable_data = test->private_data;
>  		if (test_enable_data &&
>  		    (test->cmd_ops->trigger_type ==
> @@ -1537,7 +1545,9 @@ void event_enable_unregister_trigger(char
> *glob,
>  	struct event_trigger_data *data;
>  	bool unregistered = false;
>  
> -	list_for_each_entry_rcu(data, &file->triggers, list) {
> +	lockdep_assert_held(&event_mutex);
> +
> +	list_for_each_entry(data, &file->triggers, list) {
>  		enable_data = data->private_data;
>  		if (enable_data &&
>  		    (data->cmd_ops->trigger_type ==
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2020-01-07 16:37 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-20  2:31 [PATCH -tip] tracing: trigger: Replace unneeded RCU-list traversals Masami Hiramatsu
2020-01-07  9:15 ` Masami Hiramatsu
2020-01-07 16:37 ` Tom Zanussi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.