From owner-svn-src-all@freebsd.org Wed Feb 1 11:13:07 2017 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id D2D92CCA5EB; Wed, 1 Feb 2017 11:13:07 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail109.syd.optusnet.com.au (mail109.syd.optusnet.com.au [211.29.132.80]) by mx1.freebsd.org (Postfix) with ESMTP id 0A0BC830; Wed, 1 Feb 2017 11:13:06 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from besplex.bde.org (c122-106-153-191.carlnfd1.nsw.optusnet.com.au [122.106.153.191]) by mail109.syd.optusnet.com.au (Postfix) with ESMTPS id ABB43D6424E; Wed, 1 Feb 2017 22:12:58 +1100 (AEDT) Date: Wed, 1 Feb 2017 22:12:57 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Bruce Evans cc: Mateusz Guzik , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r312975 - head/sys/i386/include In-Reply-To: <20170130142123.V953@besplex.bde.org> Message-ID: <20170201214349.H1136@besplex.bde.org> References: <201701300224.v0U2Osj1010421@repo.freebsd.org> <20170130142123.V953@besplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=BKLDlBYG c=1 sm=1 tr=0 a=Tj3pCpwHnMupdyZSltBt7Q==:117 a=Tj3pCpwHnMupdyZSltBt7Q==:17 a=kj9zAlcOel0A:10 a=c6f6VvGJeq85lbJT2RsA:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.23 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: Wed, 01 Feb 2017 11:13:07 -0000 On Mon, 30 Jan 2017, Bruce Evans wrote: > On Mon, 30 Jan 2017, Mateusz Guzik wrote: > >> Log: >> i386: add atomic_fcmpset >> >> Tested by: pho > > This is has some bugs and style bugs. This is still broken. The invalid asm breaks building at least atomic.o with gcc-4.2.1. Tested fix: X Index: i386/include/atomic.h X =================================================================== X --- i386/include/atomic.h (revision 313007) X +++ i386/include/atomic.h (working copy) X @@ -225,9 +225,9 @@ X " cmpxchgl %3,%1 ; " X " sete %0 ; " X "# atomic_cmpset_int" X - : "=r" (res), /* 0 */ X + : "=q" (res), /* 0 */ X "+m" (*dst), /* 1 */ X - "+a" (*expect) /* 2 */ X + "+a" (*expect) /* 2 */ X : "r" (src) /* 3 */ X : "memory", "cc"); X return (res); The semantics of fcmpset seem to be undocumented. On x86, *expect is updated non-atomically by a store in the output parameter. I think cmpxchg updates the "a" register atomically, but then the output parameter causes this to be stored non-atomically to *expect. A better API would somehow return the "a" register and let the caller store it if it wants. Ordinary cmpset can be built on this by not storing, and the caller can do the store atomically to a different place if *expect is too volatile to be atomic. Maybe just decouple the input parameter from the output parameter. The following works right (for an amd64 API): Y static __inline int Y atomic_xfcmpset_long(volatile u_long *dst, u_long *expect_out, u_long expect_in, Y u_long src) Y { Y u_long expect; Y u_char res; Y Y expect = expect_in; Y __asm __volatile( Y " " MPLOCKED " " Y " cmpxchgq %3,%1 ; " Y " sete %0 ; " Y "# atomic_fcmpset_long" Y : "=r" (res), /* 0 */ Y "+m" (*dst), /* 1 */ Y "+a" (expect) /* 2 */ Y : "r" (src) /* 3 */ Y : "memory", "cc"); Y *expect_out = expect; If the caller doesn't want to use *expect_out, it passes a pointer to an unused local variable. The compiler can then optimize away the store since it is not hidden in the asm. Y return (res); Y } Bruce