Date: Sat, 20 Feb 2010 12:24:24 -0600 From: Nathan Whitehorn <nwhitehorn@freebsd.org> To: Rafal Jaworowski <raj@semihalf.com> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r204126 - head/sys/powerpc/booke Message-ID: <4B8028D8.7090402@freebsd.org> In-Reply-To: <BA32954C-41CF-4187-8633-C4A8DCBC4561@semihalf.com> References: <201002201613.o1KGDiiK053065@svn.freebsd.org> <BA32954C-41CF-4187-8633-C4A8DCBC4561@semihalf.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4B8028D8.7090402>