[RFC,v2,06/20] sysdeps/futex: Use futex_time64 if avaliable

Message ID 1e92bb4f2cc06e8f3adfbd9de653c8e93c0d288a.1561421042.git.alistair.francis@wdc.com
State New
Headers show
Series
  • RISC-V glibc port for the 32-bit
Related show

Commit Message

Alistair Francis June 25, 2019, 12:09 a.m.
The futex syscall isn't avaliable on newer 32-bit architectures
(such as RV32) so if futex_time64 is avaliable lets use that instead.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

---
 ChangeLog                                    |  1 +
 sysdeps/unix/sysv/linux/lowlevellock-futex.h | 28 ++++++++++++++------
 2 files changed, 21 insertions(+), 8 deletions(-)

-- 
2.22.0

Comments

Florian Weimer June 25, 2019, 11:14 a.m. | #1
* Alistair Francis:

> +/* __NR_futex isn't defined on all archs (RV32) so use __NR_futex_time64 */

> +#ifdef __NR_futex_time64

> +# define lll_futex_syscall(nargs, futexp, op, ...)                              \

> +   ({                                                                           \

> +     INTERNAL_SYSCALL_DECL (__err);                                             \

> +     long int __ret = INTERNAL_SYSCALL (futex_time64, __err, nargs, futexp, op, \

> +                                        __VA_ARGS__);                           \

> +     (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (__ret, __err))                \

> +      ? -INTERNAL_SYSCALL_ERRNO (__ret, __err) : 0);                            \

> +   })

> +#else

> +# define lll_futex_syscall(nargs, futexp, op, ...)                       \

> +   ({                                                                    \

> +     INTERNAL_SYSCALL_DECL (__err);                                      \

> +     long int __ret = INTERNAL_SYSCALL (futex, __err, nargs, futexp, op, \

> +                                        __VA_ARGS__);                    \

> +     (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (__ret, __err))         \

> +      ? -INTERNAL_SYSCALL_ERRNO (__ret, __err) : 0);                     \

> +   })

> +#endif


I don't think a compile-time check in generic code is correct here.  It
will cause binaries to fail to run on older kernels which do not have
the system call.  This is obviously not a concern for 32-bit RISC-V, but
it is not acceptable for i386, for example.

I still think you should define __NR_futex in RV32 <sysdep.h>.

Thanks,
Florian
Andreas Schwab June 25, 2019, 11:26 a.m. | #2
On Jun 25 2019, Florian Weimer <fweimer@redhat.com> wrote:

> * Alistair Francis:

>

>> +/* __NR_futex isn't defined on all archs (RV32) so use __NR_futex_time64 */

>> +#ifdef __NR_futex_time64

>> +# define lll_futex_syscall(nargs, futexp, op, ...)                              \

>> +   ({                                                                           \

>> +     INTERNAL_SYSCALL_DECL (__err);                                             \

>> +     long int __ret = INTERNAL_SYSCALL (futex_time64, __err, nargs, futexp, op, \

>> +                                        __VA_ARGS__);                           \

>> +     (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (__ret, __err))                \

>> +      ? -INTERNAL_SYSCALL_ERRNO (__ret, __err) : 0);                            \

>> +   })

>> +#else

>> +# define lll_futex_syscall(nargs, futexp, op, ...)                       \

>> +   ({                                                                    \

>> +     INTERNAL_SYSCALL_DECL (__err);                                      \

>> +     long int __ret = INTERNAL_SYSCALL (futex, __err, nargs, futexp, op, \

>> +                                        __VA_ARGS__);                    \

>> +     (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (__ret, __err))         \

>> +      ? -INTERNAL_SYSCALL_ERRNO (__ret, __err) : 0);                     \

>> +   })

>> +#endif

>

> I don't think a compile-time check in generic code is correct here.  It

> will cause binaries to fail to run on older kernels which do not have

> the system call.  This is obviously not a concern for 32-bit RISC-V, but

