1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/3] Introduce RW locks to virDomainObjList
  4. View patch

[libvirt] [PATCH 1/3] virthread: Introduce virRWLockInitPreferWriter

Michal Privoznik posted 3 patches 7 years, 10 months ago
[libvirt] [PATCH 1/3] virthread: Introduce virRWLockInitPreferWriter
Posted by Michal Privoznik 7 years, 10 months ago 
We already have virRWLockInit. But this uses pthread defaults
which prefer reader to initialize the RW lock. This may lead to
writer starvation. Therefore we need to have the counterpart that
prefers writers. Now, according to the
pthread_rwlockattr_setkind_np() man page setting
PTHREAD_RWLOCK_PREFER_WRITER_NP attribute is no-op. Therefore we
need to use PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP
attribute. So much for good enum value names.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/libvirt_private.syms |  1 +
 src/util/virthread.c     | 35 +++++++++++++++++++++++++++++++++++
 src/util/virthread.h     |  1 +
 3 files changed, 37 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 187b12b32..a792e00c8 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2728,6 +2728,7 @@ virMutexUnlock;
 virOnce;
 virRWLockDestroy;
 virRWLockInit;
+virRWLockInitPreferWriter;
 virRWLockRead;
 virRWLockUnlock;
 virRWLockWrite;
diff --git a/src/util/virthread.c b/src/util/virthread.c
index 6c495158f..a8dd72f8b 100644
--- a/src/util/virthread.c
+++ b/src/util/virthread.c
@@ -95,6 +95,15 @@ void virMutexUnlock(virMutexPtr m)
 }
 
 
+/**
+ * virRWLockInit:
+ * @m: rwlock to init
+ *
+ * Initializes RW lock using pthread default attributes (which
+ * is PTHREAD_RWLOCK_PREFER_READER_NP).
+ *
+ * Returns 0 on success, -1 otherwise.
+ */
 int virRWLockInit(virRWLockPtr m)
 {
     int ret;
@@ -106,6 +115,32 @@ int virRWLockInit(virRWLockPtr m)
     return 0;
 }
 
+
+/**
+ * virRWLockInitPreferWriter:
+ * @m: rwlock to init
+ *
+ * Initializes RW lock which prefers writers over readers.
+ *
+ * Returns 0 on success, -1 otherwise.
+ */
+int virRWLockInitPreferWriter(virRWLockPtr m)
+{
+    int ret;
+    pthread_rwlockattr_t attr;
+
+    pthread_rwlockattr_init(&attr);
+    pthread_rwlockattr_setkind_np(&attr, PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP);
+    ret = pthread_rwlock_init(&m->lock, &attr);
+    pthread_rwlockattr_destroy(&attr);
+    if (ret != 0) {
+        errno = ret;
+        return -1;
+    }
+    return 0;
+}
+
+
 void virRWLockDestroy(virRWLockPtr m)
 {
     pthread_rwlock_destroy(&m->lock);
diff --git a/src/util/virthread.h b/src/util/virthread.h
index e466d9bf0..18b785af2 100644
--- a/src/util/virthread.h
+++ b/src/util/virthread.h
@@ -136,6 +136,7 @@ void virMutexUnlock(virMutexPtr m);
 
 
 int virRWLockInit(virRWLockPtr m) ATTRIBUTE_RETURN_CHECK;
+int virRWLockInitPreferWriter(virRWLockPtr m) ATTRIBUTE_RETURN_CHECK;
 void virRWLockDestroy(virRWLockPtr m);
 
 void virRWLockRead(virRWLockPtr m);
-- 
2.13.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] virthread: Introduce virRWLockInitPreferWriter
Posted by Pavel Hrdina 7 years, 9 months ago 
On Wed, Jul 19, 2017 at 04:31:48PM +0200, Michal Privoznik wrote:
> We already have virRWLockInit. But this uses pthread defaults
> which prefer reader to initialize the RW lock. This may lead to
> writer starvation. Therefore we need to have the counterpart that
> prefers writers. Now, according to the
> pthread_rwlockattr_setkind_np() man page setting
> PTHREAD_RWLOCK_PREFER_WRITER_NP attribute is no-op. Therefore we
> need to use PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP
> attribute. So much for good enum value names.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/libvirt_private.syms |  1 +
>  src/util/virthread.c     | 35 +++++++++++++++++++++++++++++++++++
>  src/util/virthread.h     |  1 +
>  3 files changed, 37 insertions(+)

Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] virthread: Introduce virRWLockInitPreferWriter
Posted by John Ferlan 7 years, 9 months ago 

