On Sat, 4 Apr 2020, Paul Irofti wrote:
> Here is a proper diff (both sys and libc into one).
Okay, bunch o' comments and thoughts of varying strength of opinion.
> diff --git lib/libc/Symbols.list lib/libc/Symbols.list
> index f9aa62ab6e8..4fa37a835aa 100644
> --- lib/libc/Symbols.list
> +++ lib/libc/Symbols.list
> @@ -86,7 +86,10 @@ _thread_sys_fstatat
> _thread_sys_fstatfs
> _thread_sys_fsync
> _thread_sys_ftruncate
> -_thread_sys_futex
> +_thread_sys_ofutex
The ofutex syscall should not be built (or exported) by libc. The kernel
needs to continue to provide it for the normal period to support older
libc's where futex(2) is a direct syscall,but no application does, would,
or should invoke ofutex() under that name.
> +_thread_sys_futex_wait
> +_thread_sys_futex_wake
> +_thread_sys_futex_requeue
glibc has internal inline functions futex_wait() and futex_wake() and
there has been at least discussion about exporting some version of them.
If our signatures matched the last-best-proposal over there (which was
dropped, mind you) then I would be tempted to use those names. If not,
then maybe go with _futex_wait() and _futex_wake()?
FUTEX_REQUEUE is the old bad one, with no val3 argument that's checked
before the operation. Our libc/libpthread don't actually use them, and in
the Linux world glibc switched completely to FUTEX_CMP_REQUEUE. Perhaps
we should drop support for FUTEX_REQUEUE (major bump, yah) and add
_futex_cmp_requeue(2) when we need it?
> --- lib/libc/gen/sigwait.c
> +++ lib/libc/gen/sigwait.c
This is unrelated to futex stuff. :)
> --- lib/libc/hidden/sys/futex.h
> +++ lib/libc/hidden/sys/futex.h
> @@ -20,6 +20,8 @@
>
> #include_next <sys/futex.h>
>
> -PROTO_NORMAL(futex);
You need to keep this as long as futex() is used internally. Once futex()
is no longer used in libc then this should change to
"PROTO_DEPRECATED(futex);" and the "DEF_STRONG(futex);" line should be
deleted.
(If you have DEF_STRONG(foo) or DEF_WEAK(foo), then you must have
PROTO_NORMAL(foo). c.f libc/include/README II.B.2)
> --- lib/libc/thread/synch.h
> +++ lib/libc/thread/synch.h
> @@ -19,10 +19,33 @@
> #include <sys/time.h>
> #include <sys/futex.h>
>
> +static inline int
> +_futex(volatile uint32_t *p, int op, int val, const struct timespec
> *timeout, uint32_t *g)
> +{
> + int flags = 0;
> +
> + if (op & FUTEX_PRIVATE_FLAG)
> + flags |= FT_PRIVATE;
> +
> + switch (op) {
> + case FUTEX_WAIT:
> + case FUTEX_WAIT_PRIVATE:
> + return futex_wait(p, val, timeout, flags);
> + case FUTEX_WAKE:
> + case FUTEX_WAKE_PRIVATE:
> + return futex_wake(p, val, flags);
> + case FUTEX_REQUEUE:
> + case FUTEX_REQUEUE_PRIVATE:
> + return futex_requeue(p, val, g, timeout, flags);
> + }
> +
> + return ENOSYS;
> +}
I would put this logic directly into futex()'s definition, and then...
> static inline int
> _wake(volatile uint32_t *p, int n)
> {
> - return futex(p, FUTEX_WAKE_PRIVATE, n, NULL, NULL);
> + return _futex(p, FUTEX_WAKE_PRIVATE, n, NULL, NULL);
> }
have this just be
return futex_wake(p, n, FUTEX_WAKE_PRIVATE);
and similar for _twait() below.
(I would be inclined to use FUTEX_WAKE_PRIVATE as the flag for the new
syscalls instead of exposing FT_PRIVATE into the userspace API.)
> --- sys/kern/sys_futex.c
> +++ sys/kern/sys_futex.c
> @@ -83,9 +83,74 @@ futex_init(void)
> }
>
> int
> -sys_futex(struct proc *p, void *v, register_t *retval)
> +sys_futex_wait(struct proc *p, void *v, register_t *retval)
> {
> - struct sys_futex_args /* {
> + struct sys_futex_wait_args /* {
> + syscallarg(uint32_t *) f;
> + syscallarg(inr) val;
> + syscallarg(const struct timespec *) timeout;
> + syscallarg(int) flags;
> + } */ *uap = v;
> + uint32_t *uaddr = SCARG(uap, f);
> + uint32_t val = SCARG(uap, val);
> + const struct timespec *timeout = SCARG(uap, timeout);
> + int flags = SCARG(uap, flags);
> +
> + KERNEL_LOCK();
> + rw_enter_write(&ftlock);
> + *retval = futex_wait(uaddr, val, timeout, flags);
> + rw_exit_write(&ftlock);
> + KERNEL_UNLOCK();
> +
> + return 0;
> +}
> +
> +int
> +sys_futex_wake(struct proc *p, void *v, register_t *retval)
> +{
> + struct sys_futex_wake_args /* {
> + syscallarg(uint32_t *) f;
> + syscallarg(int) val;
> + syscallarg(int) flags;
> + } */ *uap = v;
> + uint32_t *uaddr = SCARG(uap, f);
> + uint32_t val = SCARG(uap, val);
> + int flags = SCARG(uap, flags);
> +
> + rw_enter_write(&ftlock);
> + *retval = futex_wake(uaddr, val, flags);
> + rw_exit_write(&ftlock);
> +
> + return 0;
> +}
> +
> +int
> +sys_futex_requeue(struct proc *p, void *v, register_t *retval)
> +{
> + struct sys_futex_requeue_args /* {
> + syscallarg(uint32_t *) f;
> + syscallarg(int) val;
> + syscallarg(uint32_t *) g;
> + syscallarg(const struct timespec *) timeout;
> + syscallarg(int) flags;
> + } */ *uap = v;
> + uint32_t *uaddr = SCARG(uap, f);
> + uint32_t val = SCARG(uap, val);
> + const struct timespec *timeout = SCARG(uap, timeout);
> + void *g = SCARG(uap, g);
> + int flags = SCARG(uap, flags);
> +
> + rw_enter_write(&ftlock);
> + *retval = futex_requeue(uaddr, val, g, (u_long)timeout, flags);
> + rw_exit_write(&ftlock);
> +
> + return 0;
> +}
> +
> +int
> +sys_ofutex(struct proc *p, void *v, register_t *retval)
> +{
> + struct sys_ofutex_args /* {
> syscallarg(uint32_t *) f;
> syscallarg(int) op;
> syscallarg(inr) val;
> diff --git sys/kern/syscalls.c sys/kern/syscalls.c
> index 93058f5cc1c..3821676d6cb 100644
> --- sys/kern/syscalls.c
> +++ sys/kern/syscalls.c
> @@ -1,4 +1,4 @@
> -/* $OpenBSD: syscalls.c,v 1.217 2020/03/18 19:35:00 anton Exp $ */
> +/* $OpenBSD$ */
>
> /*
> * System call names.
> @@ -103,7 +103,7 @@ char *syscallnames[] = {
> "setgroups", /* 80 = setgroups */
> "getpgrp", /* 81 = getpgrp */
> "setpgid", /* 82 = setpgid */
> - "futex", /* 83 = futex */
> + "ofutex", /* 83 = ofutex */
> "utimensat", /* 84 = utimensat */
> "futimens", /* 85 = futimens */
> "kbind", /* 86 = kbind */
> @@ -393,4 +393,7 @@ char *syscallnames[] = {
> "#328 (obsolete __tfork51)", /* 328 = obsolete __tfork51 */
> "__set_tcb", /* 329 = __set_tcb */
> "__get_tcb", /* 330 = __get_tcb */
> + "futex_wait", /* 331 = futex_wait */
> + "futex_wake", /* 332 = futex_wake */
> + "futex_requeue", /* 333 = futex_requeue */
> };
> diff --git sys/kern/syscalls.master sys/kern/syscalls.master
> index fb9f1a41c9b..ffef41db774 100644
> --- sys/kern/syscalls.master
> +++ sys/kern/syscalls.master
> @@ -186,7 +186,7 @@
> const gid_t *gidset); }
> 81 STD { int sys_getpgrp(void); }
> 82 STD { int sys_setpgid(pid_t pid, pid_t pgid); }
> -83 STD NOLOCK { int sys_futex(uint32_t *f, int op, int val, \
> +83 STD NOLOCK { int sys_ofutex(uint32_t *f, int op, int val, \
> const struct timespec *timeout, uint32_t *g); }
> 84 STD { int sys_utimensat(int fd, const char *path, \
> const struct timespec *times, int flag); }
> @@ -566,3 +566,9 @@
> 328 OBSOL __tfork51
> 329 STD NOLOCK { void sys___set_tcb(void *tcb); }
> 330 STD NOLOCK { void *sys___get_tcb(void); }
> +331 STD NOLOCK { int sys_futex_wait(uint32_t *f, int val, \
> + const struct timespec *timeout, int flags); }
> +332 STD NOLOCK { int sys_futex_wake(uint32_t *f, int val, int flags); }
> +333 STD NOLOCK { int sys_futex_requeue(uint32_t *f, int val, \
> + uint32_t *g, const struct timespec *timeout, \
> + int flags); }
> diff --git sys/sys/futex.h sys/sys/futex.h
> index 62e32c977da..e6d350c1c15 100644
> --- sys/sys/futex.h
> +++ sys/sys/futex.h
> @@ -23,11 +23,19 @@
> #include <sys/cdefs.h>
>
> __BEGIN_DECLS
> -int futex(volatile uint32_t *, int, int, const struct timespec *,
> - volatile uint32_t *);
> +/* int futex(volatile uint32_t *, int, int, const struct timespec *,
> + volatile uint32_t *); */
> +int futex_wait(uint32_t *f, int val, const struct timespec *timeout,
> + int flags);
> +int futex_wake(uint32_t *f, int val, int flags);
> +int futex_requeue(uint32_t *f, int val, uint32_t *g,
> + const struct timespec *timeout, int flags);
> __END_DECLS
The prototypes I think I was imagining (*without* doing the checking
against glibc's proposal that I suggested, thus _futex* names) would be
something like:
int _futex_wait(uint32_t *_futex, uint32_t _val, clockid_t _clock_id,
const struct timespec *_timeout, int _flags);
int _futex_wake(uint32_t *_futex, int _nr_wake, int _flags);
int _futex_cmp_requeue(uint32_t *_futex, int _nr_wake, int _nr_move,
uint32_t *_futex2, uint32_t _val, int _flags);
On reflection, my hack of marking futex(2) as no-error and never setting
errno was probably too clever (c.f. the kdump issue that spawned this all)
and these should instead fail and set errno like most other syscalls,
particularly since they should all have at least EINVAL for bad flags...
Philip
