Re: [PATCH 04/14] Disable elision for any pthread_mutexattr_settype call

[Adhemerval Zanella <azanella at linux dot vnet dot ibm dot com>]

Hi all,

I am bringing this up because this commit has changed a pthread API behavior.
Consider the following example:

---

//#define _GNU_SOURCE
#include <stdio.h>
#include <pthread.h>

int main ()
{
  static const struct {
    const char *name;
    int type;
  } mutex_types[] = {
    { "PTHREAD_MUTEX_TIMED_NP",      PTHREAD_MUTEX_TIMED_NP },
    { "PTHREAD_MUTEX_RECURSIVE_NP",  PTHREAD_MUTEX_RECURSIVE_NP },
    { "PTHREAD_MUTEX_ERRORCHECK_NP", PTHREAD_MUTEX_ERRORCHECK_NP },
    { "PTHREAD_MUTEX_ADAPTIVE_NP",   PTHREAD_MUTEX_ADAPTIVE_NP },
    { "PTHREAD_MUTEX_NORMAL",        PTHREAD_MUTEX_NORMAL },
    { "PTHREAD_MUTEX_RECURSIVE",     PTHREAD_MUTEX_RECURSIVE },
    { "PTHREAD_MUTEX_ERRORCHECK",    PTHREAD_MUTEX_ERRORCHECK },
    { "PTHREAD_MUTEX_DEFAULT",       PTHREAD_MUTEX_DEFAULT },
#ifdef _GNU_SOURCE
    { "PTHREAD_MUTEX_FAST_NP",       PTHREAD_MUTEX_FAST_NP },
#endif
  };
  static const int mutex_types_size = 
    sizeof (mutex_types) / sizeof (mutex_types[0]);
  int i;

  pthread_mutexattr_t attr;
  pthread_mutexattr_init (&attr);

  for (i=0; i<mutex_types_size; ++i)
    {
      int ret, type;

      pthread_mutexattr_settype (&attr, mutex_types[i].type);
      ret = pthread_mutexattr_gettype (&attr, &type);
      printf ("%s (%i) | %i | %i\n", mutex_types[i].name, ret, type,
	type == mutex_types[i].type);
    }

  return 0;
}
---

Without this commit, the result is:

PTHREAD_MUTEX_TIMED_NP (0) | 0 | 1
PTHREAD_MUTEX_RECURSIVE_NP (0) | 1 | 1
PTHREAD_MUTEX_ERRORCHECK_NP (0) | 2 | 1
PTHREAD_MUTEX_ADAPTIVE_NP (0) | 3 | 1
PTHREAD_MUTEX_NORMAL (0) | 0 | 1
PTHREAD_MUTEX_RECURSIVE (0) | 1 | 1
PTHREAD_MUTEX_ERRORCHECK (0) | 2 | 1
PTHREAD_MUTEX_DEFAULT (0) | 0 | 1

And after:

PTHREAD_MUTEX_TIMED_NP (0) | 512 | 0
PTHREAD_MUTEX_RECURSIVE_NP (0) | 1 | 1
PTHREAD_MUTEX_ERRORCHECK_NP (0) | 2 | 1
PTHREAD_MUTEX_ADAPTIVE_NP (0) | 3 | 1
PTHREAD_MUTEX_NORMAL (0) | 512 | 0
PTHREAD_MUTEX_RECURSIVE (0) | 1 | 1
PTHREAD_MUTEX_ERRORCHECK (0) | 2 | 1
PTHREAD_MUTEX_DEFAULT (0) | 512 | 0

This is being triggered in a valgrind testcase where it intercepts the set type pthread function
and get its type to check which function to call.  Although valgrind and any other code could
be adapted to check if bit 'PTHREAD_MUTEX_NO_ELISION_NP' is set, the straight forward comparison
(pthread_mutexattr_gettype (...) == PTHREAD_MUTEX_DEFAULT) does not work correctly.  Is is the
intended behavior now?


On 28-06-2013 04:11, Carlos O'Donell wrote:
> On 06/28/2013 01:51 AM, Andi Kleen wrote:
>> From: Andi Kleen <ak@linux.intel.com>
>>
>> PTHREAD_MUTEX_NORMAL requires deadlock for nesting, DEFAULT
>> does not. Since glibc uses the same value (0) disable elision
>> for any call to pthread_mutexattr_settype() with a 0 value.
>> This implies that a program can disable elision by doing
>> pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_NORMAL)
> This patch looks good.
>
> It disables elision for NORMAL-type locks which is what we want.
>
> This patch doesn't expose any new ABI or API.
>
> This should be OK to checkin IMO.
>
> Please let me review patch 5 before we do anything.
>
>> Based on a original proposal by Rich Felker.
>>
>> nptl/:
>> 2013-06-27  Andi Kleen  <ak@linux.intel.com>
>>
>> 	* pthread_mutexattr_settype.c (__pthread_mutexattr_settype):
>> 	Disable elision for PTHREAD_MUTEX_DEFAULT.
>> ---
>>  nptl/pthread_mutexattr_settype.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/nptl/pthread_mutexattr_settype.c b/nptl/pthread_mutexattr_settype.c
>> index 7b476e9..a85cdcb 100644
>> --- a/nptl/pthread_mutexattr_settype.c
>> +++ b/nptl/pthread_mutexattr_settype.c
>> @@ -30,6 +30,11 @@ __pthread_mutexattr_settype (attr, kind)
>>    if (kind < PTHREAD_MUTEX_NORMAL || kind > PTHREAD_MUTEX_ADAPTIVE_NP)
>>      return EINVAL;
>>  
>> +  /* Cannot distinguish between DEFAULT and NORMAL. So any settype
>> +     call disables elision for now.  */
>> +  if (kind == PTHREAD_MUTEX_DEFAULT)
>> +    kind |= PTHREAD_MUTEX_NO_ELISION_NP;
>> +
> Looks good to me and it's the minimal change required.
>
>>    iattr = (struct pthread_mutexattr *) attr;
>>  
>>    iattr->mutexkind = (iattr->mutexkind & PTHREAD_MUTEXATTR_FLAG_BITS) | kind;
>>
> Cheers,
> Carlos.
>