On Fri, Jul 13, 2012 at 03:44:03PM -0600, Eric Blake wrote:
> 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?
No, I meant Win32 impl - both codebases have a GCC impl - I was
pointing out that we gain a Win32 impl, which is the only non-GCC
scenario we hugely care about.
>
> > 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.
Hmm, I wonder why I didn't see HAVE_PTHREAD_H in config.h before. I'll
investigate.
>
> > +
> > +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.
Ok, I'll separate this out.
>
> > @@ -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?
Yeah I think so.
>
> >
> > - 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)? ...
IntInc is void(), so I had to use IntAdd. That said this code is
not too pleasant so I'll check into this.
>
> > -# 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.
The reason for returning the OLD value is that this is the only thing
you can really work with, race free. If you are trying to do something
based on the new value then your code is quitely probably racy, since
any other thread can have further changed the new value at any time.
>
> > +/**
> > + * 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>?
Opps, yes.
> > +
> > +# 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?
With the inline GCC macros we can pass either 'int' or 'unsigned int'
with no warnings. When we use the WIn32/pthread helper functions though,
we would get compiler warnings from mixed-sign ints. These macro casts
get around that.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list