From owner-svn-src-all@freebsd.org Thu Feb 2 18:14:25 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 2206ACCE4B3; Thu, 2 Feb 2017 18:14:25 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail105.syd.optusnet.com.au (mail105.syd.optusnet.com.au [211.29.132.249]) by mx1.freebsd.org (Postfix) with ESMTP id 60BE0D54; Thu, 2 Feb 2017 18:14:24 +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 mail105.syd.optusnet.com.au (Postfix) with ESMTPS id 67EE910481BA; Fri, 3 Feb 2017 05:14:16 +1100 (AEDT) Date: Fri, 3 Feb 2017 05:14:15 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Mateusz Guzik cc: Bruce Evans , 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: <20170202014204.GA992@dft-labs.eu> Message-ID: <20170203040751.I2354@besplex.bde.org> References: <201701300224.v0U2Osj1010421@repo.freebsd.org> <20170130142123.V953@besplex.bde.org> <20170201214349.H1136@besplex.bde.org> <20170202014204.GA992@dft-labs.eu> 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=ZHXAibqBrz8wDKC0GT4A: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: Thu, 02 Feb 2017 18:14:25 -0000 On Thu, 2 Feb 2017, Mateusz Guzik wrote: > On Wed, Feb 01, 2017 at 10:12:57PM +1100, Bruce Evans wrote: >> 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: >> ... > > Uh, I ended up with the same fix. Committed in r313080. Thanks. > Sorry for breakage in the first place. > >> 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. >> > > The primitive was modeled after atomic_compare_exchange_* from c11 > atomics. I don't see what's the benefit of storing the result > separately. > > As it is, the primitive fits nicely into loops like "inc not zero". > > Like this: > r = *counter; > for (;;) { > if (r == 0) > break; > if (atomic_fcmpset_int(counter, &r, r + 1)) > break; > // r we can loop back to re-test r > } You won't want to ifdef this for SMP, so the i386 implementation has further bugs like I expected (fcmpset is not implemented in the CPU_DISABLE_CMPXCHG case). >> 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) The output parameter is not well named in this or in fcmpset, since when the comparison fails it holds the compared copy of *dst, not of *expect_in, and otherwise it is not useful and holds a copy of src. >> 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. > > _fcmpset is specifically for callers who want the value back. Ones which > don't can use the _cmpset variant. The main caller of _xfcmpset would be the _cmpset variantL static __inline int atomic_cmpset_int(volatile u_int *dst, u_int expect, u_int src) { u_int dummy __unused; return (atomic_xfcmpset_int(dst, &dummy, expect, src)); } Actually, _cmpset can be built out of _fcmpset even more easily: static __inline int atomic_cmpset_int(volatile u_int *dst, u_int expect, u_int src) { return (atomic_fcmpset_int(dst, &expect, src)); } This has to be a function since a macro would expose *&expect to clobbering. Bruce