Re: split futex into three
On Sun, 5 Apr 2020, Stuart Henderson wrote: > On 2020/04/05 10:28, Martin Pieuchot wrote: > > Another way to proceed would be to do a port grep for futex and see what > > the ecosystem is using. > > Sorry it's not filtered, but : > > https://junkpile.org/grep.futex.gz Sure looks like the only occurence of futex() used with FUTEX_REQUEUE (== 3) is the linux kernel test program. Everything else, including rust, is using FUTEX_CMP_REQUEUE or one of the PI operations (FUTEX_WAIT_REQUEUE_PI, FUTEX_CMP_REQUEUE_PI). Philip
Re: split futex into three
On 2020/04/05 10:28, Martin Pieuchot wrote: > Another way to proceed would be to do a port grep for futex and see what > the ecosystem is using. Sorry it's not filtered, but : https://junkpile.org/grep.futex.gz
Re: split futex into three
On 04/04/20(Sat) 22:30, Philip Guenther wrote: > [...] > 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()? Do you know if those are similar to Mesa's function with the same names found in lib/mesa/src/util/futex.h ? Is the wait variant accepting relative or absolute timeout? Another way to proceed would be to do a port grep for futex and see what the ecosystem is using. > 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? > > [...] > > 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); Isn't wake redundant with cmp_requeue?
Re: split futex into three
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
>
> -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
> #include
>
> +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();
> + *retval = futex_wait(uaddr, val, timeout, flags);
> + rw_exit_write();
> + 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();
> + *retval = futex_wake(uaddr, val, flags);
> + rw_exit_write();
> +
> + 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;
> +
Re: split futex into three
Here is a proper diff (both sys and libc into one).
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
+_thread_sys_futex_wait
+_thread_sys_futex_wake
+_thread_sys_futex_requeue
_thread_sys_futimens
_thread_sys_futimes
_thread_sys_getdents
@@ -282,7 +285,10 @@ fstatat
fstatfs
fsync
ftruncate
-futex
+ofutex
+futex_wait
+futex_wake
+futex_requeue
futimens
futimes
getdents
@@ -1685,6 +1691,7 @@ _spinunlock
_thread_atfork
_thread_dofork
_thread_set_callbacks
+futex
pthread_atfork
pthread_cond_broadcast
pthread_cond_destroy
diff --git lib/libc/gen/sigwait.c lib/libc/gen/sigwait.c
index 0321066fc81..4f6b4511d3f 100644
--- lib/libc/gen/sigwait.c
+++ lib/libc/gen/sigwait.c
@@ -58,6 +58,7 @@ sigwaitinfo(const sigset_t *set, siginfo_t *info)
LEAVE_CANCEL_POINT(ret == -1);
return (ret);
}
+#endif
int
sigtimedwait(const sigset_t *set, siginfo_t *info,
@@ -72,4 +73,3 @@ sigtimedwait(const sigset_t *set, siginfo_t *info,
LEAVE_CANCEL_POINT(ret == -1);
return (ret);
}
-#endif
diff --git lib/libc/hidden/sys/futex.h lib/libc/hidden/sys/futex.h
index dab25396b59..cec5ec68455 100644
--- lib/libc/hidden/sys/futex.h
+++ lib/libc/hidden/sys/futex.h
@@ -20,6 +20,8 @@
#include_next
-PROTO_NORMAL(futex);
+PROTO_NORMAL(futex_wait);
+PROTO_NORMAL(futex_wake);
+PROTO_NORMAL(futex_requeue);
#endif /* !_LIBC_SYS_FUTEX_H_ */
diff --git lib/libc/shlib_version lib/libc/shlib_version
index 06f98b01084..5fb0770494f 100644
--- lib/libc/shlib_version
+++ lib/libc/shlib_version
@@ -1,4 +1,4 @@
major=96
-minor=0
+minor=1
# note: If changes were made to include/thread_private.h or if system calls
# were added/changed then librthread/shlib_version must also be updated.
diff --git lib/libc/sys/Makefile.inc lib/libc/sys/Makefile.inc
index 34769576ced..e60c42267a9 100644
--- lib/libc/sys/Makefile.inc
+++ lib/libc/sys/Makefile.inc
@@ -87,7 +87,7 @@ DASM= ${ASM:.o=.do}
# syscalls that CANNOT FAIL. They can return whatever value they want,
# they just never want to set errno.
ASM_NOERR=__get_tcb.o __set_tcb.o __threxit.o __thrsleep.o __thrwakeup.o \
- futex.o \
+ ofutex.o futex_wait futex_wake futex_requeue \
getdtablecount.o getegid.o geteuid.o getgid.o getlogin_r.o \
getpgrp.o getpid.o getppid.o getrtable.o getthrid.o getuid.o \
issetugid.o \
diff --git lib/libc/thread/rthread_mutex.c lib/libc/thread/rthread_mutex.c
index c9a490033b3..9b2ca8f8fc0 100644
--- lib/libc/thread/rthread_mutex.c
+++ lib/libc/thread/rthread_mutex.c
@@ -284,3 +284,10 @@ pthread_mutex_unlock(pthread_mutex_t *mutexp)
return (0);
}
DEF_STRONG(pthread_mutex_unlock);
+
+int
+futex(volatile uint32_t *f, int op, int val, const struct timespec *timeout,
uint32_t *g)
+{
+ return _futex(f, op, val, timeout, g);
+}
+DEF_STRONG(futex);
diff --git lib/libc/thread/synch.h lib/libc/thread/synch.h
index 788890add89..d0e3dad7353 100644
--- lib/libc/thread/synch.h
+++ lib/libc/thread/synch.h
@@ -19,10 +19,33 @@
#include
#include
+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;
+}
+
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);
}
static inline int
@@ -31,7 +54,7 @@ _twait(volatile uint32_t *p, int val, clockid_t clockid,
const struct timespec *
struct timespec rel;
if (abs == NULL)
- return futex(p, FUTEX_WAIT_PRIVATE, val, NULL, NULL);
+ return _futex(p, FUTEX_WAIT_PRIVATE, val, NULL, NULL);
if (abs->tv_nsec >= 10 || clock_gettime(clockid, ))
return (EINVAL);
@@ -44,11 +67,11 @@ _twait(volatile uint32_t *p, int val, clockid_t clockid,
const struct timespec *
if (rel.tv_sec < 0)
return (ETIMEDOUT);
- return futex(p, FUTEX_WAIT_PRIVATE, val, , NULL);
+ return _futex(p, FUTEX_WAIT_PRIVATE, val, , NULL);
}
static inline int
_requeue(volatile uint32_t *p, int n, int m, volatile uint32_t *q)
{
- return futex(p, FUTEX_REQUEUE_PRIVATE, n, (void *)(long)m, q);
+ return _futex(p, FUTEX_REQUEUE_PRIVATE, n,
split futex into three
On Sat, Apr 04, 2020 at 03:53:50PM +0300, Paul Irofti wrote:
> > The real problem is that futex(2) is actually 3 different syscalls wrapped
> > into one. It was split into three then kdump could properly report
> > futex_wake(2) and futex_requeue(2) as returning a count, while
> > futex_wait(2) returns an errno. The existing 'switch' in sys_futex()
> > would just move to userspace's futex(3), provided for linux compat.
>
> I have such a diff from half a year ago. Let me get it back in shape and
> I'll send it back here.
I tried diffing sys and lib/libc at once but CVS is too retarded to do
that and diffing the whole src tree took forever. So I am sending
separated diffs for each.
When booting I get a warning from ld.so that it is not finding the
libc_futex_{wait,wake,requeue} symbols that I don't know how to fix.
Perhaps making a release would fix it but I can not do that right now
(not enough disk left).
This has not been tested enough and will probably blow up your
computer, so use it with care! Reports are welcome though :)
Here are the kernel bits:
%---
Index: kern/init_sysent.c
===
RCS file: /cvs/src/sys/kern/init_sysent.c,v
retrieving revision 1.218
diff -u -p -u -p -r1.218 init_sysent.c
--- kern/init_sysent.c 18 Mar 2020 19:35:00 - 1.218
+++ kern/init_sysent.c 4 Apr 2020 14:48:40 -
@@ -1,4 +1,4 @@
-/* $OpenBSD: init_sysent.c,v 1.218 2020/03/18 19:35:00 anton Exp $ */
+/* $OpenBSD$ */
/*
* System call switch table.
@@ -198,8 +198,8 @@ struct sysent sysent[] = {
sys_getpgrp }, /* 81 = getpgrp */
{ 2, s(struct sys_setpgid_args), 0,
sys_setpgid }, /* 82 = setpgid */
- { 5, s(struct sys_futex_args), SY_NOLOCK | 0,
- sys_futex },/* 83 = futex */
+ { 5, s(struct sys_ofutex_args), SY_NOLOCK | 0,
+ sys_ofutex }, /* 83 = ofutex */
{ 4, s(struct sys_utimensat_args), 0,
sys_utimensat },/* 84 = utimensat */
{ 2, s(struct sys_futimens_args), 0,
@@ -751,5 +751,11 @@ struct sysent sysent[] = {
sys___set_tcb },/* 329 = __set_tcb */
{ 0, 0, SY_NOLOCK | 0,
sys___get_tcb },/* 330 = __get_tcb */
+ { 4, s(struct sys_futex_wait_args), SY_NOLOCK | 0,
+ sys_futex_wait }, /* 331 = futex_wait */
+ { 3, s(struct sys_futex_wake_args), SY_NOLOCK | 0,
+ sys_futex_wake }, /* 332 = futex_wake */
+ { 5, s(struct sys_futex_requeue_args), SY_NOLOCK | 0,
+ sys_futex_requeue },/* 333 = futex_requeue */
};
Index: kern/kern_pledge.c
===
RCS file: /cvs/src/sys/kern/kern_pledge.c,v
retrieving revision 1.261
diff -u -p -u -p -r1.261 kern_pledge.c
--- kern/kern_pledge.c 15 Feb 2020 09:35:48 - 1.261
+++ kern/kern_pledge.c 4 Apr 2020 14:48:40 -
@@ -266,7 +266,10 @@ const uint64_t pledge_syscalls[SYS_MAXSY
*/
[SYS___tfork] = PLEDGE_STDIO,
[SYS_sched_yield] = PLEDGE_STDIO,
- [SYS_futex] = PLEDGE_STDIO,
+ [SYS_ofutex] = PLEDGE_STDIO,
+ [SYS_futex_wait] = PLEDGE_STDIO,
+ [SYS_futex_wake] = PLEDGE_STDIO,
+ [SYS_futex_requeue] = PLEDGE_STDIO,
[SYS___thrsleep] = PLEDGE_STDIO,
[SYS___thrwakeup] = PLEDGE_STDIO,
[SYS___threxit] = PLEDGE_STDIO,
Index: kern/sys_futex.c
===
RCS file: /cvs/src/sys/kern/sys_futex.c,v
retrieving revision 1.15
diff -u -p -u -p -r1.15 sys_futex.c
--- kern/sys_futex.c20 Mar 2020 17:17:31 - 1.15
+++ kern/sys_futex.c4 Apr 2020 14:48:40 -
@@ -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();
+ *retval = futex_wait(uaddr, val, timeout, flags);
+ rw_exit_write();
+ 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)
