Re: [PATCH 1/2] tracing: Simplify and fix "buffered event" synchronization

[Steven Rostedt]

On Wed, 29 Nov 2023 10:22:23 +0100
Petr Pavlu <petr.pavlu@xxxxxxxx> 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