This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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



On 09/01/2015 02:38 PM, Peter Bergner wrote:
> ...although looking closer, I see have the definitions we use for
> passing in failure codes is:
> 
> /* Definitions used for TEXASR Failure code (bits 0:6), they need to be even
>    because tabort. always sets the first bit.  */

This is a bad comment. This is a misinterpretation of the ISA, as you've noted,
bit 31 is always set if tabort* is used. This makes using the conditional
versions impractical within the TLE implementation.

I think the implication here is you can't trust bit 7 of texasr to deduce the
abort code, as it could be set due to one of bits 8-11. I'm not sure if the cases
leading bits 8-11 to get set are mutually exclusive with calling tabort. I.e is
failure code 1 the only reserved code?

> #define _ABORT_LOCK_BUSY       0x3f   /* Lock already used.  */
> #define _ABORT_NESTED_TRYLOCK  0x3e   /* Write operation in trylock.  */

This should definitely be an odd value too. I'll see to fixing its usage. As
implemented now, we might retry 3 (try_tbegin) times in __lll_trylock_elision
if we hit that one.

> #define _ABORT_SYSCALL         0x3d   /* Syscall issued.  */
> 
> I'm not sure I understand the "even" part above, since two of these
> values are clearly not even.  In fact, the two odd values above if
> passed to __builtin_tabort() will lead to the persistent bit being
> set, since tabort. sets the upper part of the TEXASR by taking the
> least significant 8 bits of the value you passed in and concatenating
> 0x000001 to the end of it, so essentially (using _ABORT_LOCK_BUSY as
> a failure cause example):
> 
>    TEXASR(0:31) = ((_ABORT_LOCK_BUSY & 0xff) << 24) | 0x1;
> 
> Given the persistent bit is bit 7, that means any odd value we pass
> into __builtin_tabort(XXX) will set the TEXASR persistent bit.
> Did we mean to do that?  Maybe we did, I'm just asking. :-)
> I can see that if we're about to do a syscall, that should be persistent.
> Are we also treating someone having a lock as persistent too?
> 
> If we did, then using __builtin_tabortwci() won't work, since we
> wouldn't be setting the persistent bit like we are now... unless
> we can pass in is_lock_free to __get_new_count and test that
> along with the persistent bit still.
> 
> Peter
 


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]