[libvirt PATCH v2 09/13] util: replace atomic ops impls with g_atomic_int*

Libvirt's original atomic ops impls were largely copied from GLib's code at the time. The only API difference was that libvirt's virAtomicIntInc() would return a value, but g_atomic_int_inc was void. We thus use g_atomic_int_add(v, 1) instead, though this means virAtomicIntInc() now returns the original value, instead of the new value. This rewrites libvirt's impl in terms of g_atomic_int* as a short term conversion. The key motivation was to quickly eliminate use of GNULIB's verify_expr() macro which is not a direct match for G_STATIC_ASSERT_EXPR. Long term all the callers should be updated to use g_atomic_int* directly. Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com&gt; Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- src/libxl/libxl_domain.c | 2 +- src/libxl/libxl_driver.c | 2 +- src/lxc/lxc_process.c | 4 +- src/nwfilter/nwfilter_dhcpsnoop.c | 6 +- src/qemu/qemu_process.c | 4 +- src/util/viratomic.h | 351 ++---------------------------- src/util/virprocess.c | 2 +- tests/viratomictest.c | 2 +- 8 files changed, 26 insertions(+), 347 deletions(-) diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 915aaeb8b0..f9be4ad583 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -1473,7 +1473,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) goto destroy_dom; - if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback) + if (virAtomicIntInc(&driver->nactive) == 0 && driver->inhibitCallback) driver->inhibitCallback(true, driver->inhibitOpaque); /* finally we can call the 'started' hook script if any */ diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index f021ec9c5d..ef02a066d9 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -446,7 +446,7 @@ libxlReconnectDomain(virDomainObjPtr vm, virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_UNKNOWN); - if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback) + if (virAtomicIntInc(&driver->nactive) == 0 && driver->inhibitCallback) driver->inhibitCallback(true, driver->inhibitOpaque); /* Enable domain death events */ diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 0a9ccdf9ec..af8593d6a5 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -1468,7 +1468,7 @@ int virLXCProcessStart(virConnectPtr conn, if (virCommandHandshakeNotify(cmd) < 0) goto cleanup; - if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback) + if (virAtomicIntInc(&driver->nactive) == 0 && driver->inhibitCallback) driver->inhibitCallback(true, driver->inhibitOpaque); if (lxcContainerWaitForContinue(handshakefds[0]) < 0) { @@ -1670,7 +1670,7 @@ virLXCProcessReconnectDomain(virDomainObjPtr vm, virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_UNKNOWN); - if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback) + if (virAtomicIntInc(&driver->nactive) == 0 && driver->inhibitCallback) driver->inhibitCallback(true, driver->inhibitOpaque); if (!(priv->monitor = virLXCProcessConnectMonitor(driver, vm))) diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c index 9a71d13d57..f3acaf00dd 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.c +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -888,7 +888,7 @@ virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReqPtr req, skip_instantiate: VIR_FREE(ipl); - virAtomicIntDecAndTest(&virNWFilterSnoopState.nLeases); + ignore_value(virAtomicIntDecAndTest(&virNWFilterSnoopState.nLeases)); lease_not_found: VIR_FREE(ipstr); @@ -1167,7 +1167,7 @@ static void virNWFilterDHCPDecodeWorker(void *jobdata, void *opaque) _("Instantiation of rules failed on " "interface '%s'"), req->binding->portdevname); } - virAtomicIntDecAndTest(job->qCtr); + ignore_value(virAtomicIntDecAndTest(job->qCtr)); VIR_FREE(job); } @@ -1568,7 +1568,7 @@ virNWFilterDHCPSnoopThread(void *req0) pcap_close(pcapConf[i].handle); } - virAtomicIntDecAndTest(&virNWFilterSnoopState.nThreads); + ignore_value(virAtomicIntDecAndTest(&virNWFilterSnoopState.nThreads)); return; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a7bbab9e56..420d1c9c93 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5571,7 +5571,7 @@ qemuProcessInit(virQEMUDriverPtr driver, qemuDomainSetFakeReboot(driver, vm, false); virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_STARTING_UP); - if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback) + if (virAtomicIntInc(&driver->nactive) == 0 && driver->inhibitCallback) driver->inhibitCallback(true, driver->inhibitOpaque); /* Run an early hook to set-up missing devices */ @@ -8139,7 +8139,7 @@ qemuProcessReconnect(void *opaque) goto error; } - if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback) + if (virAtomicIntInc(&driver->nactive) == 0 && driver->inhibitCallback) driver->inhibitCallback(true, driver->inhibitOpaque); cleanup: diff --git a/src/util/viratomic.h b/src/util/viratomic.h index 9dfb77b992..12b116a6cd 100644 --- a/src/util/viratomic.h +++ b/src/util/viratomic.h @@ -1,11 +1,7 @@ /* * viratomic.h: atomic integer operations * - * Copyright (C) 2012 Red Hat, Inc. - * - * Based on code taken from GLib 2.32, under the LGPLv2+ - * - * Copyright (C) 2011 Ryan Lortie + * Copyright (C) 2012-2020 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -21,18 +17,15 @@ * License along with this library. If not, see * <http://www.gnu.org/licenses/>;. * + * APIs in this header should no longer be used. Direct + * use of the g_atomic APIs is preferred & existing code + * should be converted as needed. */ #pragma once #include "internal.h" -#ifdef VIR_ATOMIC_OPS_GCC -# define VIR_STATIC /* Nothing; we just never define the functions */ -#else -# define VIR_STATIC static -#endif - /** * virAtomicIntGet: * Gets the current value of atomic. @@ -40,8 +33,7 @@ * This call acts as a full compiler and hardware memory barrier * (before the get) */ -VIR_STATIC int virAtomicIntGet(volatile int *atomic) - ATTRIBUTE_NONNULL(1); +#define virAtomicIntGet(v) g_atomic_int_get(v) /** * virAtomicIntSet: @@ -50,21 +42,18 @@ VIR_STATIC int virAtomicIntGet(volatile int *atomic) * This call acts as a full compiler and hardware memory barrier * (after the set) */ -VIR_STATIC void virAtomicIntSet(volatile int *atomic, - int newval) - ATTRIBUTE_NONNULL(1); +#define virAtomicIntSet(i, newv) g_atomic_int_set(i, newv) /** * virAtomicIntInc: * Increments the value of atomic by 1. * * Think of this operation as an atomic version of - * { *atomic += 1; return *atomic; } + * { tmp = *atomic; *atomic += 1; return tmp; } * * This call acts as a full compiler and hardware memory barrier. */ -VIR_STATIC int virAtomicIntInc(volatile int *atomic) - ATTRIBUTE_NONNULL(1); +#define virAtomicIntInc(i) g_atomic_int_add(i, 1) /** * virAtomicIntDecAndTest: @@ -75,8 +64,7 @@ VIR_STATIC int virAtomicIntInc(volatile int *atomic) * * This call acts as a full compiler and hardware memory barrier. */ -VIR_STATIC bool virAtomicIntDecAndTest(volatile int *atomic) - ATTRIBUTE_NONNULL(1); +#define virAtomicIntDecAndTest(i) (!!g_atomic_int_dec_and_test(i)) /** * virAtomicIntCompareExchange: @@ -91,10 +79,8 @@ VIR_STATIC bool virAtomicIntDecAndTest(volatile int *atomic) * * This call acts as a full compiler and hardware memory barrier. */ -VIR_STATIC bool virAtomicIntCompareExchange(volatile int *atomic, - int oldval, - int newval) - ATTRIBUTE_NONNULL(1); +#define virAtomicIntCompareExchange(i, oldi, newi) \ + (!!g_atomic_int_compare_and_exchange(i, oldi, newi)) /** * virAtomicIntAdd: @@ -105,9 +91,7 @@ VIR_STATIC bool virAtomicIntCompareExchange(volatile int *atomic, * * This call acts as a full compiler and hardware memory barrier. */ -VIR_STATIC int virAtomicIntAdd(volatile int *atomic, - int val) - ATTRIBUTE_NONNULL(1); +#define virAtomicIntAdd(i, v) g_atomic_int_add(i, v) /** * virAtomicIntAnd: @@ -119,9 +103,7 @@ VIR_STATIC int virAtomicIntAdd(volatile int *atomic, * Think of this operation as an atomic version of * { tmp = *atomic; *atomic &= val; return tmp; } */ -VIR_STATIC unsigned int virAtomicIntAnd(volatile unsigned int *atomic, - unsigned int val) - ATTRIBUTE_NONNULL(1); +#define virAtomicIntAnd(i, v) g_atomic_int_and(i, v) /** * virAtomicIntOr: @@ -133,9 +115,7 @@ VIR_STATIC unsigned int virAtomicIntAnd(volatile unsigned int *atomic, * * This call acts as a full compiler and hardware memory barrier. */ -VIR_STATIC unsigned int virAtomicIntOr(volatile unsigned int *atomic, - unsigned int val) - ATTRIBUTE_NONNULL(1); +#define virAtomicIntOr(i, v) g_atomic_int_or(i, v) /** * virAtomicIntXor: @@ -147,305 +127,4 @@ VIR_STATIC unsigned int virAtomicIntOr(volatile unsigned int *atomic, * * This call acts as a full compiler and hardware memory barrier. */ -VIR_STATIC unsigned int virAtomicIntXor(volatile unsigned int *atomic, - unsigned int val) - ATTRIBUTE_NONNULL(1); - -#undef VIR_STATIC - -#ifdef VIR_ATOMIC_OPS_GCC - -# define virAtomicIntGet(atomic) \ - (__extension__ ({ \ - (void)verify_expr(sizeof(*(atomic)) == sizeof(int), 0); \ - (void)(0 ? *(atomic) ^ *(atomic) : 0); \ - __sync_synchronize(); \ - (int)*(atomic); \ - })) -# define virAtomicIntSet(atomic, newval) \ - (__extension__ ({ \ - (void)verify_expr(sizeof(*(atomic)) == sizeof(int), 0); \ - (void)(0 ? *(atomic) ^ (newval) : 0); \ - *(atomic) = (newval); \ - __sync_synchronize(); \ - })) -# define virAtomicIntInc(atomic) \ - (__extension__ ({ \ - (void)verify_expr(sizeof(*(atomic)) == sizeof(int), 0); \ - (void)(0 ? *(atomic) ^ *(atomic) : 0); \ - __sync_add_and_fetch((atomic), 1); \ - })) -# define virAtomicIntDecAndTest(atomic) \ - (__extension__ ({ \ - (void)verify_expr(sizeof(*(atomic)) == sizeof(int), 0); \ - (void)(0 ? *(atomic) ^ *(atomic) : 0); \ - __sync_fetch_and_sub((atomic), 1) == 1; \ - })) -# define virAtomicIntCompareExchange(atomic, oldval, newval) \ - (__extension__ ({ \ - (void)verify_expr(sizeof(*(atomic)) == sizeof(int), 0); \ - (void)(0 ? *(atomic) ^ (newval) ^ (oldval) : 0); \ - (bool)__sync_bool_compare_and_swap((atomic), \ - (oldval), (newval)); \ - })) -# define virAtomicIntAdd(atomic, val) \ - (__extension__ ({ \ - (void)verify_expr(sizeof(*(atomic)) == sizeof(int), 0); \ - (void)(0 ? *(atomic) ^ (val) : 0); \ - (int) __sync_fetch_and_add((atomic), (val)); \ - })) -# define virAtomicIntAnd(atomic, val) \ - (__extension__ ({ \ - (void)verify_expr(sizeof(*(atomic)) == sizeof(int), 0); \ - (void) (0 ? *(atomic) ^ (val) : 0); \ - (unsigned int) __sync_fetch_and_and((atomic), (val)); \ - })) -# define virAtomicIntOr(atomic, val) \ - (__extension__ ({ \ - (void)verify_expr(sizeof(*(atomic)) == sizeof(int), 0); \ - (void) (0 ? *(atomic) ^ (val) : 0); \ - (unsigned int) __sync_fetch_and_or((atomic), (val)); \ - })) -# define virAtomicIntXor(atomic, val) \ - (__extension__ ({ \ - (void)verify_expr(sizeof(*(atomic)) == sizeof(int), 0); \ - (void) (0 ? *(atomic) ^ (val) : 0); \ - (unsigned int) __sync_fetch_and_xor((atomic), (val)); \ - })) - - -#else - -# ifdef VIR_ATOMIC_OPS_WIN32 - -# include <winsock2.h> -# include <windows.h> -# include <intrin.h> -# if !defined(_M_AMD64) && !defined (_M_IA64) && !defined(_M_X64) -# define InterlockedAnd _InterlockedAnd -# define InterlockedOr _InterlockedOr -# define InterlockedXor _InterlockedXor -# endif - -/* - * http://msdn.microsoft.com/en-us/library/ms684122(v=vs.85).aspx - */ -static inline int -virAtomicIntGet(volatile int *atomic) -{ - MemoryBarrier(); - return *atomic; -} - -static inline void -virAtomicIntSet(volatile int *atomic, - int newval) -{ - *atomic = newval; - MemoryBarrier(); -} - -static inline int -virAtomicIntInc(volatile int *atomic) -{ - return InterlockedIncrement((volatile LONG *)atomic); -} - -static inline bool -virAtomicIntDecAndTest(volatile int *atomic) -{ - return InterlockedDecrement((volatile LONG *)atomic) == 0; -} - -static inline bool -virAtomicIntCompareExchange(volatile int *atomic, - int oldval, - int newval) -{ - return InterlockedCompareExchange((volatile LONG *)atomic, newval, oldval) == oldval; -} - -static inline int -virAtomicIntAdd(volatile int *atomic, - int val) -{ - return InterlockedExchangeAdd((volatile LONG *)atomic, val); -} - -static inline unsigned int -virAtomicIntAnd(volatile unsigned int *atomic, - unsigned int val) -{ - return InterlockedAnd((volatile LONG *)atomic, val); -} - -static inline unsigned int -virAtomicIntOr(volatile unsigned int *atomic, - unsigned int val) -{ - return InterlockedOr((volatile LONG *)atomic, val); -} - -static inline unsigned int -virAtomicIntXor(volatile unsigned int *atomic, - unsigned int val) -{ - return InterlockedXor((volatile LONG *)atomic, val); -} - - -# else -# ifdef VIR_ATOMIC_OPS_PTHREAD -# include <pthread.h> - -extern pthread_mutex_t virAtomicLock; - -static inline int -virAtomicIntGet(volatile int *atomic) -{ - int value; - - pthread_mutex_lock(&virAtomicLock); - value = *atomic; - pthread_mutex_unlock(&virAtomicLock); - - return value; -} - -static inline void -virAtomicIntSet(volatile int *atomic, - int value) -{ - pthread_mutex_lock(&virAtomicLock); - *atomic = value; - pthread_mutex_unlock(&virAtomicLock); -} - -static inline int -virAtomicIntInc(volatile int *atomic) -{ - int value; - - pthread_mutex_lock(&virAtomicLock); - value = ++(*atomic); - pthread_mutex_unlock(&virAtomicLock); - - return value; -} - -static inline bool -virAtomicIntDecAndTest(volatile int *atomic) -{ - bool is_zero; - - pthread_mutex_lock(&virAtomicLock); - is_zero = --(*atomic) == 0; - pthread_mutex_unlock(&virAtomicLock); - - return is_zero; -} - -static inline bool -virAtomicIntCompareExchange(volatile int *atomic, - int oldval, - int newval) -{ - bool success; - - pthread_mutex_lock(&virAtomicLock); - - if ((success = (*atomic == oldval))) - *atomic = newval; - - pthread_mutex_unlock(&virAtomicLock); - - return success; -} - -static inline int -virAtomicIntAdd(volatile int *atomic, - int val) -{ - int oldval; - - pthread_mutex_lock(&virAtomicLock); - oldval = *atomic; - *atomic = oldval + val; - pthread_mutex_unlock(&virAtomicLock); - - return oldval; -} - -static inline unsigned int -virAtomicIntAnd(volatile unsigned int *atomic, - unsigned int val) -{ - unsigned int oldval; - - pthread_mutex_lock(&virAtomicLock); - oldval = *atomic; - *atomic = oldval & val; - pthread_mutex_unlock(&virAtomicLock); - - return oldval; -} - -static inline unsigned int -virAtomicIntOr(volatile unsigned int *atomic, - unsigned int val) -{ - unsigned int oldval; - - pthread_mutex_lock(&virAtomicLock); - oldval = *atomic; - *atomic = oldval | val; - pthread_mutex_unlock(&virAtomicLock); - - return oldval; -} - -static inline unsigned int -virAtomicIntXor(volatile unsigned int *atomic, - unsigned int val) -{ - unsigned int oldval; - - pthread_mutex_lock(&virAtomicLock); - oldval = *atomic; - *atomic = oldval ^ val; - pthread_mutex_unlock(&virAtomicLock); - - return oldval; -} - - -# else -# error "No atomic integer impl for this platform" -# endif -# endif - -/* The int/unsigned int casts here ensure that you can - * pass either an int or unsigned int to all atomic op - * functions, in the same way that we can with GCC - * atomic op helpers. - */ -# define virAtomicIntGet(atomic) \ - virAtomicIntGet((int *)atomic) -# define virAtomicIntSet(atomic, val) \ - virAtomicIntSet((int *)atomic, val) -# define virAtomicIntInc(atomic) \ - virAtomicIntInc((int *)atomic) -# define virAtomicIntDecAndTest(atomic) \ - virAtomicIntDecAndTest((int *)atomic) -# define virAtomicIntCompareExchange(atomic, oldval, newval) \ - virAtomicIntCompareExchange((int *)atomic, oldval, newval) -# define virAtomicIntAdd(atomic, val) \ - virAtomicIntAdd((int *)atomic, val) -# define virAtomicIntAnd(atomic, val) \ - virAtomicIntAnd((unsigned int *)atomic, val) -# define virAtomicIntOr(atomic, val) \ - virAtomicIntOr((unsigned int *)atomic, val) -# define virAtomicIntXor(atomic, val) \ - virAtomicIntXor((unsigned int *)atomic, val) - -#endif +#define virAtomicIntXor(i, v) g_atomic_int_xor(i, v) diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 32f19e6b63..d5589daf6a 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -1021,7 +1021,7 @@ int virProcessGetStartTime(pid_t pid, unsigned long long *timestamp) { static int warned; - if (virAtomicIntInc(&warned) == 1) { + if (virAtomicIntInc(&warned) == 0) { VIR_WARN("Process start time of pid %lld not available on this platform", (long long) pid); } diff --git a/tests/viratomictest.c b/tests/viratomictest.c index 8c885a5b96..e30df66cd7 100644 --- a/tests/viratomictest.c +++ b/tests/viratomictest.c @@ -50,7 +50,7 @@ testTypes(const void *data G_GNUC_UNUSED) testAssertEq(virAtomicIntAdd(&u, 1), 5); testAssertEq(u, 6); - testAssertEq(virAtomicIntInc(&u), 7); + testAssertEq(virAtomicIntInc(&u), 6); testAssertEq(u, 7); res = virAtomicIntDecAndTest(&u); -- 2.24.1