Re: [PATCHv3] PowerPC: Fix a race condition when eliding a lock

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

On 01-09-2015 18:24, Paul E. Murphy wrote:
> 
> 
> On 09/01/2015 03:46 PM, Adhemerval Zanella wrote:
>> Indeed the 'odd' comment does not make sense and we should just remove it
>> (I misread texasr definition).  My initial idea was define some codes
>> that set the persistent failures and some that do not.  I think I best
>> approach would be:
>>
>> /* tabort will set TEXASR(0:31) = ((_ABORT_LOCK_BUSY & 0xff) << 24) | 0x1
>>    and the TEXASR persistent bit is bit 25 (32-7).  Only the syscall
>>    code means a persistent error that should trigger a default lock
>>    acquisition.  */
>> #define _ABORT_SYSCALL         0x1   /* Syscall issued.  */
>> #define _ABORT_LOCK_BUSY       0x2   /* Lock already used.  */
>> #define _ABORT_NESTED_TRYLOCK  0x4   /* Write operation in trylock.  */
> 
> The kernel defines several abort codes, we'll want to work with them, or
> recycle them as needed.

Yeap, but these are the one kernel itself generates (not GLIBC) and I think
it would be useful to use different code from abort generated from
GLIBC itself (so an external tool like perf can detect from where exactly
the abort came from).

And I used the number above just an example, checking the kernel code seems it
defines:

#define TM_CAUSE_PERSISTENT     0x01
#define TM_CAUSE_KVM_RESCHED    0xe0  /* From PAPR */
#define TM_CAUSE_KVM_FAC_UNAV   0xe2  /* From PAPR */
#define TM_CAUSE_RESCHED        0xde
#define TM_CAUSE_TLBI           0xdc
#define TM_CAUSE_FAC_UNAV       0xda
#define TM_CAUSE_SYSCALL        0xd8
#define TM_CAUSE_MISC           0xd6  /* future use */
#define TM_CAUSE_SIGNAL         0xd4
#define TM_CAUSE_ALIGNMENT      0xd2
#define TM_CAUSE_EMULATE        0xd0

So I think we might use 0xaX or something for the GLIBC ones.


> 
> I'm not convinced any of the existing codes should be non-persistent:
> 
> A pthread_mutex_trylock attempt within an elided pthread_mutex_lock is
> guaranteed to fail try_tbegin times if there is no contention on the lock.
> Aborts get increasingly expensive as you increase the amount of speculative
> execution.
> 
> A busy lock likely indicates contention in the critical section which
> does not benefit from elision, I'd err on the side of a persistent
> failure.
> 

I do not that much information to decide it, although I do see for pthread_mutex_t 
at least the _ABORT_LOCK_BUSY is not persistent if the critical region does not 
generate enough contention.