diff mbox

Remove __PTHREAD_MUTEX_HAVE_ELISION undefined warning

Message ID 53330F3B.6090009@linux.vnet.ibm.com
State New
Headers show

Commit Message

Adhemerval Zanella March 26, 2014, 5:32 p.m. UTC 
On 26-03-2014 13:36, Roland McGrath wrote:
>> +#if !defined _PTHREAD_H
> This is OK but there is no reason not to use #ifndef.
>
>> +# error "Never include this file directly. Use <pthread.h> instead"
> Two spaces between the sentences, and a period at the end of each.
>
> (Those two also apply to the sysdeps variant files you're adding, of course.)
>
>> +#define __PTHREAD_MUTEX_HAVE_ELISION 0
> As Joseph also requested, add an explanatory comment describing the
> protocol for this macro and how other sysdeps variant files might define
> it.
>
>> +#endif
>> diff --git a/nptl/sysdeps/pthread/pthread.h b/nptl/sysdeps/pthread/pthread.h
>> index 1e0c5dc..39d6b91 100644
>> --- a/nptl/sysdeps/pthread/pthread.h
>> +++ b/nptl/sysdeps/pthread/pthread.h
>> @@ -26,6 +26,7 @@
>>  #include <bits/pthreadtypes.h>
>>  #include <bits/setjmp.h>
>>  #include <bits/wordsize.h>
>> +#include <bits/pthread-elision.h>
> I'd put it right after bits/pthreadtypes.h.
>
>> --- a/posix/Makefile
>> +++ b/posix/Makefile
>> @@ -29,7 +29,7 @@ headers	:= sys/utsname.h sys/times.h sys/wait.h sys/types.h unistd.h	      \
>>  	   bits/local_lim.h tar.h bits/utsname.h bits/confname.h	      \
>>  	   bits/waitflags.h bits/waitstatus.h sys/unistd.h sched.h	      \
>>  	   bits/sched.h re_comp.h wait.h bits/environments.h cpio.h	      \
>> -	   sys/sysmacros.h spawn.h bits/unistd.h
>> +	   sys/sysmacros.h spawn.h bits/unistd.h bits/pthread-elision.h
> That belongs in nptl/Makefile.
>
>
> Thanks,
> Roland
>
Thanks for the review, what about this version (fixed all the comments):

--

	* nptl/sysdeps/pthread/bits/pthread-elision.h: New header: define
	default lock elision support and defines.
	* nptl/sysdeps/pthread/pthread.h: Include pthread-elision.h.
	* nptl/sysdeps/unix/sysv/linux/s390/bits/pthreadtypes.h
	(__PTHREAD_MUTEX_HAVE_ELISION): Undefine.
	* nptl/sysdeps/unix/sysv/linux/x86/bits/pthread-elision.h: New
	header: x86 specific lock elision support.
	* nptl/sysdeps/unix/sysv/linux/x86/bits/pthreadtypes.h:
	(__PTHREAD_MUTEX_HAVE_ELISION): Definition moved to specific lock
	elision header.
	* nptl/Makefile: Add bits/pthread-elision.h to install.

---

Comments

Roland McGrath March 26, 2014, 5:50 p.m. UTC | #1 
> 	* nptl/sysdeps/pthread/bits/pthread-elision.h: New header: define
> 	default lock elision support and defines.
> 	* nptl/sysdeps/pthread/pthread.h: Include pthread-elision.h.

Say bits/pthread-elision.h, or since it follows the addition, just "it".

> 	* nptl/sysdeps/unix/sysv/linux/s390/bits/pthreadtypes.h
> 	(__PTHREAD_MUTEX_HAVE_ELISION): Undefine.

Say "Macro removed."  Saying "Undefine" implies #undef.

> 	* nptl/sysdeps/unix/sysv/linux/x86/bits/pthread-elision.h: New
> 	header: x86 specific lock elision support.
> 	* nptl/sysdeps/unix/sysv/linux/x86/bits/pthreadtypes.h:

No colon at the end of the line when there are (...) sections in the entry.

> 	(__PTHREAD_MUTEX_HAVE_ELISION): Definition moved to specific lock
> 	elision header.

That's fine, but canonical style is:

	* nptl/sysdeps/unix/sysv/linux/x86/bits/pthreadtypes.h
	(__PTHREAD_MUTEX_HAVE_ELISION): Macro moved to ...
	* nptl/sysdeps/unix/sysv/linux/x86/bits/pthread-elision.h:
	... this new file.

> 	* nptl/Makefile: Add bits/pthread-elision.h to install.

Should be:

	* nptl/Makefile (headers): Add bits/pthread-elision.h.

But I'd put this right after the pthread.h log line, and just say, "Add it."

> +#ifndef _PTHREAD_H
> +# error "Never include this file directly.  Use <pthread.h> instead"
> +#endif

Needs a trailing period.

> +/* Define it if the architecture supports lock elision using transactional
> +   memory or a similar facility.  It changes the mutex initializers to add
> +   the required elision fields.  Currently, three value are possible:
> +   * 0: No elision support, default value.
> +   * 1: Elision support for 64 bits.
> +   * 2: Elision support for 32 bits.  */
> +#define __PTHREAD_MUTEX_HAVE_ELISION 0

s/value/values/

I don't understand what "support for 64 bits" or "support for 32 bits"
means.  OK, I've looked at bits/pthreadtypes.h so I do understand.  But it
seems pretty wrong to pretend this is a generic 32/64 sort of thing when it
is really just about the x86-private layout of pthread_mutex_t.  It seems
more proper to have bits/pthreadtypes.h just define __PTHREAD_SPINS.  

That can be a separate cleanup if you want, and others may want to kibitz.
But that might involve dropping the header you're adding here, so maybe you
want to just resolve it now.

If you want to go ahead with this change, then it's OK with the other nits
above and this comment rewritten to describe concretely what the macro
means.  In actual usage so far, it doesn't actually mean anything about
elision support per se.  It just means something about how the fields of
pthread_mutex_t are structured and hence what the initializer must look like.
If that's all it's for, it should be made clear.


Thanks,
Roland
diff mbox

Patch

diff --git a/nptl/Makefile b/nptl/Makefile
index 897ac96..2c79023 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -22,7 +22,7 @@  subdir	:= nptl
 
 include ../Makeconfig
 
-headers := pthread.h semaphore.h bits/semaphore.h
+headers := pthread.h semaphore.h bits/semaphore.h bits/pthread-elision.h
 
 extra-libs := libpthread
 extra-libs-others := $(extra-libs)
diff --git a/nptl/sysdeps/pthread/bits/pthread-elision.h b/nptl/sysdeps/pthread/bits/pthread-elision.h
new file mode 100644
index 0000000..c2ea989
--- /dev/null
+++ b/nptl/sysdeps/pthread/bits/pthread-elision.h
@@ -0,0 +1,34 @@ 
+/* Lock elision default definitions.
+   Copyright (C) 2014 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#ifndef _PTHREAD_ELISION_H
+#define _PTHREAD_ELISION_H	1
+
+#ifndef _PTHREAD_H
+# error "Never include this file directly.  Use <pthread.h> instead"
+#endif
+
+/* Define it if the architecture supports lock elision using transactional
+   memory or a similar facility.  It changes the mutex initializers to add
+   the required elision fields.  Currently, three value are possible:
+   * 0: No elision support, default value.
+   * 1: Elision support for 64 bits.
+   * 2: Elision support for 32 bits.  */
+#define __PTHREAD_MUTEX_HAVE_ELISION 0
+
+#endif
diff --git a/nptl/sysdeps/pthread/pthread.h b/nptl/sysdeps/pthread/pthread.h
index 1e0c5dc..47e42c6 100644
--- a/nptl/sysdeps/pthread/pthread.h
+++ b/nptl/sysdeps/pthread/pthread.h
@@ -24,6 +24,7 @@ 
 #include <time.h>
 
 #include <bits/pthreadtypes.h>
+#include <bits/pthread-elision.h>
 #include <bits/setjmp.h>
 #include <bits/wordsize.h>
 
diff --git a/nptl/sysdeps/unix/sysv/linux/s390/bits/pthreadtypes.h b/nptl/sysdeps/unix/sysv/linux/s390/bits/pthreadtypes.h
index 23a1698..a361db6 100644
--- a/nptl/sysdeps/unix/sysv/linux/s390/bits/pthreadtypes.h
+++ b/nptl/sysdeps/unix/sysv/linux/s390/bits/pthreadtypes.h
@@ -20,8 +20,6 @@ 
 
 #include <bits/wordsize.h>
 
-# define __PTHREAD_MUTEX_HAVE_ELISION   0
-
 #if __WORDSIZE == 64
 # define __SIZEOF_PTHREAD_ATTR_T 56
 # define __SIZEOF_PTHREAD_MUTEX_T 40
diff --git a/nptl/sysdeps/unix/sysv/linux/x86/bits/pthread-elision.h b/nptl/sysdeps/unix/sysv/linux/x86/bits/pthread-elision.h
new file mode 100644
index 0000000..3ab29c4
--- /dev/null
+++ b/nptl/sysdeps/unix/sysv/linux/x86/bits/pthread-elision.h
@@ -0,0 +1,32 @@ 
+/* x86 lock elision definitions.
+   Copyright (C) 2014 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#ifndef _PTHREAD_ELISION_H
+#define _PTHREAD_ELISION_H	1
+
+#ifndef _PTHREAD_H
+# error "Never include this file directly.  Use <pthread.h> instead"
+#endif
+
+#ifdef __x86_64__
+# define __PTHREAD_MUTEX_HAVE_ELISION   1
+#else
+# define __PTHREAD_MUTEX_HAVE_ELISION   2
+#endif
+
+#endif
diff --git a/nptl/sysdeps/unix/sysv/linux/x86/bits/pthreadtypes.h b/nptl/sysdeps/unix/sysv/linux/x86/bits/pthreadtypes.h
index 28e5144..a5363ec 100644
--- a/nptl/sysdeps/unix/sysv/linux/x86/bits/pthreadtypes.h
+++ b/nptl/sysdeps/unix/sysv/linux/x86/bits/pthreadtypes.h
@@ -105,7 +105,6 @@  typedef union
     short __elision;
     __pthread_list_t __list;
 # define __PTHREAD_MUTEX_HAVE_PREV	1
-# define __PTHREAD_MUTEX_HAVE_ELISION   1
 #else
     unsigned int __nusers;
     __extension__ union
@@ -116,7 +115,6 @@  typedef union
 	short __elision;
 # define __spins d.__espins
 # define __elision d.__elision
-# define __PTHREAD_MUTEX_HAVE_ELISION   2
       } d;
       __pthread_slist_t __list;
     };