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

On 07/31/2012 10:58 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 > though, we don't expect anyone to seriously use the pthread.h > impl, so this downside is not significant. > > * .gitignore: Ignore test case > * configure.ac: Check for which atomic ops impl to use > * src/Makefile.am: Add viratomic.c > * src/nwfilter/nwfilter_dhcpsnoop.c: Switch to new atomic > ops APIs and plain int datatype > * src/util/viratomic.h: inline impls of all atomic ops > for GCC, Win32 and pthreads > * src/util/viratomic.c: Global pthreads mutex for atomic > ops > * tests/viratomictest.c: Test validate to validate safety > of atomic ops. > > Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; > --- > +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;],[ The autoconf manual prefers 'AC_COMPILE_IFELSE' over 'AC_TRY_COMPILE', but as you are copying code, I won't worry if you don't change it. > + atomic_ops=gcc > +],[]) > + > +if test "$atomic_ops" = "" ; then > + SAVE_CFLAGS="${CFLAGS}" > + CFLAGS="-march=i486" Should this be: CFLAGS="$CFLAGS -march=i486" so as not to lose previous flags that might be important?

> + AC_TRY_COMPILE([], > + [__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4;], > + [AC_MSG_ERROR([Libvirt must be build with -march=i486 or later.])], s/build/built/ Okay - the intent here is that if we compiled and __GCC_HAVE_SYNC_COMPARE_AND_SWAP failed, then we add a new option, and the recompilation attempt succeeds, then we a) must be using gcc, b) gcc recognizes the option, so we must be x86 (all other scenarios either passed on the first compilation, or point to a different architecture and/or different compiler so both compile attempts failed). Since the test is so simple, discarding all existing CFLAGS to look for just -march=i486 probably does the job, and I guess my earlier comment is not strictly necessary. Do we need to worry about updating './autogen.sh' to add this CFLAGS value automatically, or is gcc typically spec'd to a new enough platform by default in x86 distros these days? Then again, I just tested the patch myself and didn't hit the warning, so at least Fedora 17 has gcc spec'd to implicitly assume that flag.

> @@ -1301,6 +1301,10 @@ if HAVE_SASL > USED_SYM_FILES += libvirt_sasl.syms > endif > > +if WITH_ATOMIC_OPS_PTHREAD > +USED_SYM_FILES += libvirt_atomic.syms > +endif > + > EXTRA_DIST += \ > libvirt_public.syms \ > libvirt_private.syms \ Incomplete. EXTRA_DIST also needs to list libvirt_atomic.syms.

> diff --git a/src/util/viratomic.c b/src/util/viratomic.c > new file mode 100644 > index 0000000..af846ff > --- /dev/null > +++ b/src/util/viratomic.c > @@ -0,0 +1,35 @@ > +/* > + > +#ifdef VIR_ATOMIC_OPS_PTHREAD > + > +pthread_mutex_t virAtomicLock = PTHREAD_MUTEX_INITIALIZER; I guess since we know we are using pthread, we can directly use pthread_mutex_t instead of our virThread wrappers in our thread.h.

> +++ b/src/util/viratomic.h > @@ -1,10 +1,11 @@ > /* > * viratomic.h: atomic integer operations > * > - * Copyright (C) 2012 IBM Corporation > + * Copyright (C) 2012 Red Hat, Inc. Normally, I question tossing out an old copyright. But this is such a massive rewrite that I think you're safe. By the way, the diff was horrible to read; I more or less ended up reviewing the final state of the code, rather than the diff.

> +/** > + * virAtomicIntSet: > + * Sets the value of atomic to newval. > + * > + * This call acts as a full compiler and hardware memory barrier > + * (after the set) > + */ > +void virAtomicIntSet(volatile int *atomic, > + int newval) > + ATTRIBUTE_NONNULL(1); Is it worth having this function return 'int' instead of 'void' to pass back the just-set value, similar to C semantics of 'a = b = c' returning the just-set value of 'b' when assigning to 'a'? Then again, you were able to do the conversion without it, so probably not worth changing.

> +/** > + * virAtomicIntInc: > + * Increments the value of atomic by 1. > + * > + * Think of this operation as an atomic version of > + * { *atomic += 1; return *atomic; } Here we add then fetch; but with virAtomicIntAdd, we fetch then add. Is the difference going to bite us? At least it is documented, so I can review based on the documentation.

> +++ b/src/util/virfile.c > @@ -618,12 +618,11 @@ cleanup: > #else /* __linux__ */ > > int virFileLoopDeviceAssociate(const char *file, > - char **dev) > + char **dev ATTRIBUTE_UNUSED) > { > virReportSystemError(ENOSYS, > _("Unable to associate file %s with loop device"), > file); > - *dev = NULL; > return -1; > } This is a bogus hunk (conflict resolution gone wrong)

> + virAssertCmpInt(u, ==, 5); > + > + virAtomicIntAdd(&u, 1); slightly stronger test as: virAssertCmpInt(virAtomicIntAdd(&u, 1), == 5); > + virAssertCmpInt(u, ==, 6); > + > + virAtomicIntInc(&u); slightly stronger test as: virAssertCmpInt(virAtomicIntInc(&u), ==, 7);

ACK once you fix src/Makefile.am and the configure.ac typo, and omit the virfile.c hunk. The rest of my comments aren't show-stoppers, so feel free to fix or ignore as suits you.