| Message ID | 20220221095617.1974-2-longpeng2@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qemu-sem-posix: use monotonic clock instead | expand |
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
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 :|
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
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__
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(-)