Date: Mon, 30 Jan 2017 15:44:59 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Mateusz Guzik <mjg@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r312975 - head/sys/i386/include Message-ID: <20170130142123.V953@besplex.bde.org> In-Reply-To: <201701300224.v0U2Osj1010421@repo.freebsd.org> References: <201701300224.v0U2Osj1010421@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170130142123.V953>