[PATCH 1/2] misc: Set generic pselect as ENOSYS

[PATCH 1/2] misc: Set generic pselect as ENOSYS

[Adhemerval Zanella]

The generic pselect implementation has the very specific race condition
that motived the creation of the pselect syscall (no atomicity in
signal mask set/reset).  Using it as generic implementation is
counterproductive  Also currently only microblaze uses it as fallback
when used on kernel prior 3.15.

This patch moves the generic implementation to a microblaze specific
one, sets the generic internal as a ENOSYS, and cleanups the Linux
generic implementation.

Also, the microblaze generic implementation first try to issue
pselect instead of use the fallback (since it is expect that if
the microblaze usage does rely on pselect, a sufficient updated
kernel will be used).  Microblaze defines __NR_pselect6 for Linux
v3.2, although it was only wire-up on v3.15 (and the syscall number
is the same as previous defined).

Checked with a build against i686-linux-gnu (smoke test) and
microblaze-linux-gnu.
---
 misc/pselect.c                               | 41 +-----------
 sysdeps/unix/sysv/linux/microblaze/pselect.c | 69 ++++++++++++++++++++
 sysdeps/unix/sysv/linux/pselect.c            | 31 +--------
 3 files changed, 75 insertions(+), 66 deletions(-)
 create mode 100644 sysdeps/unix/sysv/linux/microblaze/pselect.c

diff --git a/misc/pselect.c b/misc/pselect.c
index 76ded850a5..7296b6ef61 100644
--- a/misc/pselect.c
+++ b/misc/pselect.c
@@ -34,43 +34,8 @@ int
 __pselect (int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
 	   const struct timespec *timeout, const sigset_t *sigmask)
 {
-  struct timeval tval;
-  int retval;
-  sigset_t savemask;
-
-  /* Change nanosecond number to microseconds.  This might mean losing
-     precision and therefore the `pselect` should be available.  But
-     for now it is hardly found.  */
-  if (timeout != NULL)
-    {
-      /* Catch bugs which would be hidden by the TIMESPEC_TO_TIMEVAL
-	 computations.  The division by 1000 truncates values.  */
-      if (__glibc_unlikely (timeout->tv_nsec < 0))
-	{
-	  __set_errno (EINVAL);
-	  return -1;
-	}
-
-      TIMESPEC_TO_TIMEVAL (&tval, timeout);
-    }
-
-  /* The setting and restoring of the signal mask and the select call
-     should be an atomic operation.  This can't be done without kernel
-     help.  */
-  if (sigmask != NULL)
-    __sigprocmask (SIG_SETMASK, sigmask, &savemask);
-
-  /* Note the pselect() is a cancellation point.  But since we call
-     select() which itself is a cancellation point we do not have
-     to do anything here.  */
-  retval = __select (nfds, readfds, writefds, exceptfds,
-		     timeout != NULL ? &tval : NULL);
-
-  if (sigmask != NULL)
-    __sigprocmask (SIG_SETMASK, &savemask, NULL);
-
-  return retval;
+  __set_errno (ENOSYS);
+  return -1;
 }
-#ifndef __pselect
 weak_alias (__pselect, pselect)
