Messages in this thread | | | Date | Wed, 22 Nov 2023 16:20:51 -0500 | From | Steven Rostedt <> | Subject | Re: [PATCH v2] trace: tracing_event_filter: fast path when no subsystem filters |
| |
On Thu, 12 Oct 2023 16:37:35 -0400 Nick Lowell <nicholas.lowell@gmail.com> 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
| |