Re: [PATCH] s390: Use generic lowlevellock-futex.h.

[Stefan Liebler <stli at linux dot vnet dot ibm dot com>]

Hi,

On 01/07/2015 04:44 PM, Torvald Riegel wrote:
> Ping.
>
> On Wed, 2014-12-17 at 23:59 +0100, Torvald Riegel wrote:
> > This untested patch removes the custom futex operations in s390
> > lowlevellock.h, and instead uses the generic ones from
> > lowlevellock-futex.h.  They seem to be equivalent.
The behaviour of lll_futex_requeue, lll_futex_wake_unlock, 
lll_futex_cmp_requeue_pi has changed. The s390-variant returns 0 or 1
and the lowlevellock-futex.h-variant returns 0 or __ret.
The used return values of those macros are compared equal or unequal to 
zero.
Thus this is okay.

> > It also removes the definition of SYS_futex because it doesn't seem to
> > be used anywhere else.
OK

> > It would be even nicer if we could remove the custom lowlevellock.h
> > altogether.  However, current s390 has custom asm for the lock fast
> > paths, so this needs a closer review than I can do.  Could you check
> > whether that would be possible?
The custom lowlevellock.h canÂt be removed completely because of the 
lock-elision defines. But including the generic lowlevellock.h as power 
does is fine.
Those lll_* macros use the atomic_* macros and the asm-output is nearly 
equivalent to the custom implementation.

The s390 lll_cond_lock uses
if ( atomic_compare_and_exchange_bool_acq(futex, 2, 0) )
but the generic code uses
if ( atomic_exchange_acq(futex, 2) != 0 ).
In both cases __lll_lock_wait is called if the futex was != 0 before.
I think this is a bug in s390-variant, because the futex is only set to 
2 if futex was 0. If futex was 2 before, then futex remains 2 and 
__lll_lock_wait is called. If futex was 1 before, then the s390-code 
does not set futex to 2, but __lll_lock_wait is called. The generic code 
always sets futex to 2.

The generic lll_[robust_]unlock macros use atomic_exchange_rel (__futex, 
0). The behaviour for s390-variant is the same, but
the asm-output for generic-variant needs one additional register and two 
additional instructions. The old-value is stored before compare-and-swap 
instruction in order to compare it against the "old-value" from 
cs-instruction. The s390-variant uses the condition code of 
cs-instruction to determine, if the cs-instruction has changed the value.
Thus lll_[robust_]unlock macros or the atomic_exchange_rel macro should 
be redefined for s390.



I think removing the custom implementation as most as possible is the 
right way. Thus IÂve changed your patch. Now the s390 - lowlevellock.h 
does only include sysdeps/nptl/lowlevellock.h and defines the macros for 
lock-elision. The atomic_exchange macro is defined for s390 in atomic.h.
The testsuite runs without regression on s390-32/s390-64 with/without 
--enable-lock-elision.

Ok to commit?

Thanks.

Bye
Stefan

---
2015-01-20  Stefan Liebler  <stli@linux.vnet.ibm.com>

	* sysdeps/unix/sysv/linux/s390/lowlevellock.h: Include
	<sysdeps/nptl/lowlevellock.h> and remove macros and
	functions that are now defined there.
	(SYS_futex): Remove.
	(lll_compare_and_swap): Remove.
	* sysdeps/s390/bits/atomic.h (atomic_exchange_acq): Define.

Attachment: glibc_lll_atomic_20012015
Description: Text document