Re: [PATCH] aarch64: Optimized implementation of pthread_spin_lock and unlock

[Szabolcs Nagy <Szabolcs dot Nagy at arm dot com>]

On 28/10/2019 13:39, Xuelei Zhang wrote:
> An optimized algorithm of spin_lock is implemented to wait for
> random time in the case that multi-threads want to store spinlock
> at the same time. This way can make the operation of different
> threads asynchronous, thereby reducing bus conflicts, and futherly
> improving the overall performance, which benefits more on aarch64
> for its multi-core features.
> 
> In addition, the assembly version of spin_unlock is also implemented.

this has many issues

spinlock benchmarking is non-trivial (workload dependent,
uarch dependent and memory system dependent) and you need
to demonstrate that the new code is significantly better
than the generic c code, otherwise we don't want to
maintain asm in the aarch64 port.

the generic c code may gain improvements that outperform
whatever asm implementation (e.g. numa aware spinlock)
and allows compiling the code to different targets
(e.g. armv8.1-a+lse).

the arm arm has a spin lock implementation in the "barrier
litmus tests" section using wfe and sevl, if i had to add
asm i'd probably start with that.

> +ENTRY (pthread_spin_lock)
> +	DELOUSE (0)
> +
> +	mov	w1, #0x1
> +
> +L(spin):
> +	prfm	pldl1strm, [x0]
> +    /* A count to distinguish wating time.
> +    The larger the value, the more intense the current lock
> +    conflicts, and the longer the waiting time. */
> +	add	w3, w3, #0x1

w3 is not initialized, i would not call that "wait for
random time", since the lock contention is most likely
between the exact same code path executing in different
threads with identical w3 at function entry.

> +	ldaxr	w2, [x0]
> +	cbnz	w2, L(spin)
> +	stxr	w2, w1, [x0]
> +	cbnz	w2, 1f
> +	b	L(end)
> +
> +L(end):
> +	mov w0, #0x0
> +	ret
> +
> +    /* Set the loop times of L(wait) from 1000 to 7000 cycles,
> +    equals waiting 1~2us per 1000 cycles. */
> +	.p2align 4
> +7:
> +	mov	w6, #0x1b58
> +	b	L(wait_init)
> +6:
> +	mov	w6, #0x1770
> +	b	L(wait_init)
> +5:
> +	mov	w6, #0x1388
> +	b	L(wait_init)
> +4:
> +	mov	w6, #0x0fa0
> +	b	L(wait_init)
> +3:
> +	mov	w6, #0x0bb8
> +	b	L(wait_init)
> +2:
> +	mov	w6, #0x07d0
> +	b	L(wait_init)
> +1:
> +	mov	w6, #0x03e8
> +	b	L(wait_init)
> +
> +L(wait_init):
> +	mov	w5, #0x0
> +L(wait):
> +	add	w5, w5, #0x1
> +	cmp	w5, w6
> +	b.lt	L(wait) /* Wait ends when w5 equals w6. */
> +
> +L(stxr_try):
> +	ldr	w2, [x0]
> +	cbz	w2, L(spin)
> +	and	w3, w3, #0x07
> +    /* 8 kinds of distinguish wating time 
> +    based on the lower three bits of w3. */
> +	cmp	w3, #0x01
> +	beq	1b
> +	cmp	w3, #0x02
> +	beq	2b
> +	cmp	w3, #0x03
> +	beq	3b
> +	cmp	w3, #0x04
> +	beq	4b
> +	cmp	w3, #0x05
> +	beq	5b
> +	cmp	w3, #0x06
> +	beq	6b
> +	cmp	w3, #0x07
> +	beq	7b
> +
> +	wfe /* w3=0x000: the most intense situation so to wait 50 us */
> +	b	L(stxr_try)
> +
> +
> +END (pthread_spin_lock)
> +libc_hidden_builtin_def (pthread_spin_lock)
> +weak_alias (pthread_spin_lock, index)

that weak_alias is wrong.

> +ENTRY (pthread_spin_unlock)
> +	DELOUSE (0)
> +	stlr	wzr, [x0]

note: current glibc code would generate

  dmb ish
  str wzr, [x0]

instead of stlr, which is faster on some cores,
but has weaker ordering guarantees.

> +	mov	w0, #0x0
> +	ret
> +
> +END (pthread_spin_unlock)
> +libc_hidden_builtin_def (pthread_spin_unlock)
> +weak_alias (pthread_spin_unlock, index)

this weak_alias is wrong too.