[RFC 0/2] qemu-sem-posix: use monotonic clock instead

[RFC 0/2] 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.

Longpeng (Mike) (2):
  sem-posix: remove the posix semaphore support
  sem-posix: use monotonic clock instead

 include/qemu/thread-posix.h |  5 +--
 meson.build                 | 12 ++++++-
 util/qemu-thread-posix.c    | 82 +++++++++++++++------------------------------
 3 files changed, 39 insertions(+), 60 deletions(-)

-- 
1.8.3.1

[RFC 1/2] 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 anymore.

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

[RFC 2/2] 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) do not support it,  the behavior
won't change in these systems.

Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
---
 include/qemu/thread-posix.h |  1 +
 meson.build                 | 11 +++++++++++
 util/qemu-thread-posix.c    | 32 +++++++++++++++++++++++++++++---
 3 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/include/qemu/thread-posix.h b/include/qemu/thread-posix.h
index 5466608..cc77000 100644
--- a/include/qemu/thread-posix.h
+++ b/include/qemu/thread-posix.h
@@ -29,6 +29,7 @@ struct QemuCond {
 struct QemuSemaphore {
     pthread_mutex_t lock;
     pthread_cond_t cond;
+    pthread_condattr_t attr;
     unsigned int count;
     bool initialized;
 };
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..d3a7c54 100644
--- a/util/qemu-thread-posix.c
+++ b/util/qemu-thread-posix.c
@@ -40,10 +40,22 @@ static void error_exit(int err, const char *msg)
 
 static void compute_abs_deadline(struct timespec *ts, int ms)
 {
+    time_t now_sec;
+    long now_nsec;
+#ifdef CONFIG_PTHREAD_CONDATTR_SETCLOCK
+    struct timespec now;
+    clock_gettime(CLOCK_MONOTONIC, &now);
+    now_sec = now.tv_sec;
+    now_nsec = now.tv_nsec;
+#else
     struct timeval tv;
     gettimeofday(&tv, NULL);
-    ts->tv_nsec = tv.tv_usec * 1000 + (ms % 1000) * 1000000;
-    ts->tv_sec = tv.tv_sec + ms / 1000;
+    now_sec = tv.tv_sec;
+    now_nsec = tv.tv_usec * 1000;
+#endif
+
+    ts->tv_nsec = now_nsec + (ms % 1000) * 1000000;
+    ts->tv_sec = now_sec + ms / 1000;
     if (ts->tv_nsec >= 1000000000) {
         ts->tv_sec++;
         ts->tv_nsec -= 1000000000;
@@ -223,7 +235,17 @@ void qemu_sem_init(QemuSemaphore *sem, int init)
     if (rc != 0) {
         error_exit(rc, __func__);
     }
-    rc = pthread_cond_init(&sem->cond, NULL);
+    rc = pthread_condattr_init(&sem->attr);
+    if (rc != 0) {
+        error_exit(rc, __func__);
+    }
+#ifdef CONFIG_PTHREAD_CONDATTR_SETCLOCK
+    rc = pthread_condattr_setclock(&sem->attr, CLOCK_MONOTONIC);
+    if (rc != 0) {
+        error_exit(rc, __func__);
+    }
+#endif
+    rc = pthread_cond_init(&sem->cond, &sem->attr);
     if (rc != 0) {
         error_exit(rc, __func__);
     }
@@ -248,6 +270,10 @@ void qemu_sem_destroy(QemuSemaphore *sem)
     if (rc < 0) {
         error_exit(rc, __func__);
     }
+    rc = pthread_condattr_destroy(&sem->attr);
+    if (rc < 0) {
+        error_exit(rc, __func__);
+    }
 }
 
 void qemu_sem_post(QemuSemaphore *sem)
-- 
1.8.3.1

Re: [RFC 1/2] sem-posix: remove the posix semaphore support

[Daniel P. Berrangé]

On Mon, Feb 21, 2022 at 05:56:16PM +0800, Longpeng(Mike) via wrote:
> 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 anymore.

It doesn't appear in any man pages on my systems, but there is a
new-ish API  sem_clockwait() that accepts a choice of clock as a
parameter.

This is apparently a proposed POSIX standard API introduced in
glibc 2.30, along with several others:

https://sourceware.org/legacy-ml/libc-announce/2019/msg00001.html

[quote]
* Add new POSIX-proposed pthread_cond_clockwait, pthread_mutex_clocklock,
  pthread_rwlock_clockrdlock, pthread_rwlock_clockwrlock and sem_clockwait
  functions.  These behave similarly to their "timed" equivalents, but also
  accept a clockid_t parameter to determine which clock their timeout should
  be measured against.  All functions allow waiting against CLOCK_MONOTONIC
  and CLOCK_REALTIME.  The decision of which clock to be used is made at the
  time of the wait (unlike with pthread_condattr_setclock, which requires
  the clock choice at initialization time).
[/quote]

> 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;
>  };

As a point of history, the original  code only used sem_t. The pthreads
based fallback was introduced by Paolo in

  commit c166cb72f1676855816340666c3b618beef4b976
  Author: Paolo Bonzini <pbonzini@redhat.com>
  Date:   Fri Nov 2 15:43:21 2012 +0100

    semaphore: implement fallback counting semaphores with mutex+condvar
    
    OpenBSD and Darwin do not have sem_timedwait.  Implement a fallback
    for them.
    
    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
    Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

I'm going to assume this fallback impl is less efficient than the
native sem_t impl as the reason for leaving the original impl, or
maybe Paolo just want to risk accidental bugs by removing the
existing usage ?


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC 0/2] qemu-sem-posix: use monotonic clock instead

