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

[longpeng2--- via <qemu-devel@nongnu.org>]

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 :|