Re: [libvirt] [PATCH 03/13] Rewrite virAtomic APIs using GLib's atomic ops code

On 07/11/2012 07:35 AM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; > > There are a few issues with the current virAtomic APIs > > - They require use of a virAtomicInt struct instead of a plain > int type > - Several of the methods do not implement memory barriers > - The methods do not implement compiler re-ordering barriers > - There is no Win32 native impl > > The GLib library has a nice LGPLv2+ licensed impl of atomic > ops that works with GCC, Win32, or pthreads.h that addresses > all these problems. The main downside to their code is that > the pthreads impl uses a single global mutex, instead of > a per-variable mutex. Given that it does have a Win32 impl Don't you mean 'gcc impl' here?

> though, we don't expect anyone to seriously use the pthread.h > impl, so this downside is not significant. Agree that the majority of compilation is currently done with gcc, even though we try hard to allow other compilers. > +++ b/configure.ac > @@ -157,7 +157,64 @@ dnl Availability of various common headers (non-fatal if missing). > AC_CHECK_HEADERS([pwd.h paths.h regex.h sys/un.h \ > sys/poll.h syslog.h mntent.h net/ethernet.h linux/magic.h \ > sys/un.h sys/syscall.h netinet/tcp.h ifaddrs.h libtasn1.h \ > - net/if.h execinfo.h]) > + net/if.h execinfo.h pthread.h]) NACK to this change. We already check for pthread.h via gnulib.

> + > +dnl We need to decide at configure time if libvirt will use real atomic > +dnl operations ("lock free") or emulated ones with a mutex. > + > +dnl Note that the atomic ops are only available with GCC on x86 when > +dnl using -march=i486 or higher. If we detect that the atomic ops are > +dnl not available but would be available given the right flags, we want > +dnl to abort and advise the user to fix their CFLAGS. It's better to do > +dnl that then to silently fall back on emulated atomic ops just because > +dnl the user had the wrong build environment. > + > +atomic_ops= > + > +AC_MSG_CHECKING([for atomic ops implementation]) > + > +AC_TRY_COMPILE([], [__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4;],[ > + atomic_ops=gcc > +],[]) > + > +if test "$atomic_ops" = "" ; then > + SAVE_CFLAGS="${CFLAGS}" > + CFLAGS="-march=i486" > + AC_TRY_COMPILE([], > + [__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4;], > + [AC_MSG_ERROR([Libvirt must be build with -march=i486 or later.])], > + []) > + CFLAGS="${SAVE_CFLAGS}" > + > + case "$host" in > + *-*-mingw* | *-*-cygwin* | *-*-msvc* ) > + atomic_ops=win32 Okay for mingw and msvc, but looks wrong for cygwin. Cygwin prides itself in having the same interface as Linux, and mucking with win32 internals violates that. But that means you are inheriting a glib bug, not something you are introducing yourself. On the other hand, cygwin cannot be compiled unless you use gcc, so the extent of the glib bug is a dead arm of the case statement, and not something that will ever trigger in reality. I suppose I can live with it, on the grounds that it was copy and paste; but I'd really like it if we isolated this into a separate file under m4/virt-...m4, and attributed it back to the glib sources, so that if glib enhances their tests, then we can more easily stay in sync with those enhancements.

> @@ -714,7 +712,7 @@ virNWFilterSnoopReqPut(virNWFilterSnoopReqPtr req) > > virNWFilterSnoopLock(); > > - if (virAtomicIntDec(&req->refctr) == 0) { > + if (virAtomicIntDecAndTest(&req->refctr)) { The old code checked for 0, the new code checks for non-zero... > /* > * delete the request: > * - if we don't find req on the global list anymore > @@ -896,7 +894,7 @@ virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReqPtr req, > skip_instantiate: > VIR_FREE(ipl); > > - virAtomicIntDec(&virNWFilterSnoopState.nLeases); > + virAtomicIntDecAndTest(&virNWFilterSnoopState.nLeases); ...but here, the old and new code both test for non-zero. Did you introduce a logic bug?

> > - if (virAtomicIntInc(&virNWFilterSnoopState.wLeases) >= > - virAtomicIntRead(&virNWFilterSnoopState.nLeases) * 20) > + if (virAtomicIntAdd(&virNWFilterSnoopState.wLeases, 1) >= > + virAtomicIntGet(&virNWFilterSnoopState.nLeases) * 20) Did you mean to use virAtomicIntInc(&virNWFilterSnoopState.wLeases) instead of virAtomicIntAdd(&virNWFilterSnoopState.wLeases, 1)? ...

> -# if ((__GNUC__ == 4) && (__GNUC_MINOR__ >= 1)) || (__GNUC__ > 4) > -# undef __VIR_ATOMIC_USES_LOCK > -# endif > +/** > + * virAtomicIntInc: > + * Increments the value of atomic by 1. > + * > + * Think of this operation as an atomic version of { *atomic += 1; } > + * > + * This call acts as a full compiler and hardware memory barrier. > + */ > +void virAtomicIntInc(volatile int *atomic) > + ATTRIBUTE_NONNULL(1); ...Oh, I guess you can't, since this returns void instead of the old value. Then again, you have a subtle bug due to change of semantics. The old virAtomicIntAdd returns the NEW value; the new virAtomicIntAdd returns the OLD value. Since you are adding one, you have thus introduced an off-by-one. Or is this an instance where our existing code has the different logic under gcc than under pthread? Good thing you're adding a unit test for semantics, but that still means we need to inspect our usage for correct logic.

> +/** > + * virAtomicIntCompareExchange: > + * Compares atomic to oldval and, if equal, sets it to newval. If > + * atomic was not equal to oldval then no change occurs. > + * > + * This compare and exchange is done atomically. > + * > + * Think of this operation as an atomic version of > + * { if (*atomic == oldval) { *atomic = newval; return TRUE; } > + * else return FALSE; } Can we s/TRUE/true/ to match the fact that we use the real <stdbool.h>?

> + > +# define virAtomicIntGet(atomic) \ > + virAtomicIntGet((int *)atomic) What are these macro casts for? You are not properly parenthesizing things, and didn't the glib definition already check that the right size was being passed in?