svn commit: r312975 - head/sys/i386/include

[Mateusz Guzik]

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:
> 
> 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);
> 

Uh, I ended up with the same fix. Committed in r313080.

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
}

> 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.
> 

_fcmpset is specifically for callers who want the value back. Ones which
don't can use the _cmpset variant.

-- 
Mateusz Guzik <mjguzik gmail.com>