Re: [PATCH] powerpc: Only enable TLE with PPC_FEATURE2_HTM_NOSC

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 19/09/2018 16:20, Tulio Magno Quites Machado Filho wrote:
> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
> 
>> On 27/08/2018 19:44, Tulio Magno Quites Machado Filho wrote:
>>> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
>>>
>>>> diff --git a/sysdeps/unix/sysv/linux/powerpc/elision-conf.c b/sysdeps/unix/sysv/linux/powerpc/elision-conf.c
>>>> index 906882a..508b917 100644
>>>> --- a/sysdeps/unix/sysv/linux/powerpc/elision-conf.c
>>>> +++ b/sysdeps/unix/sysv/linux/powerpc/elision-conf.c
>>>> @@ -127,6 +127,30 @@ elision_init (int argc __attribute__ ((unused)),
>>>>  	       TUNABLE_CALLBACK (set_elision_skip_trylock_internal_abort));
>>>>  #endif
>>>>  
>>>> +  /* Linux from 3.9 through 4.2 does not abort HTM transaction on syscalls,
>>
>> In fact it should be 4.1, 4.2 is the first release which actually abort transactions
>> on syscalls.
> 
> Ack.
> 
>>>> +     instead it suspend and resume it when leaving the kernel.  The
>>>> +     side-effects of the syscall will always remain visible, even if the
>>>> +     transaction is aborted.  This is an issue when transaction is used along
>>>> +     with futex syscall, on pthread_cond_wait for instance, where the futex
>>>> +     call might succeed but the transaction is rolled back leading the
>>>> +     pthread_cond object in an inconsistent state.
>>>> +
>>>> +     GLIBC used to prevent it by always aborting a transaction before issuing
>>>> +     a syscall.  Linux 4.2 also decided to abort active transaction in
>>>> +     syscalls which makes the GLIBC workaround superflours.  Worse, GLIBC
>>>> +     transaction abortion leads to a performance issue on recent kernels
>>>> +     where the HTM state is saved/restore lazily.  By aborting a transaction
>>>> +     on every syscalls, regardless whether a transaction has being initiated
>>>> +     before, glibc make the kernel always save/restore HTM state (it can not
>>>> +     even lazily disable it after a certain number of syscall iterations).
>>>> +
>>>> +     Because of this shortcoming, Lock Elision is just enabled when it has
>>>> +     been explictly set (either by tunables of by a configure switch) and if
>>>> +     kernel aborts HTM transactions on syscalls (PPC_FEATURE2_HTM_NOSC)  */
>>>> +
>>>> +  __pthread_force_elision = __pthread_force_elision &&
>>>> +			    GLRO (dl_hwcap2) & PPC_FEATURE2_HTM_NOSC;
>>>
>>> In other words: this patch is requiring Linux 4.3 for TLE.
>>>
>>> Can't ABORT_TRANSACTION check for PPC_FEATURE2_HTM_NOSC in tcbhead_t.hwcap and
>>> continue to support older Linux versions?
>>>
>>
>> I don't have a strong opinion here, but chatting with Breno I had the impression
>> that TLE (and HTM in general) is only used on kernel with this recent fix. This
>> patch is really a hack insertion that penalizes all syscall for an specific
>> feature which may not be present in the kernel. It is unfortunate that kernel
>> developers did not realize at feature inclusion this design for transaction 
>> state is not the best one for library usage.
> 
> My point was actually related to the fact that INSTALL still mentions Linux 3.2.
> 
> I completely agree this solution is better in terms of performance and
> maintenance in the long term.
> Maybe we (me?) could document this problem somewhere (INSTALL?) pointing
> users that want to run the latest glibc and use HTM on Linux to use
> Linux >= 4.2.

The INSTALL file still lacks to mention that powerpc64le still requires
at least a 3.10 kernel (which is the minimum version where the ABI is
actually supported). And there still some other ABIs that does not
have support for 3.2 (aarch64 for instance).

At least, I think we start to document powerpc64le requirements: a minimum
Linux version of 3.10 and 4.2 for TLE.

> 
> With the removal of ABORT_TRANSACTION, it's also necessary to remove
> sysdeps/unix/sysv/linux/powerpc/not-errno.h.
> 
> With that said, my questions have already been answered and this patch looks
> good to me with the fix in the comment you mentioned and the removal of
> not-errno.h.> 

I will update the patch with this removal.