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.