-#endif
+stub_warning (pselect)
diff --git a/sysdeps/unix/sysv/linux/microblaze/pselect.c b/sysdeps/unix/sysv/linux/microblaze/pselect.c
new file mode 100644
index 0000000000..62430ca69a
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/microblaze/pselect.c
@@ -0,0 +1,69 @@
+/* Synchronous I/O multiplexing.  Linux/microblaze version.
+   Copyright (C) 2019 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
+   <https://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <signal.h>
+#include <time.h>
+#include <sys/poll.h>
+#include <sysdep-cancel.h>
+
+/* Microblaze defines __NR_pselect6 for Linux v3.2, although it was wire-up
+   on v3.15.  */
+#define __pselect __pselect_syscall
+#include <sysdeps/unix/sysv/linux/pselect.c>
+#undef __pselect
+
+int
+__pselect (int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
+	   const struct timespec *timeout, const sigset_t *sigmask)
+{
+  int ret = __pselect_syscall (nfds, readfds, writefds, exceptfds, timeout,
+			       sigmask);
+  if (ret < 0 && errno != ENOSYS)
+    return ret;
+
+  /* The fallback uses 'selects' which shows the race condition regarding
+     signal mask set/restore, requires two additional syscalls, and has
+     a worse timeout precision (microseconds instead of nanoseconds).  */
+
+  struct timeval tval, *ptval = NULL;
+  if (timeout != NULL)
+    {
+      if (! valid_nanoseconds (timeout->tv_nsec))
+	{
+	  __set_errno (EINVAL);
+	  return -1;
+	}
+
+      TIMESPEC_TO_TIMEVAL (&tval, timeout);
+      ptval = &tval;
+    }
+
+  sigset_t savemask;
+  if (sigmask != NULL)
+    __sigprocmask (SIG_SETMASK, sigmask, &savemask);
+
+  /* select itself is a cancellation entrypoint.  */
+  ret = __select (nfds, readfds, writefds, exceptfds, ptval);
+
+  if (sigmask != NULL)
+    __sigprocmask (SIG_SETMASK, &savemask, NULL);
+
+  return ret;
+}
+weak_alias (__pselect, pselect)
diff --git a/sysdeps/unix/sysv/linux/pselect.c b/sysdeps/unix/sysv/linux/pselect.c
index acda3e0cdd..77a5179925 100644
--- a/sysdeps/unix/sysv/linux/pselect.c
+++ b/sysdeps/unix/sysv/linux/pselect.c
@@ -23,16 +23,6 @@
 #include <kernel-features.h>
 #include <sysdep-cancel.h>
 
-
-#ifdef __NR_pselect6
-# ifndef __ASSUME_PSELECT
-static int __generic_pselect (int nfds, fd_set *readfds, fd_set *writefds,
-			      fd_set *exceptfds,
-			      const struct timespec *timeout,
-			      const sigset_t *sigmask);
-# endif
-
-
 int
 __pselect (int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
 	   const struct timespec *timeout, const sigset_t *sigmask)
@@ -59,24 +49,9 @@ __pselect (int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
   data.ss = (__syscall_ulong_t) (uintptr_t) sigmask;
   data.ss_len = _NSIG / 8;
 
-  int result = SYSCALL_CANCEL (pselect6, nfds, readfds, writefds, exceptfds,
-                               timeout, &data);
-
-# ifndef __ASSUME_PSELECT
-  if (result == -1 && errno == ENOSYS)
-    result = __generic_pselect (nfds, readfds, writefds, exceptfds, timeout,
-				sigmask);
-# endif
-
-  return result;
+  return SYSCALL_CANCEL (pselect6, nfds, readfds, writefds, exceptfds,
+                         timeout, &data);
 }
+#ifndef __pselect
 weak_alias (__pselect, pselect)
-
-# ifndef __ASSUME_PSELECT
-#  define __pselect static __generic_pselect
-# endif
-#endif
-
-#ifndef __ASSUME_PSELECT
-# include <misc/pselect.c>
 #endif
-- 
2.17.1

[PATCH 2/2] linux: Remove __ASSUME_PSELECT

[Adhemerval Zanella]

The specific microblaze pselect implementation does not use
__ASSUME_PSELECT anymore.

Checked with a build against microblaze-linux-gnu.
---
 sysdeps/unix/sysv/linux/kernel-features.h            | 4 ----
 sysdeps/unix/sysv/linux/microblaze/kernel-features.h | 1 -
 2 files changed, 5 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/kernel-features.h b/sysdeps/unix/sysv/linux/kernel-features.h
index e6be76ff46..9012aae4c0 100644
--- a/sysdeps/unix/sysv/linux/kernel-features.h
+++ b/sysdeps/unix/sysv/linux/kernel-features.h
@@ -45,10 +45,6 @@
 /* The statfs64 syscalls are available in 2.5.74 (but not for alpha).  */
 #define __ASSUME_STATFS64	1
 
-/* pselect/ppoll were introduced just after 2.6.16-rc1.  On x86_64 and
-   SH this appeared first in 2.6.19-rc1, on ia64 in 2.6.22-rc1.  */
-#define __ASSUME_PSELECT	1
-
 /* The *at syscalls were introduced just after 2.6.16-rc1.  On PPC
    they were introduced in 2.6.17-rc1, on SH in 2.6.19-rc1.  */
 #define __ASSUME_ATFCTS	1
