[1/2] nptl: Remove __ASSUME_SET_ROBUST_LIST

Message ID 1492550000-31374-1-git-send-email-adhemerval.zanella@linaro.org
State New
Headers show
Series
  • [1/2] nptl: Remove __ASSUME_SET_ROBUST_LIST
Related show

Commit Message

Adhemerval Zanella April 18, 2017, 9:13 p.m.
This is another patch that was in my backlog, so I am sending it again
on the list since first version was send almost 5 months ago [1].

--

This patch removes the __ASSUME_SET_ROBUST_LIST usage on nptl generic
code.  The set_robust_list availability is defined by '__set_robust_list_avail'
which is now defined regardless.  Its initial value is set to -1 and
defined to a positive value if both __NR_set_robust_list is defined
and the syscall returns correctly.

A subsequent patch is intended to remove the Linux definitions of
__ASSUME_SET_ROBUST_LIST.

Tested on x86_64.

	* nptl/nptl-init.c (set_robust_list_not_avail): Remove definition.
	(__pthread_initialize_minimal_internal): Set __set_robust_list_avail
	to 1 if syscall returns correctly.
	(__set_robust_list_avail): Define regardless if
	__ASSUME_SET_ROBUST_LIST is defined or not.
	* nptl/pthreadP.h (__set_robust_list_avail): Likewise.
	* nptl/pthread_create.c (START_THREAD_DEFN): Remove
	__ASSUME_SET_ROBUST_LIST usage.
	* nptl/pthread_mutex_init.c (__pthread_mutex_init): Likewise.

[1] https://sourceware.org/ml/libc-alpha/2016-09/msg00373.html
---
 nptl/nptl-init.c          | 12 +++---------
 nptl/pthreadP.h           |  2 --
 nptl/pthread_create.c     |  8 ++------
 nptl/pthread_mutex_init.c |  2 --
 4 files changed, 5 insertions(+), 19 deletions(-)

-- 
2.7.4

Comments

Florian Weimer April 19, 2017, 5:36 a.m. | #1
On 04/18/2017 11:13 PM, Adhemerval Zanella wrote:
> This patch removes the __ASSUME_SET_ROBUST_LIST usage on nptl generic

> code.  The set_robust_list availability is defined by '__set_robust_list_avail'

> which is now defined regardless.  Its initial value is set to -1 and

> defined to a positive value if both __NR_set_robust_list is defined

> and the syscall returns correctly.


I think we should drop support for kernels without set_robust_list.  We 
really need something like robust mutex support for liveness detection 
of the nscd mappings.  Making POSIX robust mutexes conditionally 
supported is awkward, and the fact that some architectures may lack 
support for them is undocumented.  Application programmers will not 
expect this.

Thanks,
Florian
Adhemerval Zanella April 19, 2017, 2:17 p.m. | #2
On 19/04/2017 02:36, Florian Weimer wrote:
> On 04/18/2017 11:13 PM, Adhemerval Zanella wrote:

>> This patch removes the __ASSUME_SET_ROBUST_LIST usage on nptl generic

>> code.  The set_robust_list availability is defined by '__set_robust_list_avail'

>> which is now defined regardless.  Its initial value is set to -1 and

>> defined to a positive value if both __NR_set_robust_list is defined

>> and the syscall returns correctly.

> 

> I think we should drop support for kernels without set_robust_list.  We really need something like robust mutex support for liveness detection of the nscd mappings.  Making POSIX robust mutexes conditionally supported is awkward, and the fact that some architectures may lack support for them is undocumented.  Application programmers will not expect this.

> 


The problem is kernel supports for some architecture depends of kernel config
and the underlying hardware revision/model.

* ARM we have:

arch/arm/include/asm/futex.h

v3.0-rc1

#if defined(CONFIG_CPU_USE_DOMAINS) && defined(CONFIG_SMP)
/* ARM doesn't provide unprivileged exclusive memory accessors */
#include <asm-generic/futex.h>
#else

