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

[Torvald Riegel <triegel at redhat dot com>]

On Tue, 2015-01-20 at 12:20 +0100, Stefan Liebler wrote:
> 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?

Looks good to me.  (However, I haven't checked the atomic_exchange_acq
assembly).

Eventually, I would hope that we can have a more generic way to support
elision than more or less duplicating code in lowlevellock.h -- but
that's something for a later time.