diff --git a/sysdeps/unix/sysv/linux/microblaze/kernel-features.h b/sysdeps/unix/sysv/linux/microblaze/kernel-features.h
index 2dd9810f93..1c60488ea9 100644
--- a/sysdeps/unix/sysv/linux/microblaze/kernel-features.h
+++ b/sysdeps/unix/sysv/linux/microblaze/kernel-features.h
@@ -36,7 +36,6 @@
 /* Support for the pselect6, preadv and pwritev syscalls was added in
    3.15.  */
 #if __LINUX_KERNEL_VERSION < 0x030f00
-# undef __ASSUME_PSELECT
 # undef __ASSUME_PREADV
 # undef __ASSUME_PWRITEV
 #endif
-- 
2.17.1

Re: [PATCH 2/2] linux: Remove __ASSUME_PSELECT

[Florian Weimer]

* Adhemerval Zanella:

> The specific microblaze pselect implementation does not use
> __ASSUME_PSELECT anymore.

This looks okay, assuming the other patch goes in.

Re: [PATCH 1/2] misc: Set generic pselect as ENOSYS

[Florian Weimer]

* Adhemerval Zanella:

> diff --git a/misc/pselect.c b/misc/pselect.c
> index 76ded850a5..7296b6ef61 100644
> --- a/misc/pselect.c
> +++ b/misc/pselect.c
> @@ -34,43 +34,8 @@ int

Missing header file adjustments?

> diff --git a/sysdeps/unix/sysv/linux/microblaze/pselect.c b/sysdeps/unix/sysv/linux/microblaze/pselect.c
> new file mode 100644
> index 0000000000..62430ca69a
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/microblaze/pselect.c

> +  /* The fallback uses 'selects' which shows the race condition regarding

Typo: 'selects' (I think)

> index acda3e0cdd..77a5179925 100644
> --- a/sysdeps/unix/sysv/linux/pselect.c
> +++ b/sysdeps/unix/sysv/linux/pselect.c
> @@ -23,16 +23,6 @@
>  #include <kernel-features.h>
>  #include <sysdep-cancel.h>

Again I suspect header file cleanups are possible.

Re: [PATCH 1/2] misc: Set generic pselect as ENOSYS

[Adhemerval Zanella]

On 14/11/2019 16:28, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> diff --git a/misc/pselect.c b/misc/pselect.c
>> index 76ded850a5..7296b6ef61 100644
>> --- a/misc/pselect.c
>> +++ b/misc/pselect.c
>> @@ -34,43 +34,8 @@ int
> 
> Missing header file adjustments?

Hum, which adjustment are you referring exactly?

> 
>> diff --git a/sysdeps/unix/sysv/linux/microblaze/pselect.c b/sysdeps/unix/sysv/linux/microblaze/pselect.c
>> new file mode 100644
>> index 0000000000..62430ca69a
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/microblaze/pselect.c
> 
>> +  /* The fallback uses 'selects' which shows the race condition regarding
> 
> Typo: 'selects' (I think)

Ack.

> 
>> index acda3e0cdd..77a5179925 100644
>> --- a/sysdeps/unix/sysv/linux/pselect.c
>> +++ b/sysdeps/unix/sysv/linux/pselect.c
>> @@ -23,16 +23,6 @@
>>  #include <kernel-features.h>
>>  #include <sysdep-cancel.h>
> 
> Again I suspect header file cleanups are possible.
> 

Re: [PATCH 1/2] misc: Set generic pselect as ENOSYS

[Florian Weimer]

* Adhemerval Zanella:

> On 14/11/2019 16:28, Florian Weimer wrote:
>> * Adhemerval Zanella:
>> 
>>> diff --git a/misc/pselect.c b/misc/pselect.c
>>> index 76ded850a5..7296b6ef61 100644
>>> --- a/misc/pselect.c
>>> +++ b/misc/pselect.c
>>> @@ -34,43 +34,8 @@ int
>> 
>> Missing header file adjustments?
>
> Hum, which adjustment are you referring exactly?

The file has this right now:

#include <errno.h>
#include <signal.h>
#include <stddef.h>	/* For NULL.  */
#include <sys/time.h>
#include <sys/select.h>
#include <sysdep-cancel.h>

I think that can be trimmed substantially.

Re: [PATCH 1/2] misc: Set generic pselect as ENOSYS

[Joseph Myers]

