Messages in this thread | | | Date | Tue, 26 Apr 2022 10:25:02 -0400 | From | Steven Rostedt <> | Subject | Re: [PATCH v2 3/4] tracing: Replace usage of found with dedicated list iterator variable |
| |
On Sat, 2 Apr 2022 12:33:40 +0200 Jakob Koschel <jakobkoschel@gmail.com> wrote:
> To move the list iterator variable into the list_for_each_entry_*() > macro in the future it should be avoided to use the list iterator > variable after the loop body. > > To *never* use the list iterator variable after the loop it was > concluded to use a separate iterator variable instead of a > found boolean [1]. > > This removes the need to use a found variable and simply checking if > the variable was set, can determine if the break/goto was hit. > > Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1] > Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com> > --- > kernel/trace/trace_events_hist.c | 17 ++++++++--------- > kernel/trace/trace_events_trigger.c | 28 +++++++++++++--------------- > 2 files changed, 21 insertions(+), 24 deletions(-) > > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c > index 44db5ba9cabb..7f60d04d5b6e 100644 > --- a/kernel/trace/trace_events_hist.c > +++ b/kernel/trace/trace_events_hist.c > @@ -6089,32 +6089,31 @@ static void hist_unregister_trigger(char *glob, > struct event_trigger_data *data, > struct trace_event_file *file) > { > + struct event_trigger_data *test = NULL, *iter, *named_data = NULL; > struct hist_trigger_data *hist_data = data->private_data; > - 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(test, &file->triggers, list) { > - if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) { > - if (!hist_trigger_match(data, test, named_data, false)) > + list_for_each_entry(iter, &file->triggers, list) { > + if (iter->cmd_ops->trigger_type == ETT_EVENT_HIST) { > + if (!hist_trigger_match(data, iter, named_data, false)) > continue; > - unregistered = true; > - list_del_rcu(&test->list); > + test = iter; > + list_del_rcu(&iter->list); > trace_event_trigger_enable_disable(file, 0); > update_cond_flag(file); > break; > } > } > > - if (unregistered && test->ops->free) > + if (test && test->ops->free) > test->ops->free(test->ops, test); > > if (hist_data->enable_timestamps) { > - if (!hist_data->remove || unregistered) > + if (!hist_data->remove || test) > tracing_set_filter_buffering(file->tr, false); > } > } > diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c > index 7eb9d04f1c2e..1ba54a489416 100644 > --- a/kernel/trace/trace_events_trigger.c > +++ b/kernel/trace/trace_events_trigger.c > @@ -614,22 +614,21 @@ static void unregister_trigger(char *glob, > struct event_trigger_data *test, > struct trace_event_file *file) > { > - struct event_trigger_data *data; > - bool unregistered = false; > + struct event_trigger_data *data = NULL, *iter; > > 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); > + list_for_each_entry(iter, &file->triggers, list) { > + if (iter->cmd_ops->trigger_type == test->cmd_ops->trigger_type) { > + data = iter; > + list_del_rcu(&iter->list);
Don't use the iterator in the logic.
data = iter; list_del_rcu(&data->list);
is just fine.
> trace_event_trigger_enable_disable(file, 0); > update_cond_flag(file); > break; > } > } > > - if (unregistered && data->ops->free) > + if (data && data->ops->free) > data->ops->free(data->ops, data); > } > > @@ -1976,27 +1975,26 @@ void event_enable_unregister_trigger(char *glob, > struct trace_event_file *file) > { > struct enable_trigger_data *test_enable_data = test->private_data; > + struct event_trigger_data *data = NULL, *iter; > struct enable_trigger_data *enable_data; > - struct event_trigger_data *data; > - bool unregistered = false; > > lockdep_assert_held(&event_mutex); > > - list_for_each_entry(data, &file->triggers, list) { > - enable_data = data->private_data; > + list_for_each_entry(iter, &file->triggers, list) { > + enable_data = iter->private_data; > if (enable_data && > - (data->cmd_ops->trigger_type == > + (iter->cmd_ops->trigger_type == > test->cmd_ops->trigger_type) && > (enable_data->file == test_enable_data->file)) { > - unregistered = true; > - list_del_rcu(&data->list); > + data = iter; > + list_del_rcu(&iter->list);
Same here.
And please rebase on top of Linus's lastest tree.
-- Steve
> trace_event_trigger_enable_disable(file, 0); > update_cond_flag(file); > break; > } > } > > - if (unregistered && data->ops->free) > + if (data && data->ops->free) > data->ops->free(data->ops, data); > } >
| |