Messages in this thread | | | Date | Thu, 21 Jul 2022 18:38:42 +0200 | Subject | Re: [PATCH V6 04/16] rv/include: Add deterministic automata monitor definition via C macros | From | Daniel Bristot de Oliveira <> |
| |
On 7/21/22 15:59, Steven Rostedt wrote: > On Thu, 21 Jul 2022 14:08:38 +0200 > Daniel Bristot de Oliveira <bristot@kernel.org> wrote: > >> On 7/20/22 22:06, Steven Rostedt wrote: >>>> +/* \ >>>> + * da_monitor_enabled_##name - checks if the monitor is enabled \ >>>> + */ \ >>>> +static inline bool da_monitor_enabled_##name(void) \ >>>> +{ \ >>> Should we add a: >>> >>> smp_rmb(); >>> >>> here? And then a smp_wmb() where these switches get updated? >>> >> >> Makes sense. >> >> Should I also add the READ_ONCE/WRITE_ONCE? like >> >> smp_rmb() >> READ_ONCE(var) >> >> WRITE_ONCE(var, value) >> smp_wmb() > > I'm not sure the WRITE_ONCE() is necessary with the memory barriers. > Because they should also prevent gcc from doing anything after that > barrier. As Linus once stated, most cases WRITE_ONCE() is useless, but it's > fine to keep more for annotation (as to pair with the READ_ONCE()) than for > anything that is critical.
Ack, I can keep for annotation.
>> >> for all these on/off knobs, or just the barriers? >> >>> I guess how critical is it that these turn off immediately after the switch >>> is flipped? >> >> It is not critical to continue the execution of those that have already crossed by >> the variable. Still, waiting for the tracepoints to finish their execution before >> returning to the user-space task that disabled the variable might be a good thing. > > You mean after disabling, to wait for the tracepoints that are currently > running to end?
yes, after disabling tracepoints.
>> >> IIRC, we can do that via RCU... like, synchronize_rcu()? > > We have tracepoint_synchronize_unregister() that does that, as some > traceponits use SRCU and not RCU.
yep, that is it!
-- Daniel
> -- Steve > > >> >>>> + /* global switch */ \ >>>> + if (unlikely(!rv_monitoring_on())) \ >>>> + return 0; \ >>>> + \ >>>> + /* monitor enabled */ \ >>>> + if (unlikely(!rv_##name.enabled)) \ >>>> + return 0; \ >>>> + \ >>>> + return 1; \ >>>> +} \ >>>> + \ >
| |