On 07/19/2017 10:31 AM, Michal Privoznik wrote:
> We already have virRWLockInit. But this uses pthread defaults
> which prefer reader to initialize the RW lock. This may lead to
> writer starvation. Therefore we need to have the counterpart that
> prefers writers. Now, according to the
> pthread_rwlockattr_setkind_np() man page setting
> PTHREAD_RWLOCK_PREFER_WRITER_NP attribute is no-op. Therefore we
> need to use PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP
> attribute. So much for good enum value names.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/libvirt_private.syms |  1 +
>  src/util/virthread.c     | 35 +++++++++++++++++++++++++++++++++++
>  src/util/virthread.h     |  1 +
>  3 files changed, 37 insertions(+)
> 

This has broken the CI build, freebsd is not happy:

../../src/util/virthread.c:133:42: error: use of undeclared identifier
'PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP'
    pthread_rwlockattr_setkind_np(&attr,
PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP);
                                         ^
1 error generated.


John

You know what my suggestion is ;-)


> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 187b12b32..a792e00c8 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2728,6 +2728,7 @@ virMutexUnlock;
>  virOnce;
>  virRWLockDestroy;
>  virRWLockInit;
> +virRWLockInitPreferWriter;
>  virRWLockRead;
>  virRWLockUnlock;
>  virRWLockWrite;
> diff --git a/src/util/virthread.c b/src/util/virthread.c
> index 6c495158f..a8dd72f8b 100644
> --- a/src/util/virthread.c
> +++ b/src/util/virthread.c
> @@ -95,6 +95,15 @@ void virMutexUnlock(virMutexPtr m)
>  }
>  
>  
> +/**
> + * virRWLockInit:
> + * @m: rwlock to init
> + *
> + * Initializes RW lock using pthread default attributes (which
> + * is PTHREAD_RWLOCK_PREFER_READER_NP).
> + *
> + * Returns 0 on success, -1 otherwise.
> + */
>  int virRWLockInit(virRWLockPtr m)
>  {
>      int ret;
> @@ -106,6 +115,32 @@ int virRWLockInit(virRWLockPtr m)
>      return 0;
>  }
>  
> +
> +/**
> + * virRWLockInitPreferWriter:
> + * @m: rwlock to init
> + *
> + * Initializes RW lock which prefers writers over readers.
> + *
> + * Returns 0 on success, -1 otherwise.
> + */
> +int virRWLockInitPreferWriter(virRWLockPtr m)
> +{
> +    int ret;
> +    pthread_rwlockattr_t attr;
> +
> +    pthread_rwlockattr_init(&attr);
> +    pthread_rwlockattr_setkind_np(&attr, PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP);
> +    ret = pthread_rwlock_init(&m->lock, &attr);
> +    pthread_rwlockattr_destroy(&attr);
> +    if (ret != 0) {
> +        errno = ret;
> +        return -1;
> +    }
> +    return 0;
> +}
> +
> +
>  void virRWLockDestroy(virRWLockPtr m)
>  {
>      pthread_rwlock_destroy(&m->lock);
> diff --git a/src/util/virthread.h b/src/util/virthread.h
> index e466d9bf0..18b785af2 100644
> --- a/src/util/virthread.h
> +++ b/src/util/virthread.h
> @@ -136,6 +136,7 @@ void virMutexUnlock(virMutexPtr m);
>  
>  
>  int virRWLockInit(virRWLockPtr m) ATTRIBUTE_RETURN_CHECK;
> +int virRWLockInitPreferWriter(virRWLockPtr m) ATTRIBUTE_RETURN_CHECK;
>  void virRWLockDestroy(virRWLockPtr m);
>  
>  void virRWLockRead(virRWLockPtr m);
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] virthread: Introduce virRWLockInitPreferWriter
Posted by Michal Privoznik 7 years, 9 months ago 
On 07/24/2017 07:12 PM, John Ferlan wrote:
> 
> 
> On 07/19/2017 10:31 AM, Michal Privoznik wrote:
>> We already have virRWLockInit. But this uses pthread defaults
>> which prefer reader to initialize the RW lock. This may lead to
>> writer starvation. Therefore we need to have the counterpart that
>> prefers writers. Now, according to the
>> pthread_rwlockattr_setkind_np() man page setting
>> PTHREAD_RWLOCK_PREFER_WRITER_NP attribute is no-op. Therefore we
>> need to use PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP
>> attribute. So much for good enum value names.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/libvirt_private.syms |  1 +
>>  src/util/virthread.c     | 35 +++++++++++++++++++++++++++++++++++
>>  src/util/virthread.h     |  1 +
>>  3 files changed, 37 insertions(+)
>>
> 
> This has broken the CI build, freebsd is not happy:
> 
> ../../src/util/virthread.c:133:42: error: use of undeclared identifier
> 'PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP'
>     pthread_rwlockattr_setkind_np(&attr,
> PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP);
>                                          ^
> 1 error generated.
> 
> 
> John
> 
> You know what my suggestion is ;-)