> it is not acceptable for i386, for example.

>

> I still think you should define __NR_futex in RV32 <sysdep.h>.


For existing 32-bit archs there needs to be a runtime check (if both
__NR_futex and __NR_futex_time64 are defined).

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Arnd Bergmann June 25, 2019, 11:41 a.m. | #3
On Tue, Jun 25, 2019 at 1:26 PM Andreas Schwab <schwab@suse.de> wrote:
> On Jun 25 2019, Florian Weimer <fweimer@redhat.com> wrote:

> >

> > I don't think a compile-time check in generic code is correct here.  It

> > will cause binaries to fail to run on older kernels which do not have

> > the system call.  This is obviously not a concern for 32-bit RISC-V, but

> > it is not acceptable for i386, for example.

> >

> > I still think you should define __NR_futex in RV32 <sysdep.h>.

>

> For existing 32-bit archs there needs to be a runtime check (if both

> __NR_futex and __NR_futex_time64 are defined).


That opens the question what type this function should take,
because one of the two will require the time argument to be
converted.

For instance pthread_timedjoin_np() is a public interface
that will need an additional version to deal with time64.
Should pthread_timedjoin_np() convert the user timespec
to __timespec64 and pass it down to lll_futex_syscall(),
which then converts it back to timespec before calling
__NR_futex() on 32-bit architectures, or is there a way to
avoid the double conversion?

        Arnd

      Arnd
Florian Weimer June 25, 2019, 12:06 p.m. | #4
* Arnd Bergmann:

> On Tue, Jun 25, 2019 at 1:26 PM Andreas Schwab <schwab@suse.de> wrote:

>> On Jun 25 2019, Florian Weimer <fweimer@redhat.com> wrote:

>> >

>> > I don't think a compile-time check in generic code is correct here.  It

>> > will cause binaries to fail to run on older kernels which do not have

>> > the system call.  This is obviously not a concern for 32-bit RISC-V, but

>> > it is not acceptable for i386, for example.

>> >

>> > I still think you should define __NR_futex in RV32 <sysdep.h>.

>>

>> For existing 32-bit archs there needs to be a runtime check (if both

>> __NR_futex and __NR_futex_time64 are defined).

>

> That opens the question what type this function should take,

> because one of the two will require the time argument to be

> converted.

>

> For instance pthread_timedjoin_np() is a public interface

> that will need an additional version to deal with time64.

> Should pthread_timedjoin_np() convert the user timespec

> to __timespec64 and pass it down to lll_futex_syscall(),

> which then converts it back to timespec before calling

> __NR_futex() on 32-bit architectures, or is there a way to

> avoid the double conversion?


My expectation is that glibc will not provide a 32-bit struct timespec
for architectures which do not have a 32-bit __NR_futex system call.
It's a 64-bit only world.  Before the 2038 deadline, we will not support
kernels that removed support for the 32-bit system calls.

Based on that, I expect that the 32-bit struct timespec variant of
pthread_timedjoin_np calls futex, and the 64-bit variant (probably named
pthread_timedjoin64_np or something like that, or used by default under
appropriate feature selection macros) will call futex_time64.  This has
the advantage that the type of call is visible at the syscall layer, and
could eventually trigger kenrel warnings.  With translation, that
wouldn't be the case.  It also increases compatibility with seccomp
filters which know of the old system calls, but not the new ones.

Thanks,
Florian
Arnd Bergmann June 25, 2019, 1:32 p.m. | #5
On Tue, Jun 25, 2019 at 2:07 PM Florian Weimer <fweimer@redhat.com> wrote:
> * Arnd Bergmann:

> > For instance pthread_timedjoin_np() is a public interface

> > that will need an additional version to deal with time64.

> > Should pthread_timedjoin_np() convert the user timespec

> > to __timespec64 and pass it down to lll_futex_syscall(),

> > which then converts it back to timespec before calling

> > __NR_futex() on 32-bit architectures, or is there a way to

