This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCHv3] PowerPC: Fix a race condition when eliding a lock
- From: "Paul E. Murphy" <murphyp at linux dot vnet dot ibm dot com>
- To: libc-alpha at sourceware dot org, Peter Bergner <bergner at vnet dot ibm dot com>, Steve Munroe <sjmunroe at us dot ibm dot com>
- Date: Tue, 01 Sep 2015 15:25:06 -0500
- Subject: Re: [PATCHv3] PowerPC: Fix a race condition when eliding a lock
- Authentication-results: sourceware.org; auth=none
- References: <55D742D3 dot 9050600 at redhat dot com> <1440439895-11812-1-git-send-email-tuliom at linux dot vnet dot ibm dot com> <1441136302 dot 5089 dot 182 dot camel at otta>
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