On 07/31/2012 10:58 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> +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.
Reviewing the next part out of order, so as to hit implementation before
client's use of the implementation.
> 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.
> +/**
> + * virAtomicIntDecAndTest:
> + * Decrements the value of atomic by 1.
> + *
> + * Think of this operation as an atomic version of
> + * { *atomic -= 1; return *atomic == 0; }
I guess this is another case of add then fetch, more like virAtomicIntInc.
> +/**
> + * virAtomicIntAdd:
> + * Atomically adds val to the value of atomic.
> + *
> + * Think of this operation as an atomic version of
> + * { tmp = *atomic; *atomic += val; return tmp; }
> + *
> + * This call acts as a full compiler and hardware memory barrier.
> + */
> +int virAtomicIntAdd(volatile int *atomic,
> + int val)
> ATTRIBUTE_NONNULL(1);
> -static inline int virAtomicIntSub(virAtomicIntPtr vaip, int add)
> +
> +/**
> + * virAtomicIntAnd:
> + * Performs an atomic bitwise 'and' of the value of atomic
> + * and val, storing the result back in atomic.
> + *
> + * This call acts as a full compiler and hardware memory barrier.
> + *
> + * Think of this operation as an atomic version of
> + * { tmp = *atomic; *atomic &= val; return tmp; }
I guess the other thing to consider is that with virAtomicIntAnd, you
have no way to reconstruct the original value based solely on what you
and'ed in, but given the original value, you can reconstruct what must
have been assigned. With virAtomicIntInc and virAtomicIntAdd, you can
reconstruct either value; so it boils down to a question of which value
will be more useful (the original or the modified), as well as a
symmetry (virAtomicIntAdd is more like virAtomicIntAnd on doing
arbitrary operations, while virAtomicIntInc is limited to a change of 1).
> +# ifdef VIR_ATOMIC_OPS_GCC
> +
> +# define virAtomicIntGet(atomic) \
> + (__extension__ ({ \
> + (void)verify_true(sizeof(*(atomic)) == sizeof(int)); \
> + (void)(0 ? *(atomic) ^ *(atomic) : 0); \
The gnulib documentation actually recommends using verify_expr() these
days rather than verify_true, as in:
(void)verify_true(sizeof(*(atomic)) == sizeof(int), \
0 ? *(atomic) ^ *(atomic) : 0); \
since in some situations, verify_expr() can give better diagnostics.
> +inline int
> +virAtomicIntInc(volatile int *atomic)
> +{
> + return InterlockedIncrement((volatile LONG *)atomic);
> +}
I checked that this value does indeed return the new value...
> +inline int
> +virAtomicIntAdd(volatile int *atomic,
> + int val)
> +{
> + return InterlockedExchangeAdd((volatile LONG *)atomic, val);
...and this returns the old value, so at least you are consistent to
your documentation on this implementation.
Yay - this header looks sound. Now, back to the rest of the commit.
> @@ -1994,9 +1992,9 @@ virNWFilterSnoopLeaseFileLoad(void)
> static void
> virNWFilterSnoopJoinThreads(void)
> {
> - while (virAtomicIntRead(&virNWFilterSnoopState.nThreads) != 0) {
> + while (virAtomicIntGet(&virNWFilterSnoopState.nThreads) != 0) {
> VIR_WARN("Waiting for snooping threads to terminate: %u\n",
> - virAtomicIntRead(&virNWFilterSnoopState.nThreads));
> + virAtomicIntGet(&virNWFilterSnoopState.nThreads));
Pre-existing, but this loop could print the wrong value in the VIR_WARN
statement. It would be better to assign the condition of the loop to a
temporary, then print that temporary, so the output won't be confusing.
> +++ 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)
> +++ b/tests/viratomictest.c
> +static int
> +testTypes(const void *data ATTRIBUTE_UNUSED)
> +{
> + unsigned int u, u2;
> + int s, s2;
> + bool res;
> +
> +#define virAssertCmpInt(a, op, b) \
> + if (!((a) op (b))) \
> + return -1;
> + virAtomicIntSet(&u, 5);
> + u2 = virAtomicIntGet(&u);
> + virAssertCmpInt(u2, ==, 5);
> +
> + res = virAtomicIntCompareExchange(&u, 6, 7);
> + if (res)
> + return -1;
> + 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.
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list