From owner-svn-src-head@freebsd.org Mon Jan 30 04:45:10 2017 Return-Path: Delivered-To: svn-src-head@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 4B583CC7449; Mon, 30 Jan 2017 04:45:10 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail110.syd.optusnet.com.au (mail110.syd.optusnet.com.au [211.29.132.97]) by mx1.freebsd.org (Postfix) with ESMTP id 60CDA1733; Mon, 30 Jan 2017 04:45:09 +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 mail110.syd.optusnet.com.au (Postfix) with ESMTPS id A3D3310318C; Mon, 30 Jan 2017 15:44:59 +1100 (AEDT) Date: Mon, 30 Jan 2017 15:44:59 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Mateusz Guzik cc: 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: <201701300224.v0U2Osj1010421@repo.freebsd.org> Message-ID: <20170130142123.V953@besplex.bde.org> References: <201701300224.v0U2Osj1010421@repo.freebsd.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=H7qr+6Qi c=1 sm=1 tr=0 a=Tj3pCpwHnMupdyZSltBt7Q==:117 a=Tj3pCpwHnMupdyZSltBt7Q==:17 a=kj9zAlcOel0A:10 a=RcezWy2ymVxIhtw7SZQA:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 30 Jan 2017 04:45:10 -0000 On Mon, 30 Jan 2017, Mateusz Guzik wrote: > Log: > i386: add atomic_fcmpset > > Tested by: pho This is has some bugs and style bugs. > Modified: > head/sys/i386/include/atomic.h > > Modified: head/sys/i386/include/atomic.h > ============================================================================== > --- head/sys/i386/include/atomic.h Mon Jan 30 02:21:29 2017 (r312974) > +++ head/sys/i386/include/atomic.h Mon Jan 30 02:24:54 2017 (r312975) > @@ -214,6 +215,24 @@ atomic_cmpset_int(volatile u_int *dst, u > return (res); > } > > +static __inline int > +atomic_fcmpset_int(volatile u_int *dst, u_int *expect, u_int src) This is unusable except under SMP or CPU_DISABLE_CMPXCHG ifdefs, since it is not defined if CPU_DISABLE_CMPXCHG is configured. CPU_DISABLE_CMPXCHG is still a supported user option if !SMP. According to NOTES, it is to support vmware emulating cmpxchg poorly. This function and its excessive aliases seems to be undocumented and not yet used, so I don't know what it is supposed to be used for or whether these uses are naturally restricted to the SMP case where the function is available. > +{ > + u_char res; > + > + __asm __volatile( > + " " MPLOCKED " " > + " cmpxchgl %3,%1 ; " > + " sete %0 ; " > + "# atomic_cmpset_int" > + : "=r" (res), /* 0 */ Invalid asm. sete is only valid for q registers, except in long mode on amd64. > + "+m" (*dst), /* 1 */ > + "+a" (*expect) /* 2 */ Style bug (inconsistent indentation). Bugs like this can be created by blind indentation. Cloning atomic_cmpset_int() and then s/expect/*expect/ to get the subtle difference between these function would have given the style bug here. But that wouldn't have given the invalid asm. The i386 atomic_cmpset_int() doesn't have the invalid asm, and the amd64 atomic_fcmpset_int() doesn't have the style bug. > + : "r" (src) /* 3 */ > + : "memory", "cc"); > + return (res); > +} > + > #endif /* CPU_DISABLE_CMPXCHG */ > > /* The other style bugs seem to be consistent with the rest of the file (API explosion, unsorted #define's in the explosion, and bogus casts in the "pointer" APIs; amd64 never had the latter, so MI code can't depend on the casts hiding type errors). Bruce