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 <[email protected]>
---
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);
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);
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);
}
--
2.25.1
On Sat, 2 Apr 2022 12:33:40 +0200
Jakob Koschel <[email protected]> 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 <[email protected]>
> ---
> 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);
> }
>