Actually, I don't. I am trying to search freebsd documentation on
initializing RW locks so that they prefer writers. But no luck so far.
Does anybody here on the list know? Although, for the current usage of
RW locks it's really hard to starve a writer. But if this is going to be
used more broadly it is going to be easier. On the other hand, I just
realized that while one can use mutexes + conditions (like we do for
domain jobs), using RW locks and conditions is not implemented anywhere,
so I will have to come up with some idea.
Long story short, for now we don't care if writer will starve.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] virthread: Introduce virRWLockInitPreferWriter
Posted by Daniel P. Berrange 7 years, 9 months ago 
On Mon, Jul 24, 2017 at 01:12:59PM -0400, John Ferlan wrote:
> 
> 
> On 07/19/2017 10:31 AM, Michal Privoznik wrote:
> > We already have virRWLockInit. But this uses pthread defaults
> > which prefer reader to initialize the RW lock. This may lead to
> > writer starvation. Therefore we need to have the counterpart that
> > prefers writers. Now, according to the
> > pthread_rwlockattr_setkind_np() man page setting
> > PTHREAD_RWLOCK_PREFER_WRITER_NP attribute is no-op. Therefore we
> > need to use PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP
> > attribute. So much for good enum value names.
> > 
> > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > ---
> >  src/libvirt_private.syms |  1 +
> >  src/util/virthread.c     | 35 +++++++++++++++++++++++++++++++++++
> >  src/util/virthread.h     |  1 +
> >  3 files changed, 37 insertions(+)
> > 
> 
> This has broken the CI build, freebsd is not happy:
> 
> ../../src/util/virthread.c:133:42: error: use of undeclared identifier
> 'PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP'
>     pthread_rwlockattr_setkind_np(&attr,
> PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP);
>                                          ^
> 1 error generated.

It is not just FreeBSD, it also breaks OS-X and Win32.

The suffix '_np' / '_NP' is shorthand for nNon portable" - these
are glibc inventions. IMHO we should not really use this in our
code as if we're going to make assumptions that writers are not
starved as a result, because writers will be starved on any
non-Linux platform.

IOW, I think we need to just revert this.


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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.