And it was unconditionally define only on v3.15-rc1.  It means that for some
ARMv6 configure the futex_atomic_cmpxchg_inatomic may return ENOSYS.

* Microblaze have a nasty bug that was fixed only on 3.10-rc4 (f6a12a7d0b):

 microblaze: Reversed logic in futex cmpxchg
    
    futex_atomic_cmpxchg_inatomic exchanged if the values were
    unequal rather than equal. This caused incorrect behavior
    of robust futexes.

* MIPS still lacks proper supports depending of the underlying hardware:

arch/mips/include/asm/futex.h

142 static inline int
143 futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
144                               u32 oldval, u32 newval)
145 {
146         int ret = 0;
147         u32 val;
148 
149         if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32)))
150                 return -EFAULT;
151 
152         if (cpu_has_llsc && R10000_LLSC_WAR) {
[...]
181         } else if (cpu_has_llsc) {
182                 __asm__ __volatile__(
[...]
210         } else
211                 return -ENOSYS;

  And kernel still have support for some architecture that lacks 'cpu_has_llsc'
  (although I am not sure if glibc also have support for this kind of cpu as
  well).

* sh4 lacks proper SMP support (although not sure how common it is) and it was
  implemented on v4.8-rc1 (00b73d8d1b71).

* m68k Uses the default include/asm-generic/futex.h which only has support for
  uniprocessors.

That's why I think best option would to simplify glibc build to avoid multiple
config depending of the underlying kernel config and query robust support at
runtime.  It is indeed awkward that some kernel option lacks robust support and
we can check with hardware mantainers how often or if it desirable to support
such config/chips.
Andreas Schwab April 19, 2017, 2:51 p.m. | #3
On Apr 19 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:

> * m68k Uses the default include/asm-generic/futex.h which only has support for

>   uniprocessors.


m68k has no support for SMP.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Adhemerval Zanella April 19, 2017, 2:52 p.m. | #4
On 19/04/2017 11:51, Andreas Schwab wrote:
> On Apr 19 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:

> 

>> * m68k Uses the default include/asm-generic/futex.h which only has support for

>>   uniprocessors.

> 

> m68k has no support for SMP.

> 

> Andreas.

> 


Right, so m68k should be safe regarding robust support (assuming asm-generic/futex.h
works correctly).
Florian Weimer April 19, 2017, 3:28 p.m. | #5
On 04/19/2017 04:17 PM, Adhemerval Zanella wrote:
> The problem is kernel supports for some architecture depends of kernel config

> and the underlying hardware revision/model.


I understand that.  But I have seen a lot of use of robust mutexes 
lately, and I'm sure these developers aren't aware that the mutexes 
aren't portable across GNU/Linux (the Linux architecture subset 
supported by glibc).

I expect it's like the missing accept4 system call—you need to provide 
the set_robust_list system call in the kernel if you want to a working, 
modern system.

Thanks,
Florian
Adhemerval Zanella April 19, 2017, 5:38 p.m. | #6
On Wed, Apr 19, 2017 at 12:28 PM, Florian Weimer <fweimer@redhat.com> wrote:
> On 04/19/2017 04:17 PM, Adhemerval Zanella wrote:

>>

>> The problem is kernel supports for some architecture depends of kernel

>> config

>> and the underlying hardware revision/model.

>

>

> I understand that.  But I have seen a lot of use of robust mutexes lately,

> and I'm sure these developers aren't aware that the mutexes aren't portable

> across GNU/Linux (the Linux architecture subset supported by glibc).

>

> I expect it's like the missing accept4 system call—you need to provide the

> set_robust_list system call in the kernel if you want to a working, modern

> system.


But my point is with current minimum supported kernel version for some
architectures
we can't simple assume set_robust_list support and even bumping minimum kernel
version for some architectures also do not solve the issue (on mips
for instance).

