Skip to content

Commit aeed8aa

Browse files
mhiramatrostedt
mhiramat
authored and
rostedt
committed
tracing: trigger: Replace unneeded RCU-list traversals
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. Link: http://lkml.kernel.org/r/157680910305.11685.15110237954275915782.stgit@devnote2 Cc: [email protected] Fixes: 30350d6 ("tracing: Add variable support to hist triggers") Reviewed-by: Tom Zanussi <[email protected]> Signed-off-by: Masami Hiramatsu <[email protected]> Signed-off-by: Steven Rostedt (VMware) <[email protected]>
1 parent 99c9a92 commit aeed8aa

File tree

2 files changed

+45
-16
lines changed

2 files changed

+45
-16
lines changed

‎kernel/trace/trace_events_hist.c

Lines changed: 30 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1766,11 +1766,13 @@ static struct hist_field *find_var(struct hist_trigger_data *hist_data,
17661766
struct event_trigger_data *test;
17671767
struct hist_field *hist_field;
17681768

1769+
lockdep_assert_held(&event_mutex);
1770+
17691771
hist_field = find_var_field(hist_data, var_name);
17701772
if (hist_field)
17711773
return hist_field;
17721774

1773-
list_for_each_entry_rcu(test, &file->triggers, list) {
1775+
list_for_each_entry(test, &file->triggers, list) {
17741776
if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
17751777
test_data = test->private_data;
17761778
hist_field = find_var_field(test_data, var_name);
@@ -1820,7 +1822,9 @@ static struct hist_field *find_file_var(struct trace_event_file *file,
18201822
struct event_trigger_data *test;
18211823
struct hist_field *hist_field;
18221824

1823-
list_for_each_entry_rcu(test, &file->triggers, list) {
1825+
lockdep_assert_held(&event_mutex);
1826+
1827+
list_for_each_entry(test, &file->triggers, list) {
18241828
if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
18251829
test_data = test->private_data;
18261830
hist_field = find_var_field(test_data, var_name);
@@ -3115,7 +3119,9 @@ static char *find_trigger_filter(struct hist_trigger_data *hist_data,
31153119
{
31163120
struct event_trigger_data *test;
31173121

3118-
list_for_each_entry_rcu(test, &file->triggers, list) {
3122+
lockdep_assert_held(&event_mutex);
3123+
3124+
list_for_each_entry(test, &file->triggers, list) {
31193125
if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
31203126
if (test->private_data == hist_data)
31213127
return test->filter_str;
@@ -3166,9 +3172,11 @@ find_compatible_hist(struct hist_trigger_data *target_hist_data,
31663172
struct event_trigger_data *test;
31673173
unsigned int n_keys;
31683174

3175+
lockdep_assert_held(&event_mutex);
3176+
31693177
n_keys = target_hist_data->n_fields - target_hist_data->n_vals;
31703178

3171-
list_for_each_entry_rcu(test, &file->triggers, list) {
3179+
list_for_each_entry(test, &file->triggers, list) {
31723180
if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
31733181
hist_data = test->private_data;
31743182

@@ -5528,7 +5536,7 @@ static int hist_show(struct seq_file *m, void *v)
55285536
goto out_unlock;
55295537
}
55305538

5531-
list_for_each_entry_rcu(data, &event_file->triggers, list) {
5539+
list_for_each_entry(data, &event_file->triggers, list) {
55325540
if (data->cmd_ops->trigger_type == ETT_EVENT_HIST)
55335541
hist_trigger_show(m, data, n++);
55345542
}
@@ -5921,7 +5929,9 @@ static int hist_register_trigger(char *glob, struct event_trigger_ops *ops,
59215929
if (hist_data->attrs->name && !named_data)
59225930
goto new;
59235931

5924-
list_for_each_entry_rcu(test, &file->triggers, list) {
5932+
lockdep_assert_held(&event_mutex);
5933+
5934+
list_for_each_entry(test, &file->triggers, list) {
59255935
if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
59265936
if (!hist_trigger_match(data, test, named_data, false))
59275937
continue;
@@ -6005,10 +6015,12 @@ static bool have_hist_trigger_match(struct event_trigger_data *data,
60056015
struct event_trigger_data *test, *named_data = NULL;
60066016
bool match = false;
60076017

6018+
lockdep_assert_held(&event_mutex);
6019+
60086020
if (hist_data->attrs->name)
60096021
named_data = find_named_trigger(hist_data->attrs->name);
60106022

6011-
list_for_each_entry_rcu(test, &file->triggers, list) {
6023+
list_for_each_entry(test, &file->triggers, list) {
60126024
if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
60136025
if (hist_trigger_match(data, test, named_data, false)) {
60146026
match = true;
@@ -6026,10 +6038,12 @@ static bool hist_trigger_check_refs(struct event_trigger_data *data,
60266038
struct hist_trigger_data *hist_data = data->private_data;
60276039
struct event_trigger_data *test, *named_data = NULL;
60286040

6041+
lockdep_assert_held(&event_mutex);
6042+
60296043
if (hist_data->attrs->name)
60306044
named_data = find_named_trigger(hist_data->attrs->name);
60316045

6032-
list_for_each_entry_rcu(test, &file->triggers, list) {
6046+
list_for_each_entry(test, &file->triggers, list) {
60336047
if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
60346048
if (!hist_trigger_match(data, test, named_data, false))
60356049
continue;
@@ -6051,10 +6065,12 @@ static void hist_unregister_trigger(char *glob, struct event_trigger_ops *ops,
60516065
struct event_trigger_data *test, *named_data = NULL;
60526066
bool unregistered = false;
60536067

6068+
lockdep_assert_held(&event_mutex);
6069+
60546070
if (hist_data->attrs->name)
60556071
named_data = find_named_trigger(hist_data->attrs->name);
60566072

6057-
list_for_each_entry_rcu(test, &file->triggers, list) {
6073+
list_for_each_entry(test, &file->triggers, list) {
60586074
if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
60596075
if (!hist_trigger_match(data, test, named_data, false))
60606076
continue;
@@ -6080,7 +6096,9 @@ static bool hist_file_check_refs(struct trace_event_file *file)
60806096
struct hist_trigger_data *hist_data;
60816097
struct event_trigger_data *test;
60826098

6083-
list_for_each_entry_rcu(test, &file->triggers, list) {
6099+
lockdep_assert_held(&event_mutex);
6100+
6101+
list_for_each_entry(test, &file->triggers, list) {
60846102
if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
60856103
hist_data = test->private_data;
60866104
if (check_var_refs(hist_data))
@@ -6323,7 +6341,8 @@ hist_enable_trigger(struct event_trigger_data *data, void *rec,
63236341
struct enable_trigger_data *enable_data = data->private_data;
63246342
struct event_trigger_data *test;
63256343

6326-
list_for_each_entry_rcu(test, &enable_data->file->triggers, list) {
6344+
list_for_each_entry_rcu(test, &enable_data->file->triggers, list,
6345+
lockdep_is_held(&event_mutex)) {
63276346
if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
63286347
if (enable_data->enable)
63296348
test->paused = false;

‎kernel/trace/trace_events_trigger.c

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -501,7 +501,9 @@ void update_cond_flag(struct trace_event_file *file)
501501
struct event_trigger_data *data;
502502
bool set_cond = false;
503503

504-
list_for_each_entry_rcu(data, &file->triggers, list) {
504+
lockdep_assert_held(&event_mutex);
505+
506+
list_for_each_entry(data, &file->triggers, list) {
505507
if (data->filter || event_command_post_trigger(data->cmd_ops) ||
506508
event_command_needs_rec(data->cmd_ops)) {
507509
set_cond = true;
@@ -536,7 +538,9 @@ static int register_trigger(char *glob, struct event_trigger_ops *ops,
536538
struct event_trigger_data *test;
537539
int ret = 0;
538540

539-
list_for_each_entry_rcu(test, &file->triggers, list) {
541+
lockdep_assert_held(&event_mutex);
542+
543+
list_for_each_entry(test, &file->triggers, list) {
540544
if (test->cmd_ops->trigger_type == data->cmd_ops->trigger_type) {
541545
ret = -EEXIST;
542546
goto out;
@@ -581,7 +585,9 @@ static void unregister_trigger(char *glob, struct event_trigger_ops *ops,
581585
struct event_trigger_data *data;
582586
bool unregistered = false;
583587

584-
list_for_each_entry_rcu(data, &file->triggers, list) {
588+
lockdep_assert_held(&event_mutex);
589+
590+
list_for_each_entry(data, &file->triggers, list) {
585591
if (data->cmd_ops->trigger_type == test->cmd_ops->trigger_type) {
586592
unregistered = true;
587593
list_del_rcu(&data->list);
@@ -1497,7 +1503,9 @@ int event_enable_register_trigger(char *glob,
14971503
struct event_trigger_data *test;
14981504
int ret = 0;
14991505

1500-
list_for_each_entry_rcu(test, &file->triggers, list) {
1506+
lockdep_assert_held(&event_mutex);
1507+
1508+
list_for_each_entry(test, &file->triggers, list) {
15011509
test_enable_data = test->private_data;
15021510
if (test_enable_data &&
15031511
(test->cmd_ops->trigger_type ==
@@ -1537,7 +1545,9 @@ void event_enable_unregister_trigger(char *glob,
15371545
struct event_trigger_data *data;
15381546
bool unregistered = false;
15391547

1540-
list_for_each_entry_rcu(data, &file->triggers, list) {
1548+
lockdep_assert_held(&event_mutex);
1549+
1550+
list_for_each_entry(data, &file->triggers, list) {
15411551
enable_data = data->private_data;
15421552
if (enable_data &&
15431553
(data->cmd_ops->trigger_type ==

0 commit comments

Comments
 (0)