[PATCH] Added asm ISB as asm pause for ngx_cpu_pause() for aarch64

[Maxim Dounin]

Hello!

Thank you for the patch.
Some comments and questions below.

On Wed, Dec 06, 2023 at 10:06:57AM -0600, julio.suarez at foss.arm.com wrote:

> # HG changeset patch
> # User Julio Suarez <julio.suarez at arm.com>
> # Date 1701877879 21600
> #      Wed Dec 06 09:51:19 2023 -0600
> # Node ID 53d289b8676fc678ca90e02ece174300a3631f79
> # Parent  f366007dd23a6ce8e8427c1b3042781b618a2ade
> Added asm ISB as asm pause for ngx_cpu_pause() for aarch64

Nitpicking:

Added ISB as ngx_cpu_pause() for aarch64.

> 
> For aarch64 (A.K.A. Arm64), ngx_cpu_pause() evaluates to empty by the
> GCC preprocessor. This results in an empty for loop in the lock
> check code in ngx_rwlock.c/ngx_spinlock.c (a bug).
> Thus, on Arm CPUs, there is no wait between checks of a lock.

Could you please clarify what do you mean by "a bug"?  An empty 
ngx_cpu_pause() is certainly not a bug, it's just a lack of a more 
optimal solution for the particular architecture.

> When a delay like the PAUSE that is there for x86 is added,
> there is a 2-5% increase in the number of requests/sec Arm CPUs
> can achieve under high load.

Could you please provide some details on how did you get these 
numbers?

> Currently, options for a spin lock delay element
> are YIELD and ISB. YIELD is assembled into a NOP for most Arm CPUs.
> YIELD is for Arm implementations with SMT.
> Most Arm implementations are single core - single thread (non-SMT).
> Thus, this commit uses ISB (Instruction Barrier Sync) since it
> will provide a slightly longer delay than a NOP.
> 
> Other projects that implement spin locks have used ISB as
> the delay element which has yielded performance gains as well.

Looking through various open source projects, I'm seeing the 
following uses on arm64 CPUs:

FreeBSD, sys/arm64/include/cpu.h:

#define cpu_spinwait()          __asm __volatile("yield" ::: "memory")

FreeBSD, lib/libthr/arch/aarch64/include/pthread_md.h:

#define	CPU_SPINWAIT

Linux, arch/arm64/include/asm/vdso/processor.h:

static inline void cpu_relax(void)
{
        asm volatile("yield" ::: "memory");
}

The only popular project I was able to find which uses ISB is 
Rust:

https://github.com/rust-lang/rust/commit/c064b6560b7ce0adeb9bbf5d7dcf12b1acb0c807

>From the commit log it looks like it mostly focuses on the delay 
introduced by the instruction, ignoring other effects. In 
particular, YIELD is expected to be more friendly for various 
emulation environments, see Linux commit here:

https://github.com/torvalds/linux/commit/1baa82f48030f38d1895301f1ec93acbcb3d15db

Overall, the YIELD instruction seems to be better suited and 
specifically designed for the task in question, as per the 
documentation
(https://developer.arm.com/documentation/ddi0596/2021-12/Base-Instructions/YIELD--YIELD-):

: YIELD is a hint instruction. Software with a multithreading 
: capability can use a YIELD instruction to indicate to the PE that 
: it is performing a task, for example a spin-lock, that could be 
: swapped out to improve overall system performance. The PE can use 
: this hint to suspend and resume multiple software threads if it 
: supports the capability.

Please also note that ngx_cpu_pause() is only used on 
multiprocessor systems: the code checks if (ngx_ncpu > 1) before 
using it.

> 
> Last, ISB is not an architecturally defined solution
> for short spin lock delays.
> A candidate for a short delay solution is the WFET
> instruction (Wait For Event with Timeout).
> However, there aren't any Arm implementations in the market that
> support this instruction yet. When that occurs,
> WFET can be tested as a replacement for ISB. Until then,
> ISB will do.
> 
> diff -r f366007dd23a -r 53d289b8676f src/os/unix/ngx_atomic.h
> --- a/src/os/unix/ngx_atomic.h	Tue Nov 14 15:26:02 2023 +0400
> +++ b/src/os/unix/ngx_atomic.h	Wed Dec 06 09:51:19 2023 -0600
> @@ -66,6 +66,8 @@
>  
>  #if ( __i386__ || __i386 || __amd64__ || __amd64 )
>  #define ngx_cpu_pause()             __asm__ ("pause")
> +#elif ( __aarch64__ )
> +#define ngx_cpu_pause()             __asm__ ("isb" ::: "memory")

Could you please clarify reasons for the "memory" clobber here?

>  #else
>  #define ngx_cpu_pause()
>  #endif

-- 
Maxim Dounin
http://mdounin.ru/