[PATCH v2 0/3] qemu-sem-posix: use monotonic clock instead

[PATCH v2 0/3] qemu-sem-posix: use monotonic clock instead

[Longpeng(Mike) via]

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(-)

-- 
1.8.3.1

[PATCH v2 1/3] sem-posix: remove the posix semaphore support

[Longpeng(Mike) via]

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

[PATCH v2 2/3] sem-posix: use monotonic clock instead

[Longpeng(Mike) via]

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

[PATCH v2 3/3] sem-posix: refactor qemu-sem with qemu-cond and qemu-mutex

[Longpeng(Mike) via]

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

Re: [PATCH v2 0/3] qemu-sem-posix: use monotonic clock instead

[Paolo Bonzini]

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