[Paolo Bonzini]

On 2/21/22 10:56, 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.

This looks good, I don't think there are cases where a more optimized 
semaphore is necessary (if there were, we could introduce a futex 
fallback on Linux).

However, pthread_condattr_t need not be in the struct.  The attributes 
can be allocated on the stack, because they do not have to remain alive 
after pthread_cond_init.

Thanks,

Paolo

> Longpeng (Mike) (2):
>    sem-posix: remove the posix semaphore support
>    sem-posix: use monotonic clock instead
> 
>   include/qemu/thread-posix.h |  5 +--
>   meson.build                 | 12 ++++++-
>   util/qemu-thread-posix.c    | 82 +++++++++++++++------------------------------
>   3 files changed, 39 insertions(+), 60 deletions(-)
> 

Re: [RFC 2/2] sem-posix: use monotonic clock instead

[Paolo Bonzini]

On 2/21/22 10:56, Longpeng(Mike) via wrote:
> +    long now_nsec;
> +#ifdef CONFIG_PTHREAD_CONDATTR_SETCLOCK
> +    struct timespec now;
> +    clock_gettime(CLOCK_MONOTONIC, &now);
> +    now_sec = now.tv_sec;
> +    now_nsec = now.tv_nsec;
> +#else
>       struct timeval tv;
>       gettimeofday(&tv, NULL);
> -    ts->tv_nsec = tv.tv_usec * 1000 + (ms % 1000) * 1000000;
> -    ts->tv_sec = tv.tv_sec + ms / 1000;
> +    now_sec = tv.tv_sec;
> +    now_nsec = tv.tv_usec * 1000;
> +#endif
> +

Perhaps this might minimize the amount of conditional code, too:

diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
index 903fa33965..4743d7b714 100644
--- a/util/qemu-thread-posix.c
+++ b/util/qemu-thread-posix.c
@@ -40,10 +40,14 @@ static void error_exit(int err, const char *msg)
  
  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;
