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

[Steven Rostedt]

On Thu, 12 Oct 2023 16:37:35 -0400
Nick Lowell <nicholas.lowell@xxxxxxxxx> wrote:

Sorry, I was traveling when this was sent, and I missed it.


> I really appreciate the continued feedback.  I was able to reproduce.
> I think I'm understanding better but still need some help.
> I am actually wondering if remove_filter_string(system->filter) should

You mean to return true if filter->filter_string was not NULL?

> also return bool as an OR'd input for sync.
> Should it be something like this?
> 
>         if (!strcmp(strstrip(filter_string), "0")) {
> -               filter_free_subsystem_preds(dir, tr);
> -               remove_filter_string(system->filter);
> +              bool sync;

I would just make this an int;

> +
> +              sync = filter_free_subsystem_preds(dir, tr);
> +              sync = sync || remove_filter_string(system->filter);

And then just have:

		sync |= remove_filter_string(system->filter);

>                 filter = system->filter;
>                 system->filter = NULL;
> -               /* Ensure all filters are no longer used */
> -               tracepoint_synchronize_unregister();
> +              /* If nothing was freed, we do not need to sync */
> +              if(sync) {
> +                      /* Ensure all filters are no longer used */
> +                      tracepoint_synchronize_unregister();
> +              }
>                 filter_free_subsystem_filters(dir, tr);
>                 __free_filter(filter);
>                 goto out_unlock;
> 
> > Maybe even pass in "sync" to the filter_free_subsystem_filters() to make
> > sure there were nothing to be freed, and do the WARN_ON_ONCE() then.
> >
> >                 __free_filter(filter);
> >                 goto out_unlock;
> >         }
> >
> > -- Steve  
> 
> I'm not sure if I see the reasoning for the WARN_ON_ONCE() in
> filter_free_subsystem_filters()
> because it ends up checking the same if(!filter) just like
> filter_free_subsystem_preds() did earlier. It doesn't
> seem to do anything with system->filter.  I actually wonder if !sync,
> could filter_free_subsystem_filters()
> be skipped altogether.  Help me if I'm missing something.

The point is, code always changes. It's a bug if one of the filters had
content in filter_free_subsystem_filters() and sync is 0, hence the
WARN_ON_ONCE() if it does.

WARN_ON*()s are added to make sure the code is acting the way it is expected
to act. Yes, it should never trigger, but if it does, we know there's a bug
somewhere.

-- Steve