That's why I still think adding a runtime check support is still
required and slight better
than just disable for such architecture that might still have support.

I think in the future when I might increase minimum kernel support we
might rework
and require robust support regardless for such architectures.

-- 
“One thing I have learned in a long life: that all our science,
measured against reality, is primitive and childlike -- and yet it is
the most precious thing we have.”

― Albert Einstein
Florian Weimer April 19, 2017, 6:09 p.m. | #7
On 04/19/2017 07:38 PM, Adhemerval Zanella wrote:
> On Wed, Apr 19, 2017 at 12:28 PM, Florian Weimer <fweimer@redhat.com> wrote:

>> On 04/19/2017 04:17 PM, Adhemerval Zanella wrote:

>>>

>>> The problem is kernel supports for some architecture depends of kernel

>>> config

>>> and the underlying hardware revision/model.

>>

>>

>> I understand that.  But I have seen a lot of use of robust mutexes lately,

>> and I'm sure these developers aren't aware that the mutexes aren't portable

>> across GNU/Linux (the Linux architecture subset supported by glibc).

>>

>> I expect it's like the missing accept4 system call—you need to provide the

>> set_robust_list system call in the kernel if you want to a working, modern

>> system.

> 

> But my point is with current minimum supported kernel version for some

> architectures

> we can't simple assume set_robust_list support and even bumping minimum kernel

> version for some architectures also do not solve the issue (on mips

> for instance).


Hmm, maybe you are right, and we have to keep things this way for now.

But I don't like that your patch reintroduces the conditional code.  You 
could use

#define __set_robust_list_avail 1

for the __ASSUME_SET_ROBUST_LIST case to reduce clutter, and let GCC do 
the rest.

Thanks,
Florian
Adhemerval Zanella April 19, 2017, 9:02 p.m. | #8
On 19/04/2017 15:09, Florian Weimer wrote:
> On 04/19/2017 07:38 PM, Adhemerval Zanella wrote:

>> On Wed, Apr 19, 2017 at 12:28 PM, Florian Weimer <fweimer@redhat.com> wrote:

>>> On 04/19/2017 04:17 PM, Adhemerval Zanella wrote:

>>>>

>>>> The problem is kernel supports for some architecture depends of kernel

>>>> config

>>>> and the underlying hardware revision/model.

>>>

>>>

>>> I understand that.  But I have seen a lot of use of robust mutexes lately,

>>> and I'm sure these developers aren't aware that the mutexes aren't portable

>>> across GNU/Linux (the Linux architecture subset supported by glibc).

>>>

>>> I expect it's like the missing accept4 system call—you need to provide the

>>> set_robust_list system call in the kernel if you want to a working, modern

>>> system.

>>

>> But my point is with current minimum supported kernel version for some

>> architectures

>> we can't simple assume set_robust_list support and even bumping minimum kernel

>> version for some architectures also do not solve the issue (on mips

>> for instance).

> 

> Hmm, maybe you are right, and we have to keep things this way for now.

> 

> But I don't like that your patch reintroduces the conditional code.  You could use

> 

> #define __set_robust_list_avail 1

> 

> for the __ASSUME_SET_ROBUST_LIST case to reduce clutter, and let GCC do the rest.

> 

> Thanks,

> Florian


Alright, I will change it and send a new version.
Adhemerval Zanella April 20, 2017, 12:50 p.m. | #9
On 19/04/2017 15:09, Florian Weimer wrote:
> On 04/19/2017 07:38 PM, Adhemerval Zanella wrote:

>> On Wed, Apr 19, 2017 at 12:28 PM, Florian Weimer <fweimer@redhat.com> wrote:

>>> On 04/19/2017 04:17 PM, Adhemerval Zanella wrote:

>>>>

>>>> The problem is kernel supports for some architecture depends of kernel

>>>> config

>>>> and the underlying hardware revision/model.

