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
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 :|
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
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 :|
© 2016 - 2026 Red Hat, Inc.