[PATCH] trace: tracing_event_filter: fast path when no subsystem filters

[PATCH] trace: tracing_event_filter: fast path when no subsystem filters

[Nicholas Lowell]

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.1

Re: [PATCH] trace: tracing_event_filter: fast path when no subsystem filters

[Steven Rostedt]

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;

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;

Re: [PATCH] trace: tracing_event_filter: fast path when no subsystem filters

[Nick Lowell]

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;
>

Re: [PATCH] trace: tracing_event_filter: fast path when no subsystem filters

[Steven Rostedt]

On Mon, 2 Oct 2023 09:57:34 -0400
Nick Lowell <nicholas.lowell@gmail.com> wrote:
> >
> > 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?

Yes.

> 
> 
> > 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.
> 

Correct.

> 
> > > +     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?
> 

Because if it exists, we still want to free it and do the synchronization,
and set it to NULL. In other words, it means we missed something and need
to revert back to the original behavior.

The WARN_ON_ONCE() documents that we never expect that to happen, and if it
does, it means we have a bug.

-- Steve


> 
> 
>                 if (!filter_free_subsystem_preds(dir, tr)) {
>                         WARN_ON_ONCE(system->filter);
>                         goto out_unlock;
>                 }
>