>>>

>>>

>>> I understand that.  But I have seen a lot of use of robust mutexes lately,

>>> and I'm sure these developers aren't aware that the mutexes aren't portable

>>> across GNU/Linux (the Linux architecture subset supported by glibc).

>>>

>>> I expect it's like the missing accept4 system call—you need to provide the

>>> set_robust_list system call in the kernel if you want to a working, modern

>>> system.

>>

>> But my point is with current minimum supported kernel version for some

>> architectures

>> we can't simple assume set_robust_list support and even bumping minimum kernel

>> version for some architectures also do not solve the issue (on mips

>> for instance).

> 

> Hmm, maybe you are right, and we have to keep things this way for now.

> 

> But I don't like that your patch reintroduces the conditional code.  You could use

> 

> #define __set_robust_list_avail 1

> 

> for the __ASSUME_SET_ROBUST_LIST case to reduce clutter, and let GCC do the rest.


Rechecking what you meant, I am not following you here: the idea is to have
__set_robust_list_avail being a runtime variable and get rid of the _ASSUME
define. How using a define would help in this way?
Florian Weimer April 20, 2017, 12:52 p.m. | #10
On 04/20/2017 02:50 PM, Adhemerval Zanella wrote:
> 

> 

> On 19/04/2017 15:09, Florian Weimer wrote:

>> On 04/19/2017 07:38 PM, Adhemerval Zanella wrote:

>>> On Wed, Apr 19, 2017 at 12:28 PM, Florian Weimer <fweimer@redhat.com> wrote:

>>>> On 04/19/2017 04:17 PM, Adhemerval Zanella wrote:

>>>>>

>>>>> The problem is kernel supports for some architecture depends of kernel

>>>>> config

>>>>> and the underlying hardware revision/model.

>>>>

>>>>

>>>> I understand that.  But I have seen a lot of use of robust mutexes lately,

>>>> and I'm sure these developers aren't aware that the mutexes aren't portable

>>>> across GNU/Linux (the Linux architecture subset supported by glibc).

>>>>

>>>> I expect it's like the missing accept4 system call—you need to provide the

>>>> set_robust_list system call in the kernel if you want to a working, modern

>>>> system.

>>>

>>> But my point is with current minimum supported kernel version for some

>>> architectures

>>> we can't simple assume set_robust_list support and even bumping minimum kernel

>>> version for some architectures also do not solve the issue (on mips

>>> for instance).

>>

>> Hmm, maybe you are right, and we have to keep things this way for now.

>>

>> But I don't like that your patch reintroduces the conditional code.  You could use

>>

>> #define __set_robust_list_avail 1

>>

>> for the __ASSUME_SET_ROBUST_LIST case to reduce clutter, and let GCC do the rest.

> 

> Rechecking what you meant, I am not following you here: the idea is to have

> __set_robust_list_avail being a runtime variable and get rid of the _ASSUME

> define. How using a define would help in this way?


I suggest to keep the __ASSUME_SET_ROBUST_LIST define but concentrate 
its use to a single place by making __set_robust_list_avail available 
unconditionally, but as a constant for  __ASSUME_SET_ROBUST_LIST.  This 
is the pattern we used elsewhere (e.g. for have_cloexec).

Thanks,
Florian
Adhemerval Zanella April 20, 2017, 1:09 p.m. | #11
On 20/04/2017 09:52, Florian Weimer wrote:
> On 04/20/2017 02:50 PM, Adhemerval Zanella wrote:

>>

>>

>> On 19/04/2017 15:09, Florian Weimer wrote:

>>> On 04/19/2017 07:38 PM, Adhemerval Zanella wrote:

>>>> On Wed, Apr 19, 2017 at 12:28 PM, Florian Weimer <fweimer@redhat.com> wrote:

>>>>> On 04/19/2017 04:17 PM, Adhemerval Zanella wrote:

>>>>>>

