[libvirt PATCH v2 09/13] util: replace atomic ops impls with g_atomic_int*
Daniel P. Berrangé
berrange at redhat.com
Thu Jan 16 15:24:44 UTC 2020
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 at redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
---
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
More information about the libvir-list
mailing list