[PATCH 2/2] linux: Remove __ASSUME_SET_ROBUST_LIST defines

[PATCH 2/2] linux: Remove __ASSUME_SET_ROBUST_LIST defines

[Adhemerval Zanella]

As for the first part, I am sending this patch again.  The onyl change
was to rebase again master after __ASSUME_REQUEUE_PI removal (2e4cf77897).

--

This patch removes __ASSUME_SET_ROBUST_LIST usage and assumes that
kernel will correctly return if it supports or not
futex_atomic_cmpxchg_inatomic.

On minimum supported kernel (v3.2 and v2.6.32 for x86) kernel has:

2418 SYSCALL_DEFINE2(set_robust_list, struct robust_list_head __user *, head,
2419                 size_t, len)
2420 {
2421         if (!futex_cmpxchg_enabled)
2422                 return -ENOSYS;

The patch also adds the __set_robust_list_avail runtime check for all
architectures, since for some the syscall may still return ENOSYS if
futex_atomic_cmpxchg_inatomic is not supported (for instance ARM).

Tested on armhf (with 3.8 kernel) and x86_64.

	* sysdeps/unix/sysv/linux/arm/kernel-features.h
	(__ASSUME_SET_ROBUST_LIST): Likewise.
	* sysdeps/unix/sysv/linux/kernel-features.h:
	(__ASSUME_SET_ROBUST_LIST): Likewise.
	* sysdeps/unix/sysv/linux/m68k/kernel-features.h:
	(__ASSUME_SET_ROBUST_LIST): Likewise.
	* sysdeps/unix/sysv/linux/mips/kernel-features.h:
	(__ASSUME_SET_ROBUST_LIST): Likewise.
	* sysdeps/unix/sysv/linux/sparc/kernel-features.h:
	(__ASSUME_SET_ROBUST_LIST): Likewise.
---
 sysdeps/unix/sysv/linux/arm/kernel-features.h   | 7 -------
 sysdeps/unix/sysv/linux/kernel-features.h       | 5 -----
 sysdeps/unix/sysv/linux/mips/kernel-features.h  | 6 ------
 sysdeps/unix/sysv/linux/sparc/kernel-features.h | 6 ------
 4 files changed, 24 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/arm/kernel-features.h b/sysdeps/unix/sysv/linux/arm/kernel-features.h
index e13b049..1edd988 100644
--- a/sysdeps/unix/sysv/linux/arm/kernel-features.h
+++ b/sysdeps/unix/sysv/linux/arm/kernel-features.h
@@ -19,13 +19,6 @@
 
 #include_next <kernel-features.h>
 
-/* The ARM kernel before 3.14.3 may or may not support
-   futex_atomic_cmpxchg_inatomic, depending on kernel
-   configuration.  */
-#if __LINUX_KERNEL_VERSION < 0x030E03
-# undef __ASSUME_SET_ROBUST_LIST
-#endif
-
 /* ARM fadvise64_64 reorganize the syscall arguments.  */
 #define __ASSUME_FADVISE64_64_6ARG	1
 
diff --git a/sysdeps/unix/sysv/linux/kernel-features.h b/sysdeps/unix/sysv/linux/kernel-features.h
index 233e302..2a25eb6 100644
--- a/sysdeps/unix/sysv/linux/kernel-features.h
+++ b/sysdeps/unix/sysv/linux/kernel-features.h
@@ -59,11 +59,6 @@
    they were introduced in 2.6.17-rc1, on SH in 2.6.19-rc1.  */
 #define __ASSUME_ATFCTS	1
 
-/* Support for inter-process robust mutexes was added in 2.6.17 (but
-   some architectures lack futex_atomic_cmpxchg_inatomic in some
-   configurations).  */
-#define __ASSUME_SET_ROBUST_LIST	1
-
 /* Support for private futexes was added in 2.6.22.  */
 #define __ASSUME_PRIVATE_FUTEX	1
 
diff --git a/sysdeps/unix/sysv/linux/mips/kernel-features.h b/sysdeps/unix/sysv/linux/mips/kernel-features.h
index d5d35af..9d533aa 100644
--- a/sysdeps/unix/sysv/linux/mips/kernel-features.h
+++ b/sysdeps/unix/sysv/linux/mips/kernel-features.h
@@ -21,12 +21,6 @@
 
 #include_next <kernel-features.h>
 
-/* The MIPS kernel does not support futex_atomic_cmpxchg_inatomic if
-   emulating LL/SC.  */
-#if __mips == 1 || defined _MIPS_ARCH_R5900
-# undef __ASSUME_SET_ROBUST_LIST
-#endif
-
 /* Define this if your 32-bit syscall API requires 64-bit register
    pairs to start with an even-number register.  */
 #if _MIPS_SIM == _ABIO32
diff --git a/sysdeps/unix/sysv/linux/sparc/kernel-features.h b/sysdeps/unix/sysv/linux/sparc/kernel-features.h
index c833201..7dc8f11 100644
--- a/sysdeps/unix/sysv/linux/sparc/kernel-features.h
+++ b/sysdeps/unix/sysv/linux/sparc/kernel-features.h
@@ -31,12 +31,6 @@
 
 #include_next <kernel-features.h>
 
-/* 32-bit SPARC kernels do not support
-   futex_atomic_cmpxchg_inatomic.  */
-#if !defined __arch64__ && !defined __sparc_v9__
-# undef __ASSUME_SET_ROBUST_LIST
-#endif
-
 #if !defined __arch64__
 # undef __ASSUME_ACCEPT_SYSCALL
 # undef __ASSUME_CONNECT_SYSCALL
-- 
2.7.4

[PATCH 1/2] nptl: Remove __ASSUME_SET_ROBUST_LIST

[Adhemerval Zanella]

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

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;
     }