>>>>>> The problem is kernel supports for some architecture depends of kernel

>>>>>> config

>>>>>> and the underlying hardware revision/model.

>>>>>

>>>>>

>>>>> I understand that.  But I have seen a lot of use of robust mutexes lately,

>>>>> and I'm sure these developers aren't aware that the mutexes aren't portable

>>>>> across GNU/Linux (the Linux architecture subset supported by glibc).

>>>>>

>>>>> I expect it's like the missing accept4 system call—you need to provide the

>>>>> set_robust_list system call in the kernel if you want to a working, modern

>>>>> system.

>>>>

>>>> But my point is with current minimum supported kernel version for some

>>>> architectures

>>>> we can't simple assume set_robust_list support and even bumping minimum kernel

>>>> version for some architectures also do not solve the issue (on mips

>>>> for instance).

>>>

>>> Hmm, maybe you are right, and we have to keep things this way for now.

>>>

>>> But I don't like that your patch reintroduces the conditional code.  You could use

>>>

>>> #define __set_robust_list_avail 1

>>>

>>> for the __ASSUME_SET_ROBUST_LIST case to reduce clutter, and let GCC do the rest.

>>

>> Rechecking what you meant, I am not following you here: the idea is to have

>> __set_robust_list_avail being a runtime variable and get rid of the _ASSUME

>> define. How using a define would help in this way?

> 

> I suggest to keep the __ASSUME_SET_ROBUST_LIST define but concentrate its use to a single place by making __set_robust_list_avail available unconditionally, but as a constant for  __ASSUME_SET_ROBUST_LIST.  This is the pattern we used elsewhere (e.g. for have_cloexec).


But what I am suggesting is exactly to get rid of __ASSUME_SET_ROBUST_LIST to
simplify the conditionals and have it defined at runtime instead of having
each architecture define whether it supports or not set_robust_list. What you
are suggesting is basically current behaviour.
Florian Weimer April 20, 2017, 1:47 p.m. | #12
On 04/20/2017 03:09 PM, Adhemerval Zanella wrote:

> But what I am suggesting is exactly to get rid of __ASSUME_SET_ROBUST_LIST to

> simplify the conditionals and have it defined at runtime instead of having

> each architecture define whether it supports or not set_robust_list. What you

> are suggesting is basically current behaviour.


Right, but I don't think we should do the cleanup yet when we still need 
run-time checking.

Thanks,
Florian
Adhemerval Zanella April 20, 2017, 5:25 p.m. | #13
On 20/04/2017 10:47, Florian Weimer wrote:
> On 04/20/2017 03:09 PM, Adhemerval Zanella wrote:

> 

>> But what I am suggesting is exactly to get rid of __ASSUME_SET_ROBUST_LIST to

>> simplify the conditionals and have it defined at runtime instead of having

>> each architecture define whether it supports or not set_robust_list. What you

>> are suggesting is basically current behaviour.

> 

> Right, but I don't think we should do the cleanup yet when we still need run-time checking.


But it does not remove the run-time checking, but rather sets it as the default.
The idea is to continue provide the run-time capabilities and to simplify
the code by removing compilation defines.

Patch hide | download patch | download mbox

diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
index 2921607..c86b35e 100644
--- a/nptl/nptl-init.c
+++ b/nptl/nptl-init.c
@@ -48,14 +48,8 @@  int *__libc_multiple_threads_ptr attribute_hidden;
 size_t __static_tls_size;
 size_t __static_tls_align_m1;
 
-#ifndef __ASSUME_SET_ROBUST_LIST
 /* Negative if we do not have the system call and we can use it.  */
-int __set_robust_list_avail;
-# define set_robust_list_not_avail() \
-  __set_robust_list_avail = -1
-#else
-# define set_robust_list_not_avail() do { } while (0)
-#endif
+int __set_robust_list_avail = -1;
 
 #ifndef __ASSUME_FUTEX_CLOCK_REALTIME
 /* Nonzero if we do not have FUTEX_CLOCK_REALTIME.  */
