On Tue, Jul 31, 2012 at 06:09:13PM -0600, Eric Blake wrote:
> 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?
I don't think it really matters in the context of this test
> > + 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.
I believe all common distros have GCC builds default to at least i486
these days, precisely to get atomic op support. So don't reckon we
need todo anything here.
>
> > @@ -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.
Opps, yes, will add.
> > 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.
The main reason is to be able to get the static initializer,
which we don't support, since there's no easy way to achieve
it on Win32. (Mingw pthreads does some horrible hacks to
try, which I didn't fancy doing)
> > +++ 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.
My justification for this is that I deleted the existing
file entirely, and copied in the new file from GLib.
So any matching lines between old & new are just
co-incidence
> > +/**
> > + * 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.
I figure we can change this in the future if it proves to be
needed.
> > +/**
> > + * 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.
It is annoying, but this lets us get optimal performance on Win32.
If I made them consistent, then we'd be stuck with one of the Win32
impls being relatively inefficient :-(
> > +++ 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)
Opps, yes.
> > + 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);
Good suggestions
> 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.
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