On Thu, 14 Nov 2019, Adhemerval Zanella wrote:

> The generic pselect implementation has the very specific race condition
> that motived the creation of the pselect syscall (no atomicity in
> signal mask set/reset).  Using it as generic implementation is
> counterproductive  Also currently only microblaze uses it as fallback
> when used on kernel prior 3.15.
> 
> This patch moves the generic implementation to a microblaze specific
> one, sets the generic internal as a ENOSYS, and cleanups the Linux
> generic implementation.
> 
> Also, the microblaze generic implementation first try to issue
> pselect instead of use the fallback (since it is expect that if
> the microblaze usage does rely on pselect, a sufficient updated
> kernel will be used).  Microblaze defines __NR_pselect6 for Linux
> v3.2, although it was only wire-up on v3.15 (and the syscall number
> is the same as previous defined).

I'll raise the same issue here I raise whenever someone proposes having 
fallback code for old kernels without a corresponding __ASSUME_* macro.

We need a simple and uniform way, when increasing the minimum kernel 
version, to find all the fallback code that can be removed.  That means 
__ASSUME_* macros in kernel-features.h based on __LINUX_KERNEL_VERSION.  
A free-form text comment mentioning "3.15" is no good for that purpose as 
there is no sensible automated way to find all such comments when 
increasing the minimum from 3.2 to 4.4 (for example, as the next such 
increase that seems to make sense in terms of the cleanups it enables).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH 1/2] misc: Set generic pselect as ENOSYS

[Adhemerval Zanella]

On 14/11/2019 17:49, Joseph Myers wrote:
> On Thu, 14 Nov 2019, Adhemerval Zanella wrote:
> 
>> The generic pselect implementation has the very specific race condition
>> that motived the creation of the pselect syscall (no atomicity in
>> signal mask set/reset).  Using it as generic implementation is
>> counterproductive  Also currently only microblaze uses it as fallback
>> when used on kernel prior 3.15.
>>
>> This patch moves the generic implementation to a microblaze specific
>> one, sets the generic internal as a ENOSYS, and cleanups the Linux
>> generic implementation.
>>
>> Also, the microblaze generic implementation first try to issue
>> pselect instead of use the fallback (since it is expect that if
>> the microblaze usage does rely on pselect, a sufficient updated
>> kernel will be used).  Microblaze defines __NR_pselect6 for Linux
>> v3.2, although it was only wire-up on v3.15 (and the syscall number
>> is the same as previous defined).
> 
> I'll raise the same issue here I raise whenever someone proposes having 
> fallback code for old kernels without a corresponding __ASSUME_* macro.
> 
> We need a simple and uniform way, when increasing the minimum kernel 
> version, to find all the fallback code that can be removed.  That means 
> __ASSUME_* macros in kernel-features.h based on __LINUX_KERNEL_VERSION.  
> A free-form text comment mentioning "3.15" is no good for that purpose as 
> there is no sensible automated way to find all such comments when 
> increasing the minimum from 3.2 to 4.4 (for example, as the next such 
> increase that seems to make sense in terms of the cleanups it enables).
> 

My issue with __ASSUME_* is it adds build permutations that ideally should
be checked to get a full tests coverage.  It means we should build and test
with all possible --enable-kernel version for every architecture that might 
be affected by a __ASSUME_*.  I tend to see always building the possible
fallback and assuming a newer kernel tends to be a simpler way.

In any case, I don't have a strong opinion on pselect and I will send an
updated version with __ASSUME_PSELECT back.  It should simplify the
time64 support and I will expect that no changed on microblaze version
should be required.

[PATCH v2] misc: Set generic pselect as ENOSYS

[Adhemerval Zanella]

Changes from previous version:

  - Added some include cleanups.

  - Add the __ASSUME_PSELECT usage on microblaze version.

--

The generic pselect implementation has the very specific race condition
that motived the creation of the pselect syscall (no atomicity in
signal mask set/reset).  Using it as generic implementation is
counterproductive  Also currently only microblaze uses it as fallback
when used on kernel prior 3.15.

This patch moves the generic implementation to a microblaze specific
one, sets the generic internal as a ENOSYS, and cleanups the Linux
generic implementation.

The microblaze implementation mimics the previous Linux generic one,
where it either uses pselect6 directly if __ASSUME_PSELECT or a
first try pselect6 then the fallback otherwise.

