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@xxxxxxxxx> 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;
>                 }
>