From owner-svn-src-all@FreeBSD.ORG Sat Feb 20 18:24:27 2010 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id AE84D106566C; Sat, 20 Feb 2010 18:24:27 +0000 (UTC) (envelope-from nwhitehorn@freebsd.org) Received: from agogare.doit.wisc.edu (agogare.doit.wisc.edu [144.92.197.211]) by mx1.freebsd.org (Postfix) with ESMTP id 7BB9C8FC17; Sat, 20 Feb 2010 18:24:27 +0000 (UTC) MIME-version: 1.0 Content-transfer-encoding: 7BIT Content-type: text/plain; CHARSET=US-ASCII; format=flowed Received: from avs-daemon.smtpauth2.wiscmail.wisc.edu by smtpauth2.wiscmail.wisc.edu (Sun Java(tm) System Messaging Server 7u2-7.05 32bit (built Jul 30 2009)) id <0KY500F00KGQVB00@smtpauth2.wiscmail.wisc.edu>; Sat, 20 Feb 2010 12:24:26 -0600 (CST) Received: from comporellon.tachypleus.net (adsl-76-233-146-74.dsl.mdsnwi.sbcglobal.net [76.233.146.74]) by smtpauth2.wiscmail.wisc.edu (Sun Java(tm) System Messaging Server 7u2-7.05 32bit (built Jul 30 2009)) with ESMTPSA id <0KY5006A9KGOGA30@smtpauth2.wiscmail.wisc.edu>; Sat, 20 Feb 2010 12:24:25 -0600 (CST) Date: Sat, 20 Feb 2010 12:24:24 -0600 From: Nathan Whitehorn In-reply-to: To: Rafal Jaworowski Message-id: <4B8028D8.7090402@freebsd.org> X-Spam-Report: AuthenticatedSender=yes, SenderIP=76.233.146.74 X-Spam-PmxInfo: Server=avs-9, Version=5.5.5.374460, Antispam-Engine: 2.7.1.369594, Antispam-Data: 2010.2.20.180925, SenderIP=76.233.146.74 References: <201002201613.o1KGDiiK053065@svn.freebsd.org> User-Agent: Thunderbird 2.0.0.23 (X11/20100206) Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r204126 - head/sys/powerpc/booke X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 20 Feb 2010 18:24:27 -0000 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