Re: [PATCH] ring-buffer: Remove 32bit timestamp logic

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

On 2023-12-13 21:11, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Each event has a 27 bit timestamp delta that is used to hold the delta
> from the last event. If the time between events is greater than 2^27, then
> a timestamp is added that holds a 59 bit absolute timestamp.
> 
> Until a389d86f7fd09 ("ring-buffer: Have nested events still record running
> time stamp"), if an interrupt interrupted an event in progress, all the
> events delta would be zero to not deal with the races that need to be
> handled. The commit a389d86f7fd09 changed that to handle the races giving
> all events, even those that preempt other events, still have an accurate
> timestamp.
> 
> To handle those races requires performing 64-bit cmpxchg on the
> timestamps. But doing 64-bit cmpxchg on 32-bit architectures is considered
> very slow. To try to deal with this the timestamp logic was broken into
> two and then three 32-bit cmpxchgs, with the thought that two (or three)
> 32-bit cmpxchgs are still faster than a single 64-bit cmpxchg on 32-bit
> architectures.
> 
> Part of the problem with this is that I didn't have any 32-bit
> architectures to test on. After hitting several subtle bugs in this code,
> an effort was made to try and see if three 32-bit cmpxchgs are indeed
> faster than a single 64-bit. After a few people brushed off the dust of
> their old 32-bit machines, tests were done, and even though 32-bit cmpxchg
> was faster than a single 64-bit, it was in the order of 50% at best, not
> 300%.

I literally had to dust off my old Eee PC for this :)

> 
> This means that this complex code is not only complex but also not even
> faster than just using 64-bit cmpxchg.
> 
> Nuke it!
> 

Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

> @@ -3720,6 +3517,7 @@ rb_reserve_next_event(struct trace_buffer *buffer,
>   	struct rb_event_info info;
>   	int nr_loops = 0;
>   	int add_ts_default;
> +	static int once;

(as you spotted, should be removed)

Thanks,

Mathieu

>   
>   	/* ring buffer does cmpxchg, make sure it is safe in NMI context */
>   	if (!IS_ENABLED(CONFIG_ARCH_HAVE_NMI_SAFE_CMPXCHG) &&

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com