kernel/trace/trace_events_filter.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)
From: Nicholas Lowell <nlowell@lexmark.com>
If there are no filters in the event subsystem, then there's no
reason to continue and hit the potentially time consuming
tracepoint_synchronize_unregister function. This should give
a speed up for initial disabling/configuring
Signed-off-by: Nicholas Lowell <nlowell@lexmark.com>
---
kernel/trace/trace_events_filter.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 33264e510d16..93653d37a132 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -1317,22 +1317,29 @@ void free_event_filter(struct event_filter *filter)
__free_filter(filter);
}
-static inline void __remove_filter(struct trace_event_file *file)
+static inline int __remove_filter(struct trace_event_file *file)
{
filter_disable(file);
- remove_filter_string(file->filter);
+ if (file->filter)
+ remove_filter_string(file->filter);
+ else
+ return 0;
+
+ return 1;
}
-static void filter_free_subsystem_preds(struct trace_subsystem_dir *dir,
+static int filter_free_subsystem_preds(struct trace_subsystem_dir *dir,
struct trace_array *tr)
{
struct trace_event_file *file;
+ int i = 0;
list_for_each_entry(file, &tr->events, list) {
if (file->system != dir)
continue;
- __remove_filter(file);
+ i += __remove_filter(file);
}
+ return i;
}
static inline void __free_subsystem_filter(struct trace_event_file *file)
@@ -2411,7 +2418,9 @@ int apply_subsystem_event_filter(struct trace_subsystem_dir *dir,
}
if (!strcmp(strstrip(filter_string), "0")) {
- filter_free_subsystem_preds(dir, tr);
+ if (filter_free_subsystem_preds(dir, tr) == 0)
+ goto out_unlock;
+
remove_filter_string(system->filter);
filter = system->filter;
system->filter = NULL;
--
2.25.1On Tue, 26 Sep 2023 10:20:58 -0400
Nicholas Lowell <nicholas.lowell@gmail.com> wrote:
> From: Nicholas Lowell <nlowell@lexmark.com>
>
> If there are no filters in the event subsystem, then there's no
> reason to continue and hit the potentially time consuming
> tracepoint_synchronize_unregister function. This should give
> a speed up for initial disabling/configuring
>
> Signed-off-by: Nicholas Lowell <nlowell@lexmark.com>
> ---
> kernel/trace/trace_events_filter.c | 19 ++++++++++++++-----
> 1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index 33264e510d16..93653d37a132 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -1317,22 +1317,29 @@ void free_event_filter(struct event_filter *filter)
> __free_filter(filter);
> }
>
> -static inline void __remove_filter(struct trace_event_file *file)
> +static inline int __remove_filter(struct trace_event_file *file)
> {
> filter_disable(file);
> - remove_filter_string(file->filter);
> + if (file->filter)
> + remove_filter_string(file->filter);
> + else
> + return 0;
> +
> + return 1;
The above looks awkward. What about:
if (!file->filter)
return 0;
remove_filter_string(file->filter);
return 1;
?
Or better yet:
if (!file->filter)
return false;
remove_filter_string(file->filter);
return true;
and ...
> }
>
> -static void filter_free_subsystem_preds(struct trace_subsystem_dir *dir,
> +static int filter_free_subsystem_preds(struct trace_subsystem_dir *dir,
> struct trace_array *tr)
> {
> struct trace_event_file *file;
> + int i = 0;
We don't really need a counter. It's either do the synchronization or
we don't.
bool do_sync = false;
>
> list_for_each_entry(file, &tr->events, list) {
> if (file->system != dir)
> continue;
> - __remove_filter(file);
> + i += __remove_filter(file);
if (remove_filter(file))
do_sync = true;
> }
return do_sync;
> + return i;
> }
>
> static inline void __free_subsystem_filter(struct trace_event_file *file)
> @@ -2411,7 +2418,9 @@ int apply_subsystem_event_filter(struct trace_subsystem_dir *dir,
> }
>
> if (!strcmp(strstrip(filter_string), "0")) {
> - filter_free_subsystem_preds(dir, tr);
> + if (filter_free_subsystem_preds(dir, tr) == 0)
> + goto out_unlock;
> +
/* If nothing was freed, we do not need to sync */
if (!filter_free_subsystem_preds(dir, tr))
goto out_unlock;
And yes, add the comment.
And actually, in that block with the goto out_unlock, we should have:
if (!filter_free_subsystem_preds(dir, tr)) {
if (!(WARN_ON_ONCE(system->filter))
goto out_unlock;
}
If there were no preds, ideally there would be no subsystem filter. But
if that's not the case, we need to warn about that and then continue.
-- Steve
> remove_filter_string(system->filter);
> filter = system->filter;
> system->filter = NULL;
Sending again in plain text mode.
Thanks for the great feedback! Hopefully my inline comments/questions
aren't garbled.
On Sat, Sep 30, 2023 at 4:04 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Tue, 26 Sep 2023 10:20:58 -0400
> Nicholas Lowell <nicholas.lowell@gmail.com> wrote:
>
> > From: Nicholas Lowell <nlowell@lexmark.com>
> >
> > If there are no filters in the event subsystem, then there's no
> > reason to continue and hit the potentially time consuming
> > tracepoint_synchronize_unregister function. This should give
> > a speed up for initial disabling/configuring
> >
> > Signed-off-by: Nicholas Lowell <nlowell@lexmark.com>
> > ---
> > kernel/trace/trace_events_filter.c | 19 ++++++++++++++-----
> > 1 file changed, 14 insertions(+), 5 deletions(-)
> >
> > diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> > index 33264e510d16..93653d37a132 100644
> > --- a/kernel/trace/trace_events_filter.c
> > +++ b/kernel/trace/trace_events_filter.c
> > @@ -1317,22 +1317,29 @@ void free_event_filter(struct event_filter *filter)
> > __free_filter(filter);
> > }
> >
> > -static inline void __remove_filter(struct trace_event_file *file)
> > +static inline int __remove_filter(struct trace_event_file *file)
> > {
> > filter_disable(file);
> > - remove_filter_string(file->filter);
> > + if (file->filter)
> > + remove_filter_string(file->filter);
> > + else
> > + return 0;
> > +
> > + return 1;
>
> The above looks awkward. What about:
>
> if (!file->filter)
> return 0;
>
> remove_filter_string(file->filter);
> return 1;
>
> ?
>
> Or better yet:
>
> if (!file->filter)
> return false;
>
> remove_filter_string(file->filter);
> return true;
>
Is it safe to assume you would like the function's return type to
change from int to bool if I go with option 2?
> and ...
>
> > }
> >
> > -static void filter_free_subsystem_preds(struct trace_subsystem_dir *dir,
> > +static int filter_free_subsystem_preds(struct trace_subsystem_dir *dir,
> > struct trace_array *tr)
> > {
> > struct trace_event_file *file;
> > + int i = 0;
>
> We don't really need a counter. It's either do the synchronization or
> we don't.
>
> bool do_sync = false;
>
> >
> > list_for_each_entry(file, &tr->events, list) {
> > if (file->system != dir)
> > continue;
> > - __remove_filter(file);
> > + i += __remove_filter(file);
>
> if (remove_filter(file))
> do_sync = true;
>
> > }
>
> return do_sync;
>
Going to assume the same here--that return type should change from int to bool.
> > + return i;
> > }
> >
> > static inline void __free_subsystem_filter(struct trace_event_file *file)
> > @@ -2411,7 +2418,9 @@ int apply_subsystem_event_filter(struct trace_subsystem_dir *dir,
> > }
> >
> > if (!strcmp(strstrip(filter_string), "0")) {
> > - filter_free_subsystem_preds(dir, tr);
> > + if (filter_free_subsystem_preds(dir, tr) == 0)
> > + goto out_unlock;
> > +
>
> /* If nothing was freed, we do not need to sync */
> if (!filter_free_subsystem_preds(dir, tr))
> goto out_unlock;
>
> And yes, add the comment.
>
> And actually, in that block with the goto out_unlock, we should have:
>
> if (!filter_free_subsystem_preds(dir, tr)) {
> if (!(WARN_ON_ONCE(system->filter))
> goto out_unlock;
> }
>
Can you explain why the WARN_ON_ONCE should be in a conditional?
Don't we still want the original conditional to cause the goto regardless?
if (!filter_free_subsystem_preds(dir, tr)) {
WARN_ON_ONCE(system->filter);
goto out_unlock;
}
> If there were no preds, ideally there would be no subsystem filter. But
> if that's not the case, we need to warn about that and then continue.
>
> -- Steve
>
> > remove_filter_string(system->filter);
> > filter = system->filter;
> > system->filter = NULL;
>
© 2016 - 2026 Red Hat, Inc.