Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Message-ID: <c36473f6-6633-498c-897e-6513513bea2e@efficios.com>
Date:   Wed, 13 Dec 2023 22:05:29 -0500
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] ring-buffer: Remove 32bit timestamp logic
Content-Language: en-US
To:     Steven Rostedt <rostedt@goodmis.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Linux Trace Kernel <linux-trace-kernel@vger.kernel.org>
Cc:     Masami Hiramatsu <mhiramat@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>
References: <20231213211126.24f8c1dd@gandalf.local.home>
From:   Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
In-Reply-To: <20231213211126.24f8c1dd@gandalf.local.home>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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