Re: [PATCH 3/3] PowerPC: abort transaction in syscalls

[Adhemerval Zanella <azanella at linux dot vnet dot ibm dot com>]

Hi Will,

As previously messages, I will send an updated patchset soon. 


On 10-12-2014 18:20, Will Schmidt wrote:
> On Thu, 2014-11-27 at 15:36 -0200, Adhemerval Zanella wrote:
>> On 07-11-2014 15:13, Adhemerval Zanella wrote:
>>> Linux kernel powerpc documentation states issuing a syscall inside a
>>> transaction is not recommended and may lead to undefined behavior. It
>>> also states syscalls does not abort transactoin neither they run in
>>> transactional state.
> A few cosmetics, and questions sprinkled throughout.  No issues with
> code functionality.
>
>
>>> To avoid side-effects being visible outside transactions, GLIBC with lock
>>> elision enable issues a transaction abort instruction just before all
> s/enable issues/enabled will issue/
Fixed.
>
>
>>> syscalls if hardware supports hardware transactions.
>>> --
>>>
>>> 	* sysdeps/powerpc/nptl/tls.h (tcbhead_t): Add tm_capable field.
>>> 	(TLS_INIT_TP): Add tm_capable initialization.
>>> 	(TLS_DEFINE_INIT_TP): Likewise.
>>> 	(THREAD_GET_TM_CAPABLE): New file: get tm_capable field value from
>>> 	TCB.
>>> 	(THREAD_SET_TM_CAPABLE): New file: set tm_capable field value in TCB.
>>> 	* sysdeps/powerpc/nptl/tcb-offsets.sym (TM_CAPABLE): Add field offset
>>> 	calculation.
>>> 	* sysdeps/powerpc/powerpc32/sysdep.h (DO_CALL): Abort hardware
>>> 	transactoion is lock elision is built and TCB tm_capable is set.
>>> 	* sysdeps/powerpc/powerpc64/sysdep.h (DO_CALL): Likewise.
>>> 	* sysdeps/unix/sysv/linux/powerpc/powerpc32/sysdep.h
>>> 	(INTERNAL_SYSCALL_NCS): Likewise.
>>> 	* sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h
>>> 	(INTERNAL_SYSCALL_NCS): Likewise.
>>> 	* sysdeps/powerpc/sysdep.h (ABORT_TRANSACTION): New define.
>>>
>>> ---
>>>
>>> diff --git a/sysdeps/powerpc/nptl/tcb-offsets.sym b/sysdeps/powerpc/nptl/tcb-offsets.sym
>>> index f996759..d955142 100644
>>> --- a/sysdeps/powerpc/nptl/tcb-offsets.sym
>>> +++ b/sysdeps/powerpc/nptl/tcb-offsets.sym
>>> @@ -19,6 +19,7 @@ POINTER_GUARD			(offsetof (tcbhead_t, pointer_guard) - TLS_TCB_OFFSET - sizeof (
>>>  TAR_SAVE			(offsetof (tcbhead_t, tar_save) - TLS_TCB_OFFSET - sizeof (tcbhead_t))
>>>  DSO_SLOT1			(offsetof (tcbhead_t, dso_slot1) - TLS_TCB_OFFSET - sizeof (tcbhead_t))
>>>  DSO_SLOT2			(offsetof (tcbhead_t, dso_slot2) - TLS_TCB_OFFSET - sizeof (tcbhead_t))
>>> +TM_CAPABLE			(offsetof (tcbhead_t, tm_capable) - TLS_TCB_OFFSET - sizeof (tcbhead_t))
>>>  #ifndef __ASSUME_PRIVATE_FUTEX
>>>  PRIVATE_FUTEX_OFFSET		thread_offsetof (header.private_futex)
>>>  #endif
>>> diff --git a/sysdeps/powerpc/nptl/tls.h b/sysdeps/powerpc/nptl/tls.h
>>> index b80a5fb..37280d2 100644
>>> --- a/sysdeps/powerpc/nptl/tls.h
>>> +++ b/sysdeps/powerpc/nptl/tls.h
>>> @@ -63,6 +63,8 @@ typedef union dtv
>>>     are private.  */
>>>  typedef struct
>>>  {
>>> +  /* Indicate if hwcap2 has PPC_FEATURE2_HAS_HTM capability.  */
>>> +  int tm_capable;
> The comment here is possibly a bit verbose.  This field doesn't need to
> care where/or how we determine the value.   How about just "Indicate if
> HTM capable".
Changed.
>>
>> diff --git a/sysdeps/powerpc/sysdep.h b/sysdeps/powerpc/sysdep.h
>> index e6627c0..c683066 100644
>> --- a/sysdeps/powerpc/sysdep.h
>> +++ b/sysdeps/powerpc/sysdep.h
>> @@ -21,6 +21,10 @@
>>   */
>>  #define _SYSDEPS_SYSDEP_H 1
>>  #include <bits/hwcap.h>
>> +#ifdef ENABLE_LOCK_ELISION
>> +#include <tls.h>
>> +#include <htm.h>
>> +#endif
>>
>>  #define PPC_FEATURE_970 (PPC_FEATURE_POWER4 + PPC_FEATURE_HAS_ALTIVEC)
>>
>> @@ -164,4 +168,21 @@
>>  #define ALIGNARG(log2) log2
>>  #define ASM_SIZE_DIRECTIVE(name) .size name,.-name
>>
>> +#else
>> +
>> +/* Linux kernel powerpc documentation states issuing a syscall inside a
>> +   transaction is not recommended and may lead to undefined behavior. It
> pointer to documentation?
>
>>> +   also states syscalls does not abort transactoin neither run in
> transaction
> 'neither' is possibly out of place.
>
>>> +   transactional state.  To avoid such traps, we abort transaction just
>>> +   before syscalls.  */

Right, I will change to :

/* Linux kernel powerpc documentation [1] states issuing a syscall inside a
   transaction is not recommended and may lead to undefined behavior.  It
   also states syscalls do not abort transactions.  To avoid such traps,
   we abort transaction just before syscalls. 

   [1] Documentation/powerpc/transactional_memory.txt [Syscalls]  */