Messages in this thread | | | Date | Wed, 29 Nov 2023 09:58:26 -0500 | From | Steven Rostedt <> | Subject | Re: [PATCH 1/2] tracing: Simplify and fix "buffered event" synchronization |
| |
On Wed, 29 Nov 2023 10:22:23 +0100 Petr Pavlu <petr.pavlu@suse.com> wrote:
> Yes, I believe this should address this potential race condition. > > An alternative would be instead to update > trace_event_buffer_lock_reserve() as follows: > > if (this_cpu_inc_return(trace_buffered_event_cnt) == 1) { > smp_rmb();
This is the problem I have with your approach. That smp_rmb() is in the highly critical path. On some architectures, this has a significant impact on the overhead of this code. This path is called during code execution and increases the overhead of the tracing infrastructure.
If I'm given two solutions where one adds a smp_rmb() to the critical path and the other just slows down the non-critical path more, I'll take the slow down of non-critical path every time.
> if ((entry = __this_cpu_read(trace_buffered_event))) { > [...] > > That saves the synchronize_rcu() call but additionally modifies > trace_buffered_event_cnt even if trace_buffered_event is currently NULL. > > Another alternative is the approach taken by my patch which avoids more > RCU work and unnecessary memory modifications. > > I'd be interested if you could have a look again at what I'm proposing > in my patch. I think it simplifies the code while addressing these > problems as well. However, if you have reservations about that approach > then it is ok, I can fix the found problems individually as discussed.
Fix this without adding any memory barriers to the critical path, then I'll take another look.
FYI, this code was designed in the first place to avoid adding memory barriers in the critical path.
Thanks!
-- Steve
| |