Re: [PATCH] Add PTHREAD_MUTEX_NORMAL_INT

[Andi Kleen <andi at firstfloor dot org>]

On Mon, Jun 24, 2013 at 05:18:15PM -0400, Carlos O'Donell wrote:
> On 06/24/2013 04:31 PM, Andi Kleen wrote:
> > On Mon, Jun 24, 2013 at 04:25:52PM -0400, Carlos O'Donell wrote:
> >> On 06/24/2013 04:17 PM, Andi Kleen wrote:
> >>> From: Andi Kleen <ak@linux.intel.com>
> >>>
> >>> Carlos asked for PTHREAD_MUTEX_NORMAL_INT to be added
> >>> to make some of the internal macros be easier to understand.
> >>> It is always identical to DEFAULT, as NORMAL only makes
> >>> a difference for settype.
> >>
> >> Thanks for this.
> >>
> >> As it stands however, with pthread_mutexattr_gettype loosing 
> >> the type originally entered by the user, we are going to have to
> >> propagate the new type internally.
> > 
> > It will return DEFAULT|NO_ELISION, which is identical in behaviour
> > to NORMAL
> > 
> > So if you do 
> > 
> > settype(&a, NORMAL);
> > settype(&b, gettype(&a));
> > 
> > it will all work as expected. Even b will have all NORMAL semantics.
> > 
> > The only case that wouldn't work is someone explicitely rechecking
> > the value returned by gettype(). Is that really a problem?
> 
> Yes it is a problem.
> 
> It would violate the expected semantics of the interface.

Here is an incremential patch to fix this. It moves the 
conversion to pthread_mutex_init(), so get/set are symmetrical
on the attribute.

Let me know if I should repost the whole thing.

Subject: [PATCH] Fix pthread_mutexattr_gettype returning different type.

We move the NORMAL->DEFAULT conversion to pthread_mutex_init(),
so get/set on the mutex attribute structure works as expected.

2013-06-24  Andi Kleen  <ak@linux.intel.com>

        * pthread_mutexattr_settype.c (pthread_mutexattr_settype_worker):
        Remove PTHREAD_MUTEX_NORMAL -> DEFAULT conversion.
        (__pthread_mutexattr_settype_old): Set NORMAL flag
        * pthread_mutex_init.c (__pthread_mutex_init): ... and move to here.

diff --git a/nptl/pthread_mutex_init.c b/nptl/pthread_mutex_init.c
index f6f0f80..8b8fff8 100644
--- a/nptl/pthread_mutex_init.c
+++ b/nptl/pthread_mutex_init.c
@@ -128,6 +128,19 @@ __pthread_mutex_init (mutex, mutexattr)
   if ((imutexattr->mutexkind & (PTHREAD_MUTEXATTR_FLAG_PSHARED
 				| PTHREAD_MUTEXATTR_FLAG_ROBUST)) != 0)
     mutex->__data.__kind |= PTHREAD_MUTEX_PSHARED_BIT;
+  
+  /* When a NORMAL mutex is explicitly specified, default to no elision
+     to satisfy POSIX's deadlock requirement. Also convert the NORMAL
+     type to DEFAULT, as the rest of the lock library doesn't have
+     the code paths for them.  */
+  if ((mutex->__data.__kind & PTHREAD_MUTEX_KIND_MASK_NP) 
+      == PTHREAD_MUTEX_NORMAL)
+    {
+      if ((imutexattr->mutexkind & PTHREAD_MUTEX_ELISION_FLAGS_NP) == 0)
+	mutex->__data.__kind |= PTHREAD_MUTEX_NO_ELISION_NP;
+      mutex->__data.__kind = PTHREAD_MUTEX_DEFAULT
+	| (mutex->__data.__kind & PTHREAD_MUTEX_KIND_MASK_NP);
+    }
 
   /* Drop elision bits for any unusual flags, except for PSHARED.
      These can be set implicitely now, but the other code paths don't
diff --git a/nptl/pthread_mutexattr_settype.c b/nptl/pthread_mutexattr_settype.c
index ae1be45..d212148 100644
--- a/nptl/pthread_mutexattr_settype.c
+++ b/nptl/pthread_mutexattr_settype.c
@@ -36,17 +36,6 @@ pthread_mutexattr_settype_worker (pthread_mutexattr_t *attr, int kind)
   if ((kind & PTHREAD_MUTEX_ELISION_FLAGS_NP) == PTHREAD_MUTEX_ELISION_FLAGS_NP)
     return EINVAL;
 
-  /* When a NORMAL mutex is explicitly specified, default to no elision
-     to satisfy POSIX's deadlock requirement. Also convert the NORMAL
-     type to DEFAULT, as the rest of the lock library doesn't have
-     the code paths for them.  */
-  if (mkind == PTHREAD_MUTEX_NORMAL)
-    {
-      kind = PTHREAD_MUTEX_DEFAULT | (kind & PTHREAD_MUTEX_ELISION_FLAGS_NP);
-      if ((kind & PTHREAD_MUTEX_ELISION_FLAGS_NP) == 0)
-        kind |= PTHREAD_MUTEX_NO_ELISION_NP;
-    }
-
   /* When the CPU does not support elision never allow to set the elision
      flags.  */
   if ((kind & PTHREAD_MUTEX_ELISION_FLAGS_NP) && !ENABLE_ELISION)
@@ -82,10 +71,10 @@ int
 attribute_compat_text_section
 __pthread_mutexattr_settype_old (pthread_mutexattr_t *attr, int kind)
 {
-  /* Force no elision for the old ambigious DEFAULT/NORMAL
-     kind.  */
+  /* Force NORMAL (= no elision) for the old ambigious
+     DEFAULT/NORMAL kind.  */
   if (kind == PTHREAD_MUTEX_DEFAULT)
-    kind |= PTHREAD_MUTEX_NO_ELISION_NP;
+    kind |= PTHREAD_MUTEX_NORMAL;
   return pthread_mutexattr_settype_worker (attr, kind);
 }