* [PATCH v2 1/3] sem-posix: remove the posix semaphore support
2022-02-22 9:05 [PATCH v2 0/3] qemu-sem-posix: use monotonic clock instead Longpeng(Mike) via
@ 2022-02-22 9:05 ` Longpeng(Mike) via
2022-02-22 9:05 ` [PATCH v2 2/3] sem-posix: use monotonic clock instead Longpeng(Mike) via
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Longpeng(Mike) via @ 2022-02-22 9:05 UTC (permalink / raw)
To: pbonzini, berrange, mst
Cc: qemu-devel, arei.gonglei, wangxinxin.wang, Longpeng(Mike)
POSIX specifies an absolute time for sem_timedwait(), it would be
affected if the system time is changing, but there is not a relative
time or monotonic clock version of sem_timedwait, so we cannot gain
from POSIX semaphore any more.
An alternative way is to use sem_trywait + usleep, maybe we can
remove CONFIG_SEM_TIMEDWAIT in this way? No, because some systems
(e.g. mac os) mark the sem_xxx API as deprecated.
So maybe remove the usage of POSIX semaphore and turn to use the
pthread variant for all systems looks better.
Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
---
include/qemu/thread-posix.h | 4 ----
meson.build | 1 -
util/qemu-thread-posix.c | 54 ---------------------------------------------
3 files changed, 59 deletions(-)
diff --git a/include/qemu/thread-posix.h b/include/qemu/thread-posix.h
index b792e6e..5466608 100644
--- a/include/qemu/thread-posix.h
+++ b/include/qemu/thread-posix.h
@@ -27,13 +27,9 @@ struct QemuCond {
};
struct QemuSemaphore {
-#ifndef CONFIG_SEM_TIMEDWAIT
pthread_mutex_t lock;
pthread_cond_t cond;
unsigned int count;
-#else
- sem_t sem;
-#endif
bool initialized;
};
diff --git a/meson.build b/meson.build
index 762d7ce..3ccb110 100644
--- a/meson.build
+++ b/meson.build
@@ -1557,7 +1557,6 @@ config_host_data.set('CONFIG_POSIX_FALLOCATE', cc.has_function('posix_fallocate'
config_host_data.set('CONFIG_POSIX_MEMALIGN', cc.has_function('posix_memalign'))
config_host_data.set('CONFIG_PPOLL', cc.has_function('ppoll'))
config_host_data.set('CONFIG_PREADV', cc.has_function('preadv', prefix: '#include <sys/uio.h>'))
-config_host_data.set('CONFIG_SEM_TIMEDWAIT', cc.has_function('sem_timedwait', dependencies: threads))
config_host_data.set('CONFIG_SENDFILE', cc.has_function('sendfile'))
config_host_data.set('CONFIG_SETNS', cc.has_function('setns') and cc.has_function('unshare'))
config_host_data.set('CONFIG_SYNCFS', cc.has_function('syncfs'))
diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
index e1225b6..1ad2503 100644
--- a/util/qemu-thread-posix.c
+++ b/util/qemu-thread-posix.c
@@ -219,7 +219,6 @@ void qemu_sem_init(QemuSemaphore *sem, int init)
{
int rc;
-#ifndef CONFIG_SEM_TIMEDWAIT
rc = pthread_mutex_init(&sem->lock, NULL);
if (rc != 0) {
error_exit(rc, __func__);
@@ -232,12 +231,6 @@ void qemu_sem_init(QemuSemaphore *sem, int init)
error_exit(EINVAL, __func__);
}
sem->count = init;
-#else
- rc = sem_init(&sem->sem, 0, init);
- if (rc < 0) {
- error_exit(errno, __func__);
- }
-#endif
sem->initialized = true;
}
@@ -247,7 +240,6 @@ void qemu_sem_destroy(QemuSemaphore *sem)
assert(sem->initialized);
sem->initialized = false;
-#ifndef CONFIG_SEM_TIMEDWAIT
rc = pthread_cond_destroy(&sem->cond);
if (rc < 0) {
error_exit(rc, __func__);
@@ -256,12 +248,6 @@ void qemu_sem_destroy(QemuSemaphore *sem)
if (rc < 0) {
error_exit(rc, __func__);
}
-#else
- rc = sem_destroy(&sem->sem);
- if (rc < 0) {
- error_exit(errno, __func__);
- }
-#endif
}
void qemu_sem_post(QemuSemaphore *sem)
@@ -269,7 +255,6 @@ void qemu_sem_post(QemuSemaphore *sem)
int rc;
assert(sem->initialized);
-#ifndef CONFIG_SEM_TIMEDWAIT
pthread_mutex_lock(&sem->lock);
if (sem->count == UINT_MAX) {
rc = EINVAL;
@@ -281,12 +266,6 @@ void qemu_sem_post(QemuSemaphore *sem)
if (rc != 0) {
error_exit(rc, __func__);
}
-#else
- rc = sem_post(&sem->sem);
- if (rc < 0) {
- error_exit(errno, __func__);
- }
-#endif
}
int qemu_sem_timedwait(QemuSemaphore *sem, int ms)
@@ -295,7 +274,6 @@ int qemu_sem_timedwait(QemuSemaphore *sem, int ms)
struct timespec ts;
assert(sem->initialized);
-#ifndef CONFIG_SEM_TIMEDWAIT
rc = 0;
compute_abs_deadline(&ts, ms);
pthread_mutex_lock(&sem->lock);
@@ -313,29 +291,6 @@ int qemu_sem_timedwait(QemuSemaphore *sem, int ms)
}
pthread_mutex_unlock(&sem->lock);
return (rc == ETIMEDOUT ? -1 : 0);
-#else
- if (ms <= 0) {
- /* This is cheaper than sem_timedwait. */
- do {
- rc = sem_trywait(&sem->sem);
- } while (rc == -1 && errno == EINTR);
- if (rc == -1 && errno == EAGAIN) {
- return -1;
- }
- } else {
- compute_abs_deadline(&ts, ms);
- do {
- rc = sem_timedwait(&sem->sem, &ts);
- } while (rc == -1 && errno == EINTR);
- if (rc == -1 && errno == ETIMEDOUT) {
- return -1;
- }
- }
- if (rc < 0) {
- error_exit(errno, __func__);
- }
- return 0;
-#endif
}
void qemu_sem_wait(QemuSemaphore *sem)
@@ -343,7 +298,6 @@ void qemu_sem_wait(QemuSemaphore *sem)
int rc;
assert(sem->initialized);
-#ifndef CONFIG_SEM_TIMEDWAIT
pthread_mutex_lock(&sem->lock);
while (sem->count == 0) {
rc = pthread_cond_wait(&sem->cond, &sem->lock);
@@ -353,14 +307,6 @@ void qemu_sem_wait(QemuSemaphore *sem)
}
--sem->count;
pthread_mutex_unlock(&sem->lock);
-#else
- do {
- rc = sem_wait(&sem->sem);
- } while (rc == -1 && errno == EINTR);
- if (rc < 0) {
- error_exit(errno, __func__);
- }
-#endif
}
#ifdef __linux__
--
1.8.3.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH v2 2/3] sem-posix: use monotonic clock instead
2022-02-22 9:05 [PATCH v2 0/3] qemu-sem-posix: use monotonic clock instead Longpeng(Mike) via
2022-02-22 9:05 ` [PATCH v2 1/3] sem-posix: remove the posix semaphore support Longpeng(Mike) via
@ 2022-02-22 9:05 ` Longpeng(Mike) via
2022-02-22 9:05 ` [PATCH v2 3/3] sem-posix: refactor qemu-sem with qemu-cond and qemu-mutex Longpeng(Mike) via
2022-02-23 9:39 ` [PATCH v2 0/3] qemu-sem-posix: use monotonic clock instead Paolo Bonzini
3 siblings, 0 replies; 5+ messages in thread
From: Longpeng(Mike) via @ 2022-02-22 9:05 UTC (permalink / raw)
To: pbonzini, berrange, mst
Cc: qemu-devel, arei.gonglei, wangxinxin.wang, Longpeng(Mike)
Use CLOCK_MONOTONIC, so the timeout isn't affected by changes to
the system time. It depends on the pthread_condattr_setclock(),
while some systems(e.g. mac os) does not support it, so the behavior
won't change in these systems.
Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
---
meson.build | 11 +++++++++++
util/qemu-thread-posix.c | 49 +++++++++++++++++++++++++++++++++++++++++-------
2 files changed, 53 insertions(+), 7 deletions(-)
diff --git a/meson.build b/meson.build
index 3ccb110..2bab94f 100644
--- a/meson.build
+++ b/meson.build
@@ -1688,6 +1688,17 @@ config_host_data.set('CONFIG_PTHREAD_SETNAME_NP_WO_TID', cc.links(gnu_source_pre
pthread_create(&thread, 0, f, 0);
return 0;
}''', dependencies: threads))
+config_host_data.set('CONFIG_PTHREAD_CONDATTR_SETCLOCK', cc.links(gnu_source_prefix + '''
+ #include <pthread.h>
+ #include <time.h>
+
+ int main(void)
+ {
+ pthread_condattr_t attr
+ pthread_condattr_init(&attr);
+ pthread_condattr_setclock(&attr, CLOCK_MONOTONIC);
+ return 0;
+ }''', dependencies: threads))
config_host_data.set('CONFIG_SIGNALFD', cc.links(gnu_source_prefix + '''
#include <sys/signalfd.h>
diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
index 1ad2503..44446ce 100644
--- a/util/qemu-thread-posix.c
+++ b/util/qemu-thread-posix.c
@@ -38,12 +38,20 @@ static void error_exit(int err, const char *msg)
abort();
}
+static inline clockid_t qemu_timedwait_clockid(void)
+{
+#ifdef CONFIG_PTHREAD_CONDATTR_SETCLOCK
+ return CLOCK_MONOTONIC;
+#else
+ return CLOCK_REALTIME;
+#endif
+}
+
static void compute_abs_deadline(struct timespec *ts, int ms)
{
- struct timeval tv;
- gettimeofday(&tv, NULL);
- ts->tv_nsec = tv.tv_usec * 1000 + (ms % 1000) * 1000000;
- ts->tv_sec = tv.tv_sec + ms / 1000;
+ clock_gettime(qemu_timedwait_clockid(), ts);
+ ts->tv_nsec += (ms % 1000) * 1000000;
+ ts->tv_sec += ms / 1000;
if (ts->tv_nsec >= 1000000000) {
ts->tv_sec++;
ts->tv_nsec -= 1000000000;
@@ -147,11 +155,25 @@ void qemu_rec_mutex_unlock_impl(QemuRecMutex *mutex, const char *file, int line)
void qemu_cond_init(QemuCond *cond)
{
+ pthread_condattr_t attr;
int err;
- err = pthread_cond_init(&cond->cond, NULL);
- if (err)
+ err = pthread_condattr_init(&attr);
+ if (err) {
+ error_exit(err, __func__);
+ }
+ err = pthread_condattr_setclock(&attr, qemu_timedwait_clockid());
+ if (err) {
+ error_exit(err, __func__);
+ }
+ err = pthread_cond_init(&cond->cond, &attr);
+ if (err) {
error_exit(err, __func__);
+ }
+ err = pthread_condattr_destroy(&attr);
+ if (err) {
+ error_exit(err, __func__);
+ }
cond->initialized = true;
}
@@ -217,16 +239,29 @@ bool qemu_cond_timedwait_impl(QemuCond *cond, QemuMutex *mutex, int ms,
void qemu_sem_init(QemuSemaphore *sem, int init)
{
+ pthread_condattr_t attr;
int rc;
rc = pthread_mutex_init(&sem->lock, NULL);
if (rc != 0) {
error_exit(rc, __func__);
}
- rc = pthread_cond_init(&sem->cond, NULL);
+ rc = pthread_condattr_init(&attr);
+ if (rc != 0) {
+ error_exit(rc, __func__);
+ }
+ rc = pthread_condattr_setclock(&attr, qemu_timedwait_clockid());
if (rc != 0) {
error_exit(rc, __func__);
}
+ rc = pthread_cond_init(&sem->cond, &attr);
+ if (rc != 0) {
+ error_exit(rc, __func__);
+ }
+ rc = pthread_condattr_destroy(&attr);
+ if (rc < 0) {
+ error_exit(rc, __func__);
+ }
if (init < 0) {
error_exit(EINVAL, __func__);
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH v2 3/3] sem-posix: refactor qemu-sem with qemu-cond and qemu-mutex
2022-02-22 9:05 [PATCH v2 0/3] qemu-sem-posix: use monotonic clock instead Longpeng(Mike) via
2022-02-22 9:05 ` [PATCH v2 1/3] sem-posix: remove the posix semaphore support Longpeng(Mike) via
2022-02-22 9:05 ` [PATCH v2 2/3] sem-posix: use monotonic clock instead Longpeng(Mike) via
@ 2022-02-22 9:05 ` Longpeng(Mike) via
2022-02-23 9:39 ` [PATCH v2 0/3] qemu-sem-posix: use monotonic clock instead Paolo Bonzini
3 siblings, 0 replies; 5+ messages in thread
From: Longpeng(Mike) via @ 2022-02-22 9:05 UTC (permalink / raw)
To: pbonzini, berrange, mst
Cc: qemu-devel, arei.gonglei, wangxinxin.wang, Longpeng(Mike)
Now, qemu-sem is based on the pthread_cond only, we can use
qemu-cond and qemu-mutex to make the code neater and the mutex
trace can be supported in qemu-sem naturally.
Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
---
include/qemu/thread-posix.h | 5 +--
util/qemu-thread-posix.c | 103 ++++++++++++++------------------------------
2 files changed, 34 insertions(+), 74 deletions(-)
diff --git a/include/qemu/thread-posix.h b/include/qemu/thread-posix.h
index 5466608..5f2f3d1 100644
--- a/include/qemu/thread-posix.h
+++ b/include/qemu/thread-posix.h
@@ -27,10 +27,9 @@ struct QemuCond {
};
struct QemuSemaphore {
- pthread_mutex_t lock;
- pthread_cond_t cond;
+ QemuMutex mutex;
+ QemuCond cond;
unsigned int count;
- bool initialized;
};
struct QemuEvent {
diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
index 44446ce..f2ce47d 100644
--- a/util/qemu-thread-posix.c
+++ b/util/qemu-thread-posix.c
@@ -220,16 +220,15 @@ void qemu_cond_wait_impl(QemuCond *cond, QemuMutex *mutex, const char *file, con
error_exit(err, __func__);
}
-bool qemu_cond_timedwait_impl(QemuCond *cond, QemuMutex *mutex, int ms,
- const char *file, const int line)
+static bool
+qemu_cond_timedwait_ts(QemuCond *cond, QemuMutex *mutex, struct timespec *ts,
+ const char *file, const int line)
{
int err;
- struct timespec ts;
assert(cond->initialized);
trace_qemu_mutex_unlock(mutex, file, line);
- compute_abs_deadline(&ts, ms);
- err = pthread_cond_timedwait(&cond->cond, &mutex->lock, &ts);
+ err = pthread_cond_timedwait(&cond->cond, &mutex->lock, ts);
trace_qemu_mutex_locked(mutex, file, line);
if (err && err != ETIMEDOUT) {
error_exit(err, __func__);
@@ -237,111 +236,73 @@ bool qemu_cond_timedwait_impl(QemuCond *cond, QemuMutex *mutex, int ms,
return err != ETIMEDOUT;
}
+bool qemu_cond_timedwait_impl(QemuCond *cond, QemuMutex *mutex, int ms,
+ const char *file, const int line)
+{
+ struct timespec ts;
+
+ compute_abs_deadline(&ts, ms);
+ return qemu_cond_timedwait_ts(cond, mutex, &ts, file, line);
+}
+
void qemu_sem_init(QemuSemaphore *sem, int init)
{
- pthread_condattr_t attr;
- int rc;
+ qemu_mutex_init(&sem->mutex);
+ qemu_cond_init(&sem->cond);
- rc = pthread_mutex_init(&sem->lock, NULL);
- if (rc != 0) {
- error_exit(rc, __func__);
- }
- rc = pthread_condattr_init(&attr);
- if (rc != 0) {
- error_exit(rc, __func__);
- }
- rc = pthread_condattr_setclock(&attr, qemu_timedwait_clockid());
- if (rc != 0) {
- error_exit(rc, __func__);
- }
- rc = pthread_cond_init(&sem->cond, &attr);
- if (rc != 0) {
- error_exit(rc, __func__);
- }
- rc = pthread_condattr_destroy(&attr);
- if (rc < 0) {
- error_exit(rc, __func__);
- }
if (init < 0) {
error_exit(EINVAL, __func__);
}
sem->count = init;
- sem->initialized = true;
}
void qemu_sem_destroy(QemuSemaphore *sem)
{
- int rc;
-
- assert(sem->initialized);
- sem->initialized = false;
- rc = pthread_cond_destroy(&sem->cond);
- if (rc < 0) {
- error_exit(rc, __func__);
- }
- rc = pthread_mutex_destroy(&sem->lock);
- if (rc < 0) {
- error_exit(rc, __func__);
- }
+ qemu_cond_destroy(&sem->cond);
+ qemu_mutex_destroy(&sem->mutex);
}
void qemu_sem_post(QemuSemaphore *sem)
{
- int rc;
-
- assert(sem->initialized);
- pthread_mutex_lock(&sem->lock);
+ qemu_mutex_lock(&sem->mutex);
if (sem->count == UINT_MAX) {
- rc = EINVAL;
+ error_exit(EINVAL, __func__);
} else {
sem->count++;
- rc = pthread_cond_signal(&sem->cond);
- }
- pthread_mutex_unlock(&sem->lock);
- if (rc != 0) {
- error_exit(rc, __func__);
+ qemu_cond_signal(&sem->cond);
}
+ qemu_mutex_unlock(&sem->mutex);
}
int qemu_sem_timedwait(QemuSemaphore *sem, int ms)
{
- int rc;
+ bool rc = true;
struct timespec ts;
- assert(sem->initialized);
- rc = 0;
compute_abs_deadline(&ts, ms);
- pthread_mutex_lock(&sem->lock);
+ qemu_mutex_lock(&sem->mutex);
while (sem->count == 0) {
- rc = pthread_cond_timedwait(&sem->cond, &sem->lock, &ts);
- if (rc == ETIMEDOUT) {
+ rc = qemu_cond_timedwait_ts(&sem->cond, &sem->mutex, &ts,
+ __FILE__, __LINE__);
+ if (!rc) { /* timeout */
break;
}
- if (rc != 0) {
- error_exit(rc, __func__);
- }
}
- if (rc != ETIMEDOUT) {
+ if (rc) {
--sem->count;
}
- pthread_mutex_unlock(&sem->lock);
- return (rc == ETIMEDOUT ? -1 : 0);
+ qemu_mutex_unlock(&sem->mutex);
+ return (rc ? 0 : -1);
}
void qemu_sem_wait(QemuSemaphore *sem)
{
- int rc;
-
- assert(sem->initialized);
- pthread_mutex_lock(&sem->lock);
+ qemu_mutex_lock(&sem->mutex);
while (sem->count == 0) {
- rc = pthread_cond_wait(&sem->cond, &sem->lock);
- if (rc != 0) {
- error_exit(rc, __func__);
- }
+ qemu_cond_wait(&sem->cond, &sem->mutex);
}
--sem->count;
- pthread_mutex_unlock(&sem->lock);
+ qemu_mutex_unlock(&sem->mutex);
}
#ifdef __linux__
--
1.8.3.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v2 0/3] qemu-sem-posix: use monotonic clock instead
2022-02-22 9:05 [PATCH v2 0/3] qemu-sem-posix: use monotonic clock instead Longpeng(Mike) via
` (2 preceding siblings ...)
2022-02-22 9:05 ` [PATCH v2 3/3] sem-posix: refactor qemu-sem with qemu-cond and qemu-mutex Longpeng(Mike) via
@ 2022-02-23 9:39 ` Paolo Bonzini
3 siblings, 0 replies; 5+ messages in thread
From: Paolo Bonzini @ 2022-02-23 9:39 UTC (permalink / raw)
To: Longpeng(Mike), berrange, mst; +Cc: wangxinxin.wang, arei.gonglei, qemu-devel
On 2/22/22 10:05, Longpeng(Mike) via wrote:
> The qemu_sem_timedwait() uses system time as default, it would be affected by
> changes to the system time. In the real scenario, the time that goes faster or
> slower is a common case and the NTP service could help us to sync time
> periodically.
>
> This patchset uses monotonic clock instead of the realtime clock, this could
> make sure we would not be affected by the system time anymore.
>
> Changes v1(RFC) -> v2:
> Patch 2:
> - clean the code [Paolo]
> - use pthread_condattr_setclock when initializing qemu-cond. [Paolo]
> Patch 3:
> - new added, make the qemu-sem code neater. [Longpeng]
>
> Longpeng (Mike) (3):
> sem-posix: remove the posix semaphore support
> sem-posix: use monotonic clock instead
> sem-posix: refactor qemu-sem with qemu-cond and qemu-mutex
>
> include/qemu/thread-posix.h | 9 +--
> meson.build | 12 ++-
> util/qemu-thread-posix.c | 178 +++++++++++++++-----------------------------
> 3 files changed, 73 insertions(+), 126 deletions(-)
>
Queued, thanks! I also sent a small optimization patch on top.
Paolo
^ permalink raw reply [flat|nested] 5+ messages in thread