@@ -308,9 +302,9 @@  __pthread_initialize_minimal_internal (void)
     INTERNAL_SYSCALL_DECL (err);
     int res = INTERNAL_SYSCALL (set_robust_list, err, 2, &pd->robust_head,
 				sizeof (struct robust_list_head));
-    if (INTERNAL_SYSCALL_ERROR_P (res, err))
+    if (!INTERNAL_SYSCALL_ERROR_P (res, err))
+      __set_robust_list_avail = 1;
 #endif
-      set_robust_list_not_avail ();
   }
 
 #ifdef __NR_futex
diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
index 7fc1e50..4125e7f 100644
--- a/nptl/pthreadP.h
+++ b/nptl/pthreadP.h
@@ -213,10 +213,8 @@  hidden_proto (__pthread_keys)
 /* Number of threads running.  */
 extern unsigned int __nptl_nthreads attribute_hidden;
 
-#ifndef __ASSUME_SET_ROBUST_LIST
 /* Negative if we do not have the system call and we can use it.  */
 extern int __set_robust_list_avail attribute_hidden;
-#endif
 
 /* Thread Priority Protection.  */
 extern int __sched_fifo_min_prio attribute_hidden;
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index d0d7414..76b0ac4 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -387,18 +387,16 @@  START_THREAD_DEFN
   if (__glibc_unlikely (atomic_exchange_acq (&pd->setxid_futex, 0) == -2))
     futex_wake (&pd->setxid_futex, 1, FUTEX_PRIVATE);
 
-#ifdef __NR_set_robust_list
-# ifndef __ASSUME_SET_ROBUST_LIST
   if (__set_robust_list_avail >= 0)
-# endif
     {
+#ifdef __NR_set_robust_list
       INTERNAL_SYSCALL_DECL (err);
       /* This call should never fail because the initial call in init.c
 	 succeeded.  */
       INTERNAL_SYSCALL (set_robust_list, err, 2, &pd->robust_head,
 			sizeof (struct robust_list_head));
-    }
 #endif
+    }
 
 #ifdef SIGCANCEL
   /* If the parent was running cancellation handlers while creating
@@ -508,7 +506,6 @@  START_THREAD_DEFN
      the breakpoint reports TD_THR_RUN state rather than TD_THR_ZOMBIE.  */
   atomic_bit_set (&pd->cancelhandling, EXITING_BIT);
 
-#ifndef __ASSUME_SET_ROBUST_LIST
   /* If this thread has any robust mutexes locked, handle them now.  */
 # ifdef __PTHREAD_MUTEX_HAVE_PREV
   void *robust = pd->robust_head.list;
@@ -539,7 +536,6 @@  START_THREAD_DEFN
 	}
       while (robust != (void *) &pd->robust_head);
     }
-#endif
 
   /* Mark the memory of the stack as usable to the kernel.  We free
      everything except for the space used for the TCB itself.  */
diff --git a/nptl/pthread_mutex_init.c b/nptl/pthread_mutex_init.c
index 138e144..18724e8 100644
--- a/nptl/pthread_mutex_init.c
+++ b/nptl/pthread_mutex_init.c
@@ -91,11 +91,9 @@  __pthread_mutex_init (pthread_mutex_t *mutex,
 
   if ((imutexattr->mutexkind & PTHREAD_MUTEXATTR_FLAG_ROBUST) != 0)
     {
-#ifndef __ASSUME_SET_ROBUST_LIST
       if ((imutexattr->mutexkind & PTHREAD_MUTEXATTR_FLAG_PSHARED) != 0
 	  && __set_robust_list_avail < 0)
 	return ENOTSUP;
-#endif
 
       mutex->__data.__kind |= PTHREAD_MUTEX_ROBUST_NORMAL_NP;
     }