-- 
2.7.4

Re: [PATCH 1/2] nptl: Remove __ASSUME_SET_ROBUST_LIST

[Florian Weimer]

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

Re: [PATCH 1/2] nptl: Remove __ASSUME_SET_ROBUST_LIST

[Adhemerval Zanella]

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.

Re: [PATCH 1/2] nptl: Remove __ASSUME_SET_ROBUST_LIST

[Andreas Schwab]

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."

Re: [PATCH 1/2] nptl: Remove __ASSUME_SET_ROBUST_LIST

[Adhemerval Zanella]

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

Re: [PATCH 1/2] nptl: Remove __ASSUME_SET_ROBUST_LIST

[Florian Weimer]

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

Re: [PATCH 1/2] nptl: Remove __ASSUME_SET_ROBUST_LIST

[Adhemerval Zanella]

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

Re: [PATCH 1/2] nptl: Remove __ASSUME_SET_ROBUST_LIST

[Florian Weimer]

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

Re: [PATCH 1/2] nptl: Remove __ASSUME_SET_ROBUST_LIST

[Adhemerval Zanella]

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.

Re: [PATCH 1/2] nptl: Remove __ASSUME_SET_ROBUST_LIST

[Adhemerval Zanella]

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?

Re: [PATCH 1/2] nptl: Remove __ASSUME_SET_ROBUST_LIST

[Florian Weimer]

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

Re: [PATCH 1/2] nptl: Remove __ASSUME_SET_ROBUST_LIST

[Adhemerval Zanella]

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.

Re: [PATCH 1/2] nptl: Remove __ASSUME_SET_ROBUST_LIST

[Florian Weimer]

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

Re: [PATCH 1/2] nptl: Remove __ASSUME_SET_ROBUST_LIST

[Adhemerval Zanella]

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 1/2] nptl: Remove __ASSUME_SET_ROBUST_LIST

[Adhemerval Zanella]

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.
---
 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(-)

diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
index bdbdfed..6dd658a 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.  */
@@ -335,9 +329,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 6e0dd09..0ce65f3 100644
--- a/nptl/pthreadP.h
+++ b/nptl/pthreadP.h
@@ -199,10 +199,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 a834063..04c5e7f 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -271,18 +271,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
@@ -388,7 +386,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;
@@ -419,7 +416,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 6e5acb6..6aef890 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;
     }
-- 
2.7.4