On 07/11/2012 07:35 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
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?
> +++ b/src/util/virfile.c
> @@ -622,12 +622,12 @@ 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);
> - return -1;m
> + return -1;
> }
This hunk is stale (commit 56f34e5).
> +++ b/tests/Makefile.am
> @@ -89,6 +89,7 @@ test_programs = virshtest sockettest \
> nodeinfotest virbuftest \
> commandtest seclabeltest \
> virhashtest virnetmessagetest virnetsockettest \
> + viratomictest \
TAB vs. space.
> +static void
> +thread_func(void *data)
> +{
> + int idx = (int)(long)data;
> + int i;
> + int d;
> +
> + for(i = 0; i < ROUNDS; i++) {
Space after 'for'
> + d = virRandomBits(7);
> + bucket[idx] += d;
> + virAtomicIntAdd(&atomic, d);
> +#ifdef WIN32
> + SleepEx (0,0);
No space after SleepEx.
> +static int
> +testThreads(const void *data ATTRIBUTE_UNUSED)
> +{
> + int sum;
> + int i;
Make this a long...
> + virThread threads[THREADS];
> +
> + atomic = 0;
> + for(i = 0; i < THREADS; i++)
> + bucket[i] = 0;
> +
> + for(i = 0; i < THREADS; i++) {
Space after 'for' (twice)
> + if (virThreadCreate(&(threads[i]), true, thread_func, (void*)(long)i) < 0)
...and you can avoid one of the casts here.
> + return -1;
> + }
> +
> + for(i = 0; i < THREADS; i++)
> + virThreadJoin(&threads[i]);
> +
> + sum = 0;
> + for(i = 0; i < THREADS; i++)
(twice more)
Enough problems that I'll need to see a v2.
--
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