svn commit: r204126 - head/sys/powerpc/booke

[Nathan Whitehorn]

Rafal Jaworowski wrote:
> On 2010-02-20, at 17:13, Nathan Whitehorn wrote:
>
>   
>> Author: nwhitehorn
>> Date: Sat Feb 20 16:13:43 2010
>> New Revision: 204126
>> URL: http://svn.freebsd.org/changeset/base/204126
>>
>> Log:
>>  Merge r198724 to Book-E. casuword() non-atomically read the current value
>>  of its argument before atomically replacing it, which could occasionally
>>  return the wrong value on an SMP system. This resulted in user mutex
>>  operations hanging when using threaded applications.
>>     
>
> Have you got a particular test case when this was breaking, so I can test?
>   
This typically shows up with heavy lock contention on umtx operations. I 
discovered this because running csup died for me 100% of the time on my 
Xserve, by hanging forever in some umtx code.

This change explicitly preserves the semantics of casuword -- it is just 
the code for atomic_cmpset_32 copied from atomic.h, but returning the 
value loading with lwarx, instead of replacing it with a success code. 
This closes a race between val = *addr and atomic_cmpset. With the old 
code, another CPU could change the value at addr between val = *addr and 
atomic_cmpset, causing casuword to return the wrong value.
>> Modified:
>>  head/sys/powerpc/booke/copyinout.c
>>
>> Modified: head/sys/powerpc/booke/copyinout.c
>> ==============================================================================
>> --- head/sys/powerpc/booke/copyinout.c	Sat Feb 20 16:12:37 2010	(r204125)
>> +++ head/sys/powerpc/booke/copyinout.c	Sat Feb 20 16:13:43 2010	(r204126)
>> @@ -295,8 +295,19 @@ casuword(volatile u_long *addr, u_long o
>> 		return (EFAULT);
>> 	}
>>
>> -	val = *addr;
>> -	(void) atomic_cmpset_32((volatile uint32_t *)addr, old, new);
>> +	__asm __volatile (
>> +		"1:\tlwarx %0, 0, %2\n\t"	/* load old value */
>> +		"cmplw %3, %0\n\t"		/* compare */
>> +		"bne 2f\n\t"			/* exit if not equal */
>> +		"stwcx. %4, 0, %2\n\t"      	/* attempt to store */
>> +		"bne- 1b\n\t"			/* spin if failed */
>> +		"b 3f\n\t"			/* we've succeeded */
>> +		"2:\n\t"
>> +		"stwcx. %0, 0, %2\n\t"       	/* clear reservation (74xx) */
>>     
>
> The 74xx comment reference is somewhat confusing as the clear reservation operation is pretty uniform accross 32-bit PowerPC I guess, and not 74xx specific.
>   
That's true. It should also be updated in atomic.h.
-Nathan