* [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 related [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.