Re: [PATCH] Add PTHREAD_MUTEX_NORMAL_INT

["Carlos O'Donell" <carlos at redhat dot com>]

On 06/24/2013 07:26 PM, Andi Kleen wrote:
>>> 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.

So I've done some more work on this last night.

The problem with continuing down this path is that it creates an ABI
event by exposing NORMAL as a new external type.

I looked at the generated code for adding NORMAL as an internal type
and ran some crude benchmarks and the performance difference is in the
noise.

What I'm trying to balance is:

* Avoid ABI changes in this first pass.

vs.

* Performance.

Once we make NORMAL a new external type, it's there forever.

If we use two internal types, we don't introduce an ABI, but we have
a slight performance difference. The win, that I didn't think about 
though is that it's easy to review and make sure no ABI has changed.
In the future we can get the performance back if want to by merging
the internal types and applying a patch just like this one.

Thus this patch I think has to go into a second patchset that changes
the ABI/API.

We need a patch with internal types that doesn't change the external
ABI, but still allows us to elide DEFAULT mutexes.

I'll still review this patch because it's going to be the way forward
to add a new external type and gain back any lost performance.

> 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);

This assignment clears all of the flags that were copied into __kind?

It also treats the mutex types as bit flags, but they aren't, they 
are values.

Why not:

mutex->__data.__kind &= ~PTHREAD_MUTEX_KIND_MASK_NP;
mutex->__data.__kind |= PTHREAD_MUTEX_DEFAULT;


> +    }
>  
>    /* 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;
> -    }
> -

OK. Note this code only uses |= and therefore doesn't have the same problem
as above.

>    /* 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;

You can't do this without assuming DEFAULT is zero.

I think you should clear with the KIND mask and then set NORMAL, and
let the compiler figure it out, *or* assert, *or* a big comment.

>    return pthread_mutexattr_settype_worker (attr, kind);
>  }
>  
> 

Cheers,
Carlos.