Checked on x86_64-linux-gnu and microblaze-linux-gnu.
---
 misc/pselect.c                               | 46 +-----------
 sysdeps/unix/sysv/linux/microblaze/pselect.c | 73 ++++++++++++++++++++
 sysdeps/unix/sysv/linux/pselect.c            | 37 ++--------
 3 files changed, 80 insertions(+), 76 deletions(-)
 create mode 100644 sysdeps/unix/sysv/linux/microblaze/pselect.c

diff --git a/misc/pselect.c b/misc/pselect.c
index 76ded850a5..b53d9cdcc1 100644
--- a/misc/pselect.c
+++ b/misc/pselect.c
@@ -17,12 +17,7 @@
    <https://www.gnu.org/licenses/>.  */
 
 #include <errno.h>
-#include <signal.h>
-#include <stddef.h>	/* For NULL.  */
-#include <sys/time.h>
 #include <sys/select.h>
-#include <sysdep-cancel.h>
-
 
 /* Check the first NFDS descriptors each in READFDS (if not NULL) for read
    readiness, in WRITEFDS (if not NULL) for write readiness, and in EXCEPTFDS
@@ -34,43 +29,8 @@ int
 __pselect (int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
 	   const struct timespec *timeout, const sigset_t *sigmask)
 {
-  struct timeval tval;
-  int retval;
-  sigset_t savemask;
-
-  /* Change nanosecond number to microseconds.  This might mean losing
-     precision and therefore the `pselect` should be available.  But
-     for now it is hardly found.  */
-  if (timeout != NULL)
-    {
-      /* Catch bugs which would be hidden by the TIMESPEC_TO_TIMEVAL
-	 computations.  The division by 1000 truncates values.  */
-      if (__glibc_unlikely (timeout->tv_nsec < 0))
-	{
-	  __set_errno (EINVAL);
-	  return -1;
-	}
-
-      TIMESPEC_TO_TIMEVAL (&tval, timeout);
-    }
-
-  /* The setting and restoring of the signal mask and the select call
-     should be an atomic operation.  This can't be done without kernel
-     help.  */
-  if (sigmask != NULL)
-    __sigprocmask (SIG_SETMASK, sigmask, &savemask);
-
-  /* Note the pselect() is a cancellation point.  But since we call
-     select() which itself is a cancellation point we do not have
-     to do anything here.  */
-  retval = __select (nfds, readfds, writefds, exceptfds,
-		     timeout != NULL ? &tval : NULL);
-
-  if (sigmask != NULL)
-    __sigprocmask (SIG_SETMASK, &savemask, NULL);
-
-  return retval;
+  __set_errno (ENOSYS);
+  return -1;
 }
-#ifndef __pselect
 weak_alias (__pselect, pselect)
-#endif
+stub_warning (pselect)
diff --git a/sysdeps/unix/sysv/linux/microblaze/pselect.c b/sysdeps/unix/sysv/linux/microblaze/pselect.c
new file mode 100644
index 0000000000..561ff99fb4
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/microblaze/pselect.c
@@ -0,0 +1,73 @@
+/* Synchronous I/O multiplexing.  Linux/microblaze version.
+   Copyright (C) 2019 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
+   <https://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <signal.h>
+#include <time.h>
+#include <sys/poll.h>
+#include <sysdep-cancel.h>
+
+#ifndef __ASSUME_PSELECT
+# define __pselect __pselect_syscall
+#endif
+
+/* If pselect is supported, just use the Linux generic implementation.  */
+#include <sysdeps/unix/sysv/linux/pselect.c>
+
+#ifndef __ASSUME_PSELECT
+# undef __pselect
+int
+__pselect (int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
+	   const struct timespec *timeout, const sigset_t *sigmask)
+{
+  int ret = __pselect_syscall (nfds, readfds, writefds, exceptfds, timeout,
+			       sigmask);
+  if (ret < 0 && errno != ENOSYS)
+    return ret;
+
+  /* The fallback uses 'select' which shows the race condition regarding
+     signal mask set/restore, requires two additional syscalls, and has
+     a worse timeout precision (microseconds instead of nanoseconds).  */
+
+  struct timeval tval, *ptval = NULL;
+  if (timeout != NULL)
+    {
+      if (! valid_nanoseconds (timeout->tv_nsec))
+	{
+	  __set_errno (EINVAL);
+	  return -1;
+	}
+
+      TIMESPEC_TO_TIMEVAL (&tval, timeout);
+      ptval = &tval;
+    }
+
+  sigset_t savemask;
+  if (sigmask != NULL)
+    __sigprocmask (SIG_SETMASK, sigmask, &savemask);
+
+  /* select itself is a cancellation entrypoint.  */
+  ret = __select (nfds, readfds, writefds, exceptfds, ptval);
+
+  if (sigmask != NULL)
+    __sigprocmask (SIG_SETMASK, &savemask, NULL);
+
+  return ret;
+}
+weak_alias (__pselect, pselect)
+#endif
diff --git a/sysdeps/unix/sysv/linux/pselect.c b/sysdeps/unix/sysv/linux/pselect.c
index acda3e0cdd..7a3dc8c4ed 100644
--- a/sysdeps/unix/sysv/linux/pselect.c
+++ b/sysdeps/unix/sysv/linux/pselect.c
@@ -16,23 +16,9 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
-#include <errno.h>
-#include <signal.h>
-#include <time.h>
-#include <sys/poll.h>
-#include <kernel-features.h>
+#include <sys/select.h>
 #include <sysdep-cancel.h>
 