+#ifdef CONFIG_PTHREAD_CONDATTR_SETCLOCK
+    clock_gettime(CLOCK_MONOTONIC, ts);
+#else
+    clock_gettime(CLOCK_REALTIME, ts);
+#endif
+
+    ts->tv_nsec += (ms % 1000) * 1000000;
+    ts->tv_sec += ms / 1000;
      if (ts->tv_nsec >= 1000000000) {
          ts->tv_sec++;
          ts->tv_nsec -= 1000000000;


Finally, the conditional variables initialization qemu_cond_init must
also use pthread_condattr_setclock.

Paolo

RE: [RFC 1/2] sem-posix: remove the posix semaphore support

[longpeng2--- via]

Hi Daniel,

> -----Original Message-----
> From: Daniel P. Berrangé [mailto:berrange@redhat.com]
> Sent: Monday, February 21, 2022 7:12 PM
> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
> <longpeng2@huawei.com>
> Cc: pbonzini@redhat.com; mst@redhat.com; qemu-devel@nongnu.org; Gonglei (Arei)
> <arei.gonglei@huawei.com>
> Subject: Re: [RFC 1/2] sem-posix: remove the posix semaphore support
> 
> On Mon, Feb 21, 2022 at 05:56:16PM +0800, Longpeng(Mike) via wrote:
> > 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 anymore.
> 
> It doesn't appear in any man pages on my systems, but there is a
> new-ish API  sem_clockwait() that accepts a choice of clock as a
> parameter.
> 
> This is apparently a proposed POSIX standard API introduced in
> glibc 2.30, along with several others:
> 

But the API is only supported in glibc.

https://www.gnu.org/software/gnulib/manual/html_node/sem_005fclockwait.html

> https://sourceware.org/legacy-ml/libc-announce/2019/msg00001.html
> 
> [quote]
> * Add new POSIX-proposed pthread_cond_clockwait, pthread_mutex_clocklock,
>   pthread_rwlock_clockrdlock, pthread_rwlock_clockwrlock and sem_clockwait
>   functions.  These behave similarly to their "timed" equivalents, but also
>   accept a clockid_t parameter to determine which clock their timeout should
>   be measured against.  All functions allow waiting against CLOCK_MONOTONIC
>   and CLOCK_REALTIME.  The decision of which clock to be used is made at the
>   time of the wait (unlike with pthread_condattr_setclock, which requires
>   the clock choice at initialization time).
> [/quote]
> 

It seems pthread_condattr_setclock() can meet our requirement here, it's OK
for us to choose the clock at initialization time.

> > 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;
> >  };
> 
> As a point of history, the original  code only used sem_t. The pthreads
> based fallback was introduced by Paolo in
> 
>   commit c166cb72f1676855816340666c3b618beef4b976
>   Author: Paolo Bonzini <pbonzini@redhat.com>
>   Date:   Fri Nov 2 15:43:21 2012 +0100
> 
>     semaphore: implement fallback counting semaphores with mutex+condvar
> 
>     OpenBSD and Darwin do not have sem_timedwait.  Implement a fallback
>     for them.
> 
>     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>     Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> I'm going to assume this fallback impl is less efficient than the
> native sem_t impl as the reason for leaving the original impl, or
> maybe Paolo just want to risk accidental bugs by removing the
> existing usage ?
> 

Paolo has replied, seems this change is acceptable, so I'll continue to
work on this solution. Thanks :)

> 
> Regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|

RE: [RFC 0/2] qemu-sem-posix: use monotonic clock instead

[longpeng2--- via]

> -----Original Message-----
> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
> Sent: Monday, February 21, 2022 7:31 PM
> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
> <longpeng2@huawei.com>; berrange@redhat.com; mst@redhat.com
> Cc: qemu-devel@nongnu.org; Gonglei (Arei) <arei.gonglei@huawei.com>
> Subject: Re: [RFC 0/2] qemu-sem-posix: use monotonic clock instead
> 
> On 2/21/22 10:56, 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.
> 
> This looks good, I don't think there are cases where a more optimized
> semaphore is necessary (if there were, we could introduce a futex
> fallback on Linux).
> 
> However, pthread_condattr_t need not be in the struct.  The attributes
> can be allocated on the stack, because they do not have to remain alive
> after pthread_cond_init.
> 