> > avoid the double conversion?

>

> My expectation is that glibc will not provide a 32-bit struct timespec

> for architectures which do not have a 32-bit __NR_futex system call.

> It's a 64-bit only world.  Before the 2038 deadline, we will not support

> kernels that removed support for the 32-bit system calls.

>

> Based on that, I expect that the 32-bit struct timespec variant of

> pthread_timedjoin_np calls futex, and the 64-bit variant (probably named

> pthread_timedjoin64_np or something like that, or used by default under

> appropriate feature selection macros) will call futex_time64.  This has

> the advantage that the type of call is visible at the syscall layer, and

> could eventually trigger kenrel warnings.  With translation, that

> wouldn't be the case.  It also increases compatibility with seccomp

> filters which know of the old system calls, but not the new ones.


Ok, that's perfect as far as I'm concerned. From earlier discussions
I had understood that you were planning to avoid the associated
code duplication and only have one internal implementation for
the two external interfaces.

      Arnd

Patch

diff --git a/ChangeLog b/ChangeLog
index b90c5ab60c..a700783ef3 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -3,6 +3,7 @@ 
 	* nptl/thrd_sleep.c: Use clock_nanosleep_time64 instead of nanosleep.
 	* sysdeps/unix/sysv/linux/nanosleep.c: Likewise.
 	* sysdeps/unix/sysv/linux/nanosleep_nocancel.c: Likewise.
+	* sysdeps/unix/sysv/linux/lowlevellock-futex.h: Use __NR_futex_time64 if we don't have __NR_futex.
 
 2019-06-20  Dmitry V. Levin  <ldv@altlinux.org>
 	    Florian Weimer  <fweimer@redhat.com>
diff --git a/sysdeps/unix/sysv/linux/lowlevellock-futex.h b/sysdeps/unix/sysv/linux/lowlevellock-futex.h
index 030a14b8dc..e310542bfa 100644
--- a/sysdeps/unix/sysv/linux/lowlevellock-futex.h
+++ b/sysdeps/unix/sysv/linux/lowlevellock-futex.h
@@ -65,14 +65,26 @@ 
   (((fl) | FUTEX_PRIVATE_FLAG) ^ (private))
 #endif
 
-#define lll_futex_syscall(nargs, futexp, op, ...)                       \
-  ({                                                                    \
-    INTERNAL_SYSCALL_DECL (__err);                                      \
-    long int __ret = INTERNAL_SYSCALL (futex, __err, nargs, futexp, op, \
-				       __VA_ARGS__);                    \
-    (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (__ret, __err))         \
-     ? -INTERNAL_SYSCALL_ERRNO (__ret, __err) : 0);                     \
-  })
+/* __NR_futex isn't defined on all archs (RV32) so use __NR_futex_time64 */
+#ifdef __NR_futex_time64
+# define lll_futex_syscall(nargs, futexp, op, ...)                              \
+   ({                                                                           \
+     INTERNAL_SYSCALL_DECL (__err);                                             \
+     long int __ret = INTERNAL_SYSCALL (futex_time64, __err, nargs, futexp, op, \
+                                        __VA_ARGS__);                           \
+     (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (__ret, __err))                \
+      ? -INTERNAL_SYSCALL_ERRNO (__ret, __err) : 0);                            \
+   })
+#else
+# define lll_futex_syscall(nargs, futexp, op, ...)                       \
+   ({                                                                    \
+     INTERNAL_SYSCALL_DECL (__err);                                      \
+     long int __ret = INTERNAL_SYSCALL (futex, __err, nargs, futexp, op, \
+                                        __VA_ARGS__);                    \
+     (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (__ret, __err))         \
+      ? -INTERNAL_SYSCALL_ERRNO (__ret, __err) : 0);                     \
+   })
+#endif
 
 #define lll_futex_wait(futexp, val, private) \
   lll_futex_timed_wait (futexp, val, NULL, private)