-
-#ifdef __NR_pselect6
-# ifndef __ASSUME_PSELECT
-static int __generic_pselect (int nfds, fd_set *readfds, fd_set *writefds,
-			      fd_set *exceptfds,
-			      const struct timespec *timeout,
-			      const sigset_t *sigmask);
-# endif
-
-
 int
 __pselect (int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
 	   const struct timespec *timeout, const sigset_t *sigmask)
@@ -59,24 +45,9 @@ __pselect (int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
   data.ss = (__syscall_ulong_t) (uintptr_t) sigmask;
   data.ss_len = _NSIG / 8;
 
-  int result = SYSCALL_CANCEL (pselect6, nfds, readfds, writefds, exceptfds,
-                               timeout, &data);
-
-# ifndef __ASSUME_PSELECT
-  if (result == -1 && errno == ENOSYS)
-    result = __generic_pselect (nfds, readfds, writefds, exceptfds, timeout,
-				sigmask);
-# endif
-
-  return result;
+  return SYSCALL_CANCEL (pselect6, nfds, readfds, writefds, exceptfds,
+                         timeout, &data);
 }
+#ifndef __pselect
 weak_alias (__pselect, pselect)
-
-# ifndef __ASSUME_PSELECT
-#  define __pselect static __generic_pselect
-# endif
-#endif
-
-#ifndef __ASSUME_PSELECT
-# include <misc/pselect.c>
 #endif
-- 
2.17.1

Re: [PATCH v2] misc: Set generic pselect as ENOSYS

[Adhemerval Zanella]

I will commit this patch with Lukasz Majewski remark if no one opposes.