OK, will do in the next version, thanks!

> Thanks,
> 
> Paolo
> 
> > Longpeng (Mike) (2):
> >    sem-posix: remove the posix semaphore support
> >    sem-posix: use monotonic clock instead
> >
> >   include/qemu/thread-posix.h |  5 +--
> >   meson.build                 | 12 ++++++-
> >   util/qemu-thread-posix.c    | 82
> +++++++++++++++------------------------------
> >   3 files changed, 39 insertions(+), 60 deletions(-)
> >

RE: [RFC 2/2] sem-posix: use monotonic clock instead

[longpeng2--- via]

> -----Original Message-----
> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
> Sent: Monday, February 21, 2022 7:42 PM
> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
> <longpeng2@huawei.com>; berrange@redhat.com; mst@redhat.com
> Cc: qemu-devel@nongnu.org; Gonglei (Arei) <arei.gonglei@huawei.com>
> Subject: Re: [RFC 2/2] sem-posix: use monotonic clock instead
> 
> On 2/21/22 10:56, Longpeng(Mike) via wrote:
> > +    long now_nsec;
> > +#ifdef CONFIG_PTHREAD_CONDATTR_SETCLOCK
> > +    struct timespec now;
> > +    clock_gettime(CLOCK_MONOTONIC, &now);
> > +    now_sec = now.tv_sec;
> > +    now_nsec = now.tv_nsec;
> > +#else
> >       struct timeval tv;
> >       gettimeofday(&tv, NULL);
> > -    ts->tv_nsec = tv.tv_usec * 1000 + (ms % 1000) * 1000000;
> > -    ts->tv_sec = tv.tv_sec + ms / 1000;
> > +    now_sec = tv.tv_sec;
> > +    now_nsec = tv.tv_usec * 1000;
> > +#endif
> > +
> 
> Perhaps this might minimize the amount of conditional code, too:
> 
> diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
> index 903fa33965..4743d7b714 100644
> --- a/util/qemu-thread-posix.c
> +++ b/util/qemu-thread-posix.c
> @@ -40,10 +40,14 @@ static void error_exit(int err, const char *msg)
> 
>   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;
> +#ifdef CONFIG_PTHREAD_CONDATTR_SETCLOCK
> +    clock_gettime(CLOCK_MONOTONIC, ts);
> +#else
> +    clock_gettime(CLOCK_REALTIME, ts);
> +#endif
> +
> +    ts->tv_nsec += (ms % 1000) * 1000000;
> +    ts->tv_sec += ms / 1000;
>       if (ts->tv_nsec >= 1000000000) {
>           ts->tv_sec++;
>           ts->tv_nsec -= 1000000000;
> 
> 
> Finally, the conditional variables initialization qemu_cond_init must
> also use pthread_condattr_setclock.
> 

Make sense! Will optimize the code in the next version. Thanks.

> Paolo

Re: [RFC 1/2] sem-posix: remove the posix semaphore support

[Paolo Bonzini]

On 2/21/22 12:11, Daniel P. Berrangé wrote:
> As a point of history, the original  code only used sem_t. The pthreads
> based fallback was introduced by Paolo in
> 
>    commit c166cb72f1676855816340666c3b618beef4b976
>    Author: Paolo Bonzini <pbonzini@redhat.com>
>    Date:   Fri Nov 2 15:43:21 2012 +0100
> 
>      semaphore: implement fallback counting semaphores with mutex+condvar
>      
>      OpenBSD and Darwin do not have sem_timedwait.  Implement a fallback
>      for them.
>      
>      Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>      Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> I'm going to assume this fallback impl is less efficient than the
> native sem_t impl as the reason for leaving the original impl, or
> maybe Paolo just want to risk accidental bugs by removing the
> existing usage ?

Yes, it is a bit less efficient.  But really there aren't any places 
where semaphores vs. mutex+condvar will make a difference.  The original 
reason to use semaphores was that Windows had a hand-written condition 
variable implementation that didn't support cond_timedwait.

Paolo