diff mbox

[ovs-dev] Windows tests: Applications abort when using threading (ovs_rwlock_init)

Message ID 20170104172042.GP2016@ovn.org
State Not Applicable
Headers show

Commit Message

Ben Pfaff Jan. 4, 2017, 5:20 p.m. UTC 
On Wed, Dec 28, 2016 at 10:27:17PM +0000, Alin Serdean wrote:
> Commit number: 1a15f390afd66efd161db78b86600832214dfb3c
> https://github.com/openvswitch/ovs/commit/1a15f390afd66efd161db78b86600832214dfb3c
> switched from `NULL` to `attr`. According to POSIX documentation this is
> correct, unfortunately on Windows the current implementation of pthreads does
> not support a pre-initialized attribute. Please see a fork of the implementation
> https://github.com/GerHobbelt/pthread-win32/blob/19fd5054b29af1b4e3b3278bfffbb6274c6c89f5/pthread_rwlock_init.c#L59-L63
> This is the same implementation as the official version found under:
> ftp://sourceware.org/pub/pthreads-win32/)

Thanks.

I didn't like that this was Windows-specific, since it was possible to
make it generic.

I changed the patch to the following, and applied it to master.

--8<--------------------------cut here-------------------------->8--

From: Alin Serdean <aserdean@cloudbasesolutions.com>
Date: Wed, 28 Dec 2016 22:27:17 +0000
Subject: [PATCH] ovs-thread: Avoid pthread_rwlockattr_t on Windows.

A recent commit fixed ovs_rwlock_init() to pass the pthread_rwlockattr_t
that it initialized to pthread_rwlock_init().  According to POSIX
documentation this is correct, but on Windows the current implementation of
pthreads does not support a pre-initialized attribute.  Please see a fork
of the implementation
https://github.com/GerHobbelt/pthread-win32/blob/19fd5054b29af1b4e3b3278bfffbb6274c6c89f5/pthread_rwlock_init.c#L59-L63
This is the same implementation as the official version found under:
ftp://sourceware.org/pub/pthreads-win32/)

A short debug output from `vswitch` to confirm the above:

>k
 Index  Function
--------------------------------------------------------------------------------
*1      ovs-vswitchd.exe!ovs_rwlock_init(const ovs_rwlock * l_=0x000001721c7da250)
 2      ovs-vswitchd.exe!open_dpif_backer(const char * type=0x000001721c7d8d60, dpif_backer * * backerp=0x000001721c7d89c0)
 3      ovs-vswitchd.exe!construct(ofproto * ofproto_=0x000001721c7d87d0)
 4      ovs-vswitchd.exe!ofproto_create(const char * datapath_name=0x000001721c7d86e0, const char * datapath_type=0x000001721c7d8750, ofproto * * ofprotop=0x000001721c7d80b8)
 5      ovs-vswitchd.exe!bridge_reconfigure(const ovsrec_open_vswitch * ovs_cfg=0x000001721c7e05b0)
 6      ovs-vswitchd.exe!bridge_run()
 7      ovs-vswitchd.exe!main(int argc=6, char * * argv=0x000001721c729e10)
 8      [External Code]

>? error
22
https://github.com/openvswitch/ovs/blob/master/lib/ovs-thread.c#L243

This patch is critical because the majority (over 800) of the unit tests
are failing.

Fixes: 1a15f390afd6 ("lib/ovs-thread: set prefer writer lock for ovs_rwlock_init()")
Signed-off-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>
Acked-by: Shashank Ram <rams@vmware.com>
[blp@ovn.org changed the details of the approach]
Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 lib/ovs-thread.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

Comments

Alin Serdean Jan. 5, 2017, 5:37 p.m. UTC | #1 
> -----Original Message-----
> From: Ben Pfaff [mailto:blp@ovn.org]
> Sent: Wednesday, January 4, 2017 7:21 PM
> To: Alin Serdean <aserdean@cloudbasesolutions.com>
> Cc: dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH] Windows tests: Applications abort when
> using threading (ovs_rwlock_init)
> 
> On Wed, Dec 28, 2016 at 10:27:17PM +0000, Alin Serdean wrote:
> > Commit number: 1a15f390afd66efd161db78b86600832214dfb3c
> >
> https://github.com/openvswitch/ovs/commit/1a15f390afd66efd161db78b86
> 60
> > 0832214dfb3c switched from `NULL` to `attr`. According to POSIX
> > documentation this is correct, unfortunately on Windows the current
> > implementation of pthreads does not support a pre-initialized
> > attribute. Please see a fork of the implementation
> > https://github.com/GerHobbelt/pthread-
> win32/blob/19fd5054b29af1b4e3b32
> > 78bfffbb6274c6c89f5/pthread_rwlock_init.c#L59-L63
> > This is the same implementation as the official version found under:
> > ftp://sourceware.org/pub/pthreads-win32/)
> 
> Thanks.
> 
> I didn't like that this was Windows-specific, since it was possible to make it
> generic.
> 
> I changed the patch to the following, and applied it to master.
> 
[Alin Serdean] Much nicer. Thanks!
diff mbox

Patch

diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c
index 058c434..1f4995b 100644
--- a/lib/ovs-thread.c
+++ b/lib/ovs-thread.c
@@ -230,21 +230,27 @@  void
 ovs_rwlock_init(const struct ovs_rwlock *l_)
 {
     struct ovs_rwlock *l = CONST_CAST(struct ovs_rwlock *, l_);
-    pthread_rwlockattr_t attr;
     int error;
 
     l->where = "<unlocked>";
 
-    xpthread_rwlockattr_init(&attr);
 #ifdef PTHREAD_RWLOCK_WRITER_NONRECURSIVE_INITIALIZER_NP
+    pthread_rwlockattr_t attr;
+    xpthread_rwlockattr_init(&attr);
     xpthread_rwlockattr_setkind_np(
         &attr, PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP);
-#endif
     error = pthread_rwlock_init(&l->lock, &attr);
+    xpthread_rwlockattr_destroy(&attr);
+#else
+    /* It is important to avoid passing a rwlockattr in this case because
+     * Windows pthreads 2.9.1 (and earlier) fail and abort if passed one, even
+     * one without any special attributes. */
+    error = pthread_rwlock_init(&l->lock, NULL);
+#endif
+
     if (OVS_UNLIKELY(error)) {
         ovs_abort(error, "pthread_rwlock_init failed");
     }
-    xpthread_rwlockattr_destroy(&attr);
 }
 
 /* Provides an error-checking wrapper around pthread_cond_wait().