On 14/11/2019 19:00, Adhemerval Zanella wrote:
> Changes from previous version:
> 
>   - Added some include cleanups.
> 
>   - Add the __ASSUME_PSELECT usage on microblaze version.
> 
> --
> 
> The generic pselect implementation has the very specific race condition
> that motived the creation of the pselect syscall (no atomicity in
> signal mask set/reset).  Using it as generic implementation is
> counterproductive  Also currently only microblaze uses it as fallback
> when used on kernel prior 3.15.
> 
> This patch moves the generic implementation to a microblaze specific
> one, sets the generic internal as a ENOSYS, and cleanups the Linux
> generic implementation.
> 
> The microblaze implementation mimics the previous Linux generic one,
> where it either uses pselect6 directly if __ASSUME_PSELECT or a
> first try pselect6 then the fallback otherwise.
> 
> Checked on x86_64-linux-gnu and microblaze-linux-gnu.
> ---
>  misc/pselect.c                               | 46 +-----------
>  sysdeps/unix/sysv/linux/microblaze/pselect.c | 73 ++++++++++++++++++++
>  sysdeps/unix/sysv/linux/pselect.c            | 37 ++--------
>  3 files changed, 80 insertions(+), 76 deletions(-)
>  create mode 100644 sysdeps/unix/sysv/linux/microblaze/pselect.c
> 
> diff --git a/misc/pselect.c b/misc/pselect.c
> index 76ded850a5..b53d9cdcc1 100644
> --- a/misc/pselect.c
> +++ b/misc/pselect.c
> @@ -17,12 +17,7 @@
>     <https://www.gnu.org/licenses/>.  */
>  
>  #include <errno.h>
> -#include <signal.h>
> -#include <stddef.h>	/* For NULL.  */
> -#include <sys/time.h>
>  #include <sys/select.h>
> -#include <sysdep-cancel.h>
> -
>  
>  /* Check the first NFDS descriptors each in READFDS (if not NULL) for read
>     readiness, in WRITEFDS (if not NULL) for write readiness, and in EXCEPTFDS
> @@ -34,43 +29,8 @@ int
>  __pselect (int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
>  	   const struct timespec *timeout, const sigset_t *sigmask)
>  {
> -  struct timeval tval;
> -  int retval;
> -  sigset_t savemask;
> -
> -  /* Change nanosecond number to microseconds.  This might mean losing
> -     precision and therefore the `pselect` should be available.  But
> -     for now it is hardly found.  */
> -  if (timeout != NULL)
> -    {
> -      /* Catch bugs which would be hidden by the TIMESPEC_TO_TIMEVAL
> -	 computations.  The division by 1000 truncates values.  */
> -      if (__glibc_unlikely (timeout->tv_nsec < 0))
> -	{
> -	  __set_errno (EINVAL);
> -	  return -1;
> -	}
> -
> -      TIMESPEC_TO_TIMEVAL (&tval, timeout);
> -    }
> -
> -  /* The setting and restoring of the signal mask and the select call
> -     should be an atomic operation.  This can't be done without kernel
> -     help.  */
> -  if (sigmask != NULL)
> -    __sigprocmask (SIG_SETMASK, sigmask, &savemask);
> -
> -  /* Note the pselect() is a cancellation point.  But since we call
> -     select() which itself is a cancellation point we do not have
> -     to do anything here.  */
> -  retval = __select (nfds, readfds, writefds, exceptfds,
> -		     timeout != NULL ? &tval : NULL);
> -
> -  if (sigmask != NULL)
> -    __sigprocmask (SIG_SETMASK, &savemask, NULL);
> -
> -  return retval;
> +  __set_errno (ENOSYS);
> +  return -1;
>  }
> -#ifndef __pselect
>  weak_alias (__pselect, pselect)
> -#endif
> +stub_warning (pselect)
> diff --git a/sysdeps/unix/sysv/linux/microblaze/pselect.c b/sysdeps/unix/sysv/linux/microblaze/pselect.c
> new file mode 100644
> index 0000000000..561ff99fb4
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/microblaze/pselect.c
> @@ -0,0 +1,73 @@
> +/* Synchronous I/O multiplexing.  Linux/microblaze version.
> +   Copyright (C) 2019 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
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <errno.h>
> +#include <signal.h>
> +#include <time.h>
> +#include <sys/poll.h>
> +#include <sysdep-cancel.h>
> +
> +#ifndef __ASSUME_PSELECT
> +# define __pselect __pselect_syscall
> +#endif
> +
> +/* If pselect is supported, just use the Linux generic implementation.  */
> +#include <sysdeps/unix/sysv/linux/pselect.c>
> +
> +#ifndef __ASSUME_PSELECT
> +# undef __pselect
> +int
> +__pselect (int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
> +	   const struct timespec *timeout, const sigset_t *sigmask)
> +{
> +  int ret = __pselect_syscall (nfds, readfds, writefds, exceptfds, timeout,
> +			       sigmask);
> +  if (ret < 0 && errno != ENOSYS)
> +    return ret;
> +
> +  /* The fallback uses 'select' which shows the race condition regarding
> +     signal mask set/restore, requires two additional syscalls, and has
> +     a worse timeout precision (microseconds instead of nanoseconds).  */
> +
> +  struct timeval tval, *ptval = NULL;
> +  if (timeout != NULL)
> +    {
> +      if (! valid_nanoseconds (timeout->tv_nsec))
> +	{
> +	  __set_errno (EINVAL);
> +	  return -1;
> +	}
> +
> +      TIMESPEC_TO_TIMEVAL (&tval, timeout);
> +      ptval = &tval;
> +    }
> +
> +  sigset_t savemask;
> +  if (sigmask != NULL)
> +    __sigprocmask (SIG_SETMASK, sigmask, &savemask);
> +
> +  /* select itself is a cancellation entrypoint.  */
> +  ret = __select (nfds, readfds, writefds, exceptfds, ptval);
> +
> +  if (sigmask != NULL)
> +    __sigprocmask (SIG_SETMASK, &savemask, NULL);
> +
> +  return ret;
> +}
> +weak_alias (__pselect, pselect)
> +#endif
> diff --git a/sysdeps/unix/sysv/linux/pselect.c b/sysdeps/unix/sysv/linux/pselect.c
> index acda3e0cdd..7a3dc8c4ed 100644
> --- a/sysdeps/unix/sysv/linux/pselect.c
> +++ b/sysdeps/unix/sysv/linux/pselect.c
> @@ -16,23 +16,9 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> -#include <errno.h>
> -#include <signal.h>
> -#include <time.h>
> -#include <sys/poll.h>
> -#include <kernel-features.h>
> +#include <sys/select.h>
>  #include <sysdep-cancel.h>
>  
> -
> -#ifdef __NR_pselect6
> -# ifndef __ASSUME_PSELECT
> -static int __generic_pselect (int nfds, fd_set *readfds, fd_set *writefds,
> -			      fd_set *exceptfds,
> -			      const struct timespec *timeout,
> -			      const sigset_t *sigmask);
> -# endif
> -
> -
>  int
>  __pselect (int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
>  	   const struct timespec *timeout, const sigset_t *sigmask)
> @@ -59,24 +45,9 @@ __pselect (int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
>    data.ss = (__syscall_ulong_t) (uintptr_t) sigmask;
>    data.ss_len = _NSIG / 8;
>  
> -  int result = SYSCALL_CANCEL (pselect6, nfds, readfds, writefds, exceptfds,
> -                               timeout, &data);
> -
> -# ifndef __ASSUME_PSELECT
> -  if (result == -1 && errno == ENOSYS)
> -    result = __generic_pselect (nfds, readfds, writefds, exceptfds, timeout,
> -				sigmask);
> -# endif
> -
> -  return result;
> +  return SYSCALL_CANCEL (pselect6, nfds, readfds, writefds, exceptfds,
> +                         timeout, &data);
>  }
> +#ifndef __pselect
>  weak_alias (__pselect, pselect)
> -
> -# ifndef __ASSUME_PSELECT
> -#  define __pselect static __generic_pselect
> -# endif
> -#endif
> -
> -#ifndef __ASSUME_PSELECT
> -# include <misc/pselect.c>
>  #endif
> 

Re: [PATCH 1/2] misc: Set generic pselect as ENOSYS

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 848 bytes --]

Dear Adhemerval,

> +__pselect (int nfds, fd_set *readfds, fd_set *writefds, fd_set
> *exceptfds,
> +	   const struct timespec *timeout, const sigset_t *sigmask)
> +{
> +  int ret = __pselect_syscall (nfds, readfds, writefds, exceptfds,
> timeout,
> +			       sigmask);
> +  if (ret < 0 && errno != ENOSYS)
> +    return ret;

Is the above condition correct?

The pselect returns -1 on error or >= 0 on success.

Shouldn't it be instead:

if (! (ret == -1 && errno == ENOSYS))

Only when pselect is not supported we shall fallback to select based
implementation.


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] misc: Set generic pselect as ENOSYS

[Adhemerval Zanella]

[-- Attachment #1.1: Type: text/plain, Size: 736 bytes --]



On 14/11/2019 19:20, Lukasz Majewski wrote:
> Dear Adhemerval,
> 
>> +__pselect (int nfds, fd_set *readfds, fd_set *writefds, fd_set
>> *exceptfds,
>> +	   const struct timespec *timeout, const sigset_t *sigmask)
>> +{
>> +  int ret = __pselect_syscall (nfds, readfds, writefds, exceptfds,
>> timeout,
>> +			       sigmask);
>> +  if (ret < 0 && errno != ENOSYS)
>> +    return ret;
> 
> Is the above condition correct?
> 
> The pselect returns -1 on error or >= 0 on success.
> 
> Shouldn't it be instead:
> 
> if (! (ret == -1 && errno == ENOSYS))
> 
> Only when pselect is not supported we shall fallback to select based
> implementation.

Indeed, we need to take care of ret being 0.  Fixed locally.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]