public inbox for [email protected]
 help / color / mirror / Atom feed
* [PATCHSET v3 0/10] Add io_uring futex/futexv support
@ 2023-07-20 22:18 Jens Axboe
  2023-07-20 22:18 ` [PATCH 01/10] futex: Clarify FUTEX2 flags Jens Axboe
                   ` (9 more replies)
  0 siblings, 10 replies; 22+ messages in thread
From: Jens Axboe @ 2023-07-20 22:18 UTC (permalink / raw)
  To: io-uring, linux-kernel; +Cc: peterz, andres

Hi,

This patchset adds support for first futex wake and wait, and then
futexv.

For both wait/wake/waitv, we support the bitset variant, as the
"normal" variants can be easily implemented on top of that.

PI and requeue are not supported through io_uring, just the above
mentioned parts. This may change in the future, but in the spirit
of keeping this small (and based on what people have been asking for),
this is what we currently have.

When I did these patches, I forgot that Pavel had previously posted a
futex variant for io_uring. The major thing that had been holding me
back from people asking about futexes and io_uring, is that I wanted
to do this what I consider the right way - no usage of io-wq or thread
offload, an actually async implementation that is efficient to use
and don't rely on a blocking thread for futex wait/waitv. This is what
this patchset attempts to do, while being minimally invasive on the
futex side. I believe the diffstat reflects that.

As far as I can recall, the first request for futex support with
io_uring came from Andres Freund, working on postgres. His aio rework
of postgres was one of the early adopters of io_uring, and futex
support was a natural extension for that. This is relevant from both
a usability point of view, as well as for effiency and performance.
In Andres's words, for the former:

"Futex wait support in io_uring makes it a lot easier to avoid deadlocks
in concurrent programs that have their own buffer pool: Obviously pages in
the application buffer pool have to be locked during IO. If the initiator
of IO A needs to wait for a held lock B, the holder of lock B might wait
for the IO A to complete.  The ability to wait for a lock and IO
completions at the same time provides an efficient way to avoid such
deadlocks."

and in terms of effiency, even without unlocking the full potential yet,
Andres says:

"Futex wake support in io_uring is useful because it allows for more
efficient directed wakeups.  For some "locks" postgres has queues
implemented in userspace, with wakeup logic that cannot easily be
implemented with FUTEX_WAKE_BITSET on a single "futex word" (imagine
waiting for journal flushes to have completed up to a certain point). Thus
a "lock release" sometimes need to wake up many processes in a row.  A
quick-and-dirty conversion to doing these wakeups via io_uring lead to a
3% throughput increase, with 12% fewer context switches, albeit in a
fairly extreme workload."

Some basic io_uring futex support and test cases are available in the
liburing 'futex' branch:

https://git.kernel.dk/cgit/liburing/log/?h=futex

testing all of the variants. I originally wrote this code about a
month ago and Andres has been using it with postgres, and I'm not
aware of any bugs in it. That's not to say it's perfect, obviously,
and I welcome some feedback so we can move this forward and hash out
any potential issues.

 include/linux/io_uring_types.h |   3 +
 include/uapi/linux/futex.h     |  17 +-
 include/uapi/linux/io_uring.h  |   4 +
 io_uring/Makefile              |   4 +-
 io_uring/cancel.c              |   5 +
 io_uring/cancel.h              |   4 +
 io_uring/futex.c               | 364 +++++++++++++++++++++++++++++++++
 io_uring/futex.h               |  36 ++++
 io_uring/io_uring.c            |   5 +
 io_uring/opdef.c               |  35 +++-
 kernel/futex/futex.h           |  64 +++++-
 kernel/futex/requeue.c         |   3 +-
 kernel/futex/syscalls.c        |  42 ++--
 kernel/futex/waitwake.c        |  53 +++--
 14 files changed, 593 insertions(+), 46 deletions(-)

You can also find the code here:

https://git.kernel.dk/cgit/linux/log/?h=io_uring-futex

V3:
- Rebase on top of Peter's futex flag patches
- Move to using FUTEX2 flags

-- 
Jens Axboe



^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 01/10] futex: Clarify FUTEX2 flags
  2023-07-20 22:18 [PATCHSET v3 0/10] Add io_uring futex/futexv support Jens Axboe
@ 2023-07-20 22:18 ` Jens Axboe
  2023-07-20 22:18 ` [PATCH 02/10] futex: Extend the " Jens Axboe
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 22+ messages in thread
From: Jens Axboe @ 2023-07-20 22:18 UTC (permalink / raw)
  To: io-uring, linux-kernel; +Cc: peterz, andres, Jens Axboe

From: Peter Zijlstra <[email protected]>

sys_futex_waitv() is part of the futex2 series (the first and only so
far) of syscalls and has a flags field per futex (as opposed to flags
being encoded in the futex op).

This new flags field has a new namespace, which unfortunately isn't
super explicit. Notably it currently takes FUTEX_32 and
FUTEX_PRIVATE_FLAG.

Introduce the FUTEX2 namespace to clarify this

Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Signed-off-by: Jens Axboe <[email protected]>
---
 include/uapi/linux/futex.h | 16 +++++++++++++---
 kernel/futex/syscalls.c    |  7 +++----
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/include/uapi/linux/futex.h b/include/uapi/linux/futex.h
index 71a5df8d2689..0c5abb6aa8f8 100644
--- a/include/uapi/linux/futex.h
+++ b/include/uapi/linux/futex.h
@@ -44,10 +44,20 @@
 					 FUTEX_PRIVATE_FLAG)
 
 /*
- * Flags to specify the bit length of the futex word for futex2 syscalls.
- * Currently, only 32 is supported.
+ * Flags for futex2 syscalls.
  */
-#define FUTEX_32		2
+			/*	0x00 */
+			/*	0x01 */
+#define FUTEX2_32		0x02
+			/*	0x04 */
+			/*	0x08 */
+			/*	0x10 */
+			/*	0x20 */
+			/*	0x40 */
+#define FUTEX2_PRIVATE		FUTEX_PRIVATE_FLAG
+
+/* do not use */
+#define FUTEX_32		FUTEX2_32 /* historical accident :-( */
 
 /*
  * Max numbers of elements in a futex_waitv array
diff --git a/kernel/futex/syscalls.c b/kernel/futex/syscalls.c
index a8074079b09e..42b6c2fac7db 100644
--- a/kernel/futex/syscalls.c
+++ b/kernel/futex/syscalls.c
@@ -183,8 +183,7 @@ SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
 	return do_futex(uaddr, op, val, tp, uaddr2, (unsigned long)utime, val3);
 }
 
-/* Mask of available flags for each futex in futex_waitv list */
-#define FUTEXV_WAITER_MASK (FUTEX_32 | FUTEX_PRIVATE_FLAG)
+#define FUTEX2_MASK (FUTEX2_32 | FUTEX2_PRIVATE)
 
 /**
  * futex_parse_waitv - Parse a waitv array from userspace
@@ -205,10 +204,10 @@ static int futex_parse_waitv(struct futex_vector *futexv,
 		if (copy_from_user(&aux, &uwaitv[i], sizeof(aux)))
 			return -EFAULT;
 
-		if ((aux.flags & ~FUTEXV_WAITER_MASK) || aux.__reserved)
+		if ((aux.flags & ~FUTEX2_MASK) || aux.__reserved)
 			return -EINVAL;
 
-		if (!(aux.flags & FUTEX_32))
+		if (!(aux.flags & FUTEX2_32))
 			return -EINVAL;
 
 		futexv[i].w.flags = aux.flags;
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 02/10] futex: Extend the FUTEX2 flags
  2023-07-20 22:18 [PATCHSET v3 0/10] Add io_uring futex/futexv support Jens Axboe
  2023-07-20 22:18 ` [PATCH 01/10] futex: Clarify FUTEX2 flags Jens Axboe
@ 2023-07-20 22:18 ` Jens Axboe
  2023-07-20 22:18 ` [PATCH 03/10] futex: Flag conversion Jens Axboe
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 22+ messages in thread
From: Jens Axboe @ 2023-07-20 22:18 UTC (permalink / raw)
  To: io-uring, linux-kernel; +Cc: peterz, andres, Jens Axboe

From: Peter Zijlstra <[email protected]>

Add the definition for the missing but always intended extra sizes,
and add a NUMA flag for the planned numa extention.

Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Signed-off-by: Jens Axboe <[email protected]>
---
 include/uapi/linux/futex.h | 7 ++++---
 kernel/futex/syscalls.c    | 4 ++--
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/uapi/linux/futex.h b/include/uapi/linux/futex.h
index 0c5abb6aa8f8..0ed021acc1d9 100644
--- a/include/uapi/linux/futex.h
+++ b/include/uapi/linux/futex.h
@@ -46,10 +46,11 @@
 /*
  * Flags for futex2 syscalls.
  */
-			/*	0x00 */
-			/*	0x01 */
+#define FUTEX2_8		0x00
+#define FUTEX2_16		0x01
 #define FUTEX2_32		0x02
-			/*	0x04 */
+#define FUTEX2_64		0x03
+#define FUTEX2_NUMA		0x04
 			/*	0x08 */
 			/*	0x10 */
 			/*	0x20 */
diff --git a/kernel/futex/syscalls.c b/kernel/futex/syscalls.c
index 42b6c2fac7db..d5bb6dad22fe 100644
--- a/kernel/futex/syscalls.c
+++ b/kernel/futex/syscalls.c
@@ -183,7 +183,7 @@ SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
 	return do_futex(uaddr, op, val, tp, uaddr2, (unsigned long)utime, val3);
 }
 
-#define FUTEX2_MASK (FUTEX2_32 | FUTEX2_PRIVATE)
+#define FUTEX2_MASK (FUTEX2_64 | FUTEX2_PRIVATE)
 
 /**
  * futex_parse_waitv - Parse a waitv array from userspace
@@ -207,7 +207,7 @@ static int futex_parse_waitv(struct futex_vector *futexv,
 		if ((aux.flags & ~FUTEX2_MASK) || aux.__reserved)
 			return -EINVAL;
 
-		if (!(aux.flags & FUTEX2_32))
+		if ((aux.flags & FUTEX2_64) != FUTEX2_32)
 			return -EINVAL;
 
 		futexv[i].w.flags = aux.flags;
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 03/10] futex: Flag conversion
  2023-07-20 22:18 [PATCHSET v3 0/10] Add io_uring futex/futexv support Jens Axboe
  2023-07-20 22:18 ` [PATCH 01/10] futex: Clarify FUTEX2 flags Jens Axboe
  2023-07-20 22:18 ` [PATCH 02/10] futex: Extend the " Jens Axboe
@ 2023-07-20 22:18 ` Jens Axboe
  2023-07-20 22:18 ` [PATCH 04/10] futex: factor out the futex wake handling Jens Axboe
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 22+ messages in thread
From: Jens Axboe @ 2023-07-20 22:18 UTC (permalink / raw)
  To: io-uring, linux-kernel; +Cc: peterz, andres, Jens Axboe

From: Peter Zijlstra <[email protected]>

Futex has 3 sets of flags:

 - legacy futex op bits
 - futex2 flags
 - internal flags

Add a few helpers to convert from the API flags into the internal
flags.

Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Signed-off-by: Jens Axboe <[email protected]>
---
 kernel/futex/futex.h    | 48 ++++++++++++++++++++++++++++++++++++++---
 kernel/futex/syscalls.c | 21 +++++++++++-------
 kernel/futex/waitwake.c |  4 ++--
 3 files changed, 60 insertions(+), 13 deletions(-)

diff --git a/kernel/futex/futex.h b/kernel/futex/futex.h
index b5379c0e6d6d..54f4470b7db8 100644
--- a/kernel/futex/futex.h
+++ b/kernel/futex/futex.h
@@ -16,8 +16,15 @@
  * Futex flags used to encode options to functions and preserve them across
  * restarts.
  */
+#define FLAGS_SIZE_8		0x00
+#define FLAGS_SIZE_16		0x01
+#define FLAGS_SIZE_32		0x02
+#define FLAGS_SIZE_64		0x03
+
+#define FLAGS_SIZE_MASK		0x03
+
 #ifdef CONFIG_MMU
-# define FLAGS_SHARED		0x01
+# define FLAGS_SHARED		0x10
 #else
 /*
  * NOMMU does not have per process address space. Let the compiler optimize
@@ -25,8 +32,43 @@
  */
 # define FLAGS_SHARED		0x00
 #endif
-#define FLAGS_CLOCKRT		0x02
-#define FLAGS_HAS_TIMEOUT	0x04
+#define FLAGS_CLOCKRT		0x20
+#define FLAGS_HAS_TIMEOUT	0x40
+#define FLAGS_NUMA		0x80
+
+/* FUTEX_ to FLAGS_ */
+static inline unsigned int futex_to_flags(unsigned int op)
+{
+	unsigned int flags = FLAGS_SIZE_32;
+
+	if (!(op & FUTEX_PRIVATE_FLAG))
+		flags |= FLAGS_SHARED;
+
+	if (op & FUTEX_CLOCK_REALTIME)
+		flags |= FLAGS_CLOCKRT;
+
+	return flags;
+}
+
+/* FUTEX2_ to FLAGS_ */
+static inline unsigned int futex2_to_flags(unsigned int flags2)
+{
+	unsigned int flags = flags2 & FUTEX2_64;
+
+	if (!(flags2 & FUTEX2_PRIVATE))
+		flags |= FLAGS_SHARED;
+
+	if (flags2 & FUTEX2_NUMA)
+		flags |= FLAGS_NUMA;
+
+	return flags;
+}
+
+static inline unsigned int futex_size(unsigned int flags)
+{
+	unsigned int size = flags & FLAGS_SIZE_MASK;
+	return 1 << size; /* {0,1,2,3} -> {1,2,4,8} */
+}
 
 #ifdef CONFIG_FAIL_FUTEX
 extern bool should_fail_futex(bool fshared);
diff --git a/kernel/futex/syscalls.c b/kernel/futex/syscalls.c
index d5bb6dad22fe..7234538a490d 100644
--- a/kernel/futex/syscalls.c
+++ b/kernel/futex/syscalls.c
@@ -85,15 +85,12 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
 long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
 		u32 __user *uaddr2, u32 val2, u32 val3)
 {
+	unsigned int flags = futex_to_flags(op);
 	int cmd = op & FUTEX_CMD_MASK;
-	unsigned int flags = 0;
 
-	if (!(op & FUTEX_PRIVATE_FLAG))
-		flags |= FLAGS_SHARED;
-
-	if (op & FUTEX_CLOCK_REALTIME) {
-		flags |= FLAGS_CLOCKRT;
-		if (cmd != FUTEX_WAIT_BITSET && cmd != FUTEX_WAIT_REQUEUE_PI &&
+	if (flags & FLAGS_CLOCKRT) {
+		if (cmd != FUTEX_WAIT_BITSET &&
+		    cmd != FUTEX_WAIT_REQUEUE_PI &&
 		    cmd != FUTEX_LOCK_PI2)
 			return -ENOSYS;
 	}
@@ -201,6 +198,8 @@ static int futex_parse_waitv(struct futex_vector *futexv,
 	unsigned int i;
 
 	for (i = 0; i < nr_futexes; i++) {
+		unsigned int bits, flags;
+
 		if (copy_from_user(&aux, &uwaitv[i], sizeof(aux)))
 			return -EFAULT;
 
@@ -210,7 +209,13 @@ static int futex_parse_waitv(struct futex_vector *futexv,
 		if ((aux.flags & FUTEX2_64) != FUTEX2_32)
 			return -EINVAL;
 
-		futexv[i].w.flags = aux.flags;
+		flags = futex2_to_flags(aux.flags);
+		bits = 8 * futex_size(flags);
+
+		if (bits < 64 && aux.val >> bits)
+			return -EINVAL;
+
+		futexv[i].w.flags = flags;
 		futexv[i].w.val = aux.val;
 		futexv[i].w.uaddr = aux.uaddr;
 		futexv[i].q = futex_q_init;
diff --git a/kernel/futex/waitwake.c b/kernel/futex/waitwake.c
index ba01b9408203..fa9757766103 100644
--- a/kernel/futex/waitwake.c
+++ b/kernel/futex/waitwake.c
@@ -419,11 +419,11 @@ static int futex_wait_multiple_setup(struct futex_vector *vs, int count, int *wo
 	 */
 retry:
 	for (i = 0; i < count; i++) {
-		if ((vs[i].w.flags & FUTEX_PRIVATE_FLAG) && retry)
+		if (!(vs[i].w.flags & FLAGS_SHARED) && retry)
 			continue;
 
 		ret = get_futex_key(u64_to_user_ptr(vs[i].w.uaddr),
-				    !(vs[i].w.flags & FUTEX_PRIVATE_FLAG),
+				    vs[i].w.flags & FLAGS_SHARED,
 				    &vs[i].q.key, FUTEX_READ);
 
 		if (unlikely(ret))
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 04/10] futex: factor out the futex wake handling
  2023-07-20 22:18 [PATCHSET v3 0/10] Add io_uring futex/futexv support Jens Axboe
                   ` (2 preceding siblings ...)
  2023-07-20 22:18 ` [PATCH 03/10] futex: Flag conversion Jens Axboe
@ 2023-07-20 22:18 ` Jens Axboe
  2023-07-20 22:18 ` [PATCH 05/10] futex: abstract out a __futex_wake_mark() helper Jens Axboe
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 22+ messages in thread
From: Jens Axboe @ 2023-07-20 22:18 UTC (permalink / raw)
  To: io-uring, linux-kernel; +Cc: peterz, andres, Jens Axboe

In preparation for having another waker that isn't futex_wake_mark(),
add a wake handler in futex_q. No extra data is associated with the
handler outside of struct futex_q itself. futex_wake_mark() is defined as
the standard wakeup helper, now set through futex_q_init like other
defaults.

Normal sync futex waiting relies on wake_q holding tasks that should
be woken up. This is what futex_wake_mark() does, it'll unqueue the
futex and add the associated task to the wake queue. For async usage of
futex waiting, rather than having tasks sleeping on the futex, we'll
need to deal with a futex wake differently. For the planned io_uring
case, that means posting a completion event for the task in question.
Having a definable wake handler can help support that use case.

Signed-off-by: Jens Axboe <[email protected]>
---
 kernel/futex/futex.h    | 4 ++++
 kernel/futex/requeue.c  | 3 ++-
 kernel/futex/waitwake.c | 6 +++---
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/kernel/futex/futex.h b/kernel/futex/futex.h
index 54f4470b7db8..2b18eb889cce 100644
--- a/kernel/futex/futex.h
+++ b/kernel/futex/futex.h
@@ -111,6 +111,9 @@ struct futex_pi_state {
 	union futex_key key;
 } __randomize_layout;
 
+struct futex_q;
+typedef void (futex_wake_fn)(struct wake_q_head *wake_q, struct futex_q *q);
+
 /**
  * struct futex_q - The hashed futex queue entry, one per waiting task
  * @list:		priority-sorted list of tasks waiting on this futex
@@ -140,6 +143,7 @@ struct futex_q {
 
 	struct task_struct *task;
 	spinlock_t *lock_ptr;
+	futex_wake_fn *wake;
 	union futex_key key;
 	struct futex_pi_state *pi_state;
 	struct rt_mutex_waiter *rt_waiter;
diff --git a/kernel/futex/requeue.c b/kernel/futex/requeue.c
index cba8b1a6a4cc..e892bc6c41d8 100644
--- a/kernel/futex/requeue.c
+++ b/kernel/futex/requeue.c
@@ -58,6 +58,7 @@ enum {
 
 const struct futex_q futex_q_init = {
 	/* list gets initialized in futex_queue()*/
+	.wake		= futex_wake_mark,
 	.key		= FUTEX_KEY_INIT,
 	.bitset		= FUTEX_BITSET_MATCH_ANY,
 	.requeue_state	= ATOMIC_INIT(Q_REQUEUE_PI_NONE),
@@ -591,7 +592,7 @@ int futex_requeue(u32 __user *uaddr1, unsigned int flags, u32 __user *uaddr2,
 		/* Plain futexes just wake or requeue and are done */
 		if (!requeue_pi) {
 			if (++task_count <= nr_wake)
-				futex_wake_mark(&wake_q, this);
+				this->wake(&wake_q, this);
 			else
 				requeue_futex(this, hb1, hb2, &key2);
 			continue;
diff --git a/kernel/futex/waitwake.c b/kernel/futex/waitwake.c
index fa9757766103..0272b8c3b132 100644
--- a/kernel/futex/waitwake.c
+++ b/kernel/futex/waitwake.c
@@ -174,7 +174,7 @@ int futex_wake(u32 __user *uaddr, unsigned int flags, int nr_wake, u32 bitset)
 			if (!(this->bitset & bitset))
 				continue;
 
-			futex_wake_mark(&wake_q, this);
+			this->wake(&wake_q, this);
 			if (++ret >= nr_wake)
 				break;
 		}
@@ -289,7 +289,7 @@ int futex_wake_op(u32 __user *uaddr1, unsigned int flags, u32 __user *uaddr2,
 				ret = -EINVAL;
 				goto out_unlock;
 			}
-			futex_wake_mark(&wake_q, this);
+			this->wake(&wake_q, this);
 			if (++ret >= nr_wake)
 				break;
 		}
@@ -303,7 +303,7 @@ int futex_wake_op(u32 __user *uaddr1, unsigned int flags, u32 __user *uaddr2,
 					ret = -EINVAL;
 					goto out_unlock;
 				}
-				futex_wake_mark(&wake_q, this);
+				this->wake(&wake_q, this);
 				if (++op_ret >= nr_wake2)
 					break;
 			}
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 05/10] futex: abstract out a __futex_wake_mark() helper
  2023-07-20 22:18 [PATCHSET v3 0/10] Add io_uring futex/futexv support Jens Axboe
                   ` (3 preceding siblings ...)
  2023-07-20 22:18 ` [PATCH 04/10] futex: factor out the futex wake handling Jens Axboe
@ 2023-07-20 22:18 ` Jens Axboe
  2023-07-20 22:18 ` [PATCH 06/10] io_uring: add support for futex wake and wait Jens Axboe
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 22+ messages in thread
From: Jens Axboe @ 2023-07-20 22:18 UTC (permalink / raw)
  To: io-uring, linux-kernel; +Cc: peterz, andres, Jens Axboe

Move the unqueue and lock_ptr clear into a helper that futex_wake_mark()
calls. Add it to the public functions as well, in preparation for using
it outside the core futex code.

Suggested-by: Peter Zijlstra <[email protected]>
Signed-off-by: Jens Axboe <[email protected]>
---
 kernel/futex/futex.h    |  1 +
 kernel/futex/waitwake.c | 33 ++++++++++++++++++++++-----------
 2 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/kernel/futex/futex.h b/kernel/futex/futex.h
index 2b18eb889cce..98d43e9efe65 100644
--- a/kernel/futex/futex.h
+++ b/kernel/futex/futex.h
@@ -190,6 +190,7 @@ extern int futex_wait_setup(u32 __user *uaddr, u32 val, unsigned int flags,
 			    struct futex_q *q, struct futex_hash_bucket **hb);
 extern void futex_wait_queue(struct futex_hash_bucket *hb, struct futex_q *q,
 				   struct hrtimer_sleeper *timeout);
+extern bool __futex_wake_mark(struct futex_q *q);
 extern void futex_wake_mark(struct wake_q_head *wake_q, struct futex_q *q);
 
 extern int fault_in_user_writeable(u32 __user *uaddr);
diff --git a/kernel/futex/waitwake.c b/kernel/futex/waitwake.c
index 0272b8c3b132..86f67f652b95 100644
--- a/kernel/futex/waitwake.c
+++ b/kernel/futex/waitwake.c
@@ -106,20 +106,11 @@
  * double_lock_hb() and double_unlock_hb(), respectively.
  */
 
-/*
- * The hash bucket lock must be held when this is called.
- * Afterwards, the futex_q must not be accessed. Callers
- * must ensure to later call wake_up_q() for the actual
- * wakeups to occur.
- */
-void futex_wake_mark(struct wake_q_head *wake_q, struct futex_q *q)
+bool __futex_wake_mark(struct futex_q *q)
 {
-	struct task_struct *p = q->task;
-
 	if (WARN(q->pi_state || q->rt_waiter, "refusing to wake PI futex\n"))
-		return;
+		return false;
 
-	get_task_struct(p);
 	__futex_unqueue(q);
 	/*
 	 * The waiting task can free the futex_q as soon as q->lock_ptr = NULL
@@ -130,6 +121,26 @@ void futex_wake_mark(struct wake_q_head *wake_q, struct futex_q *q)
 	 */
 	smp_store_release(&q->lock_ptr, NULL);
 
+	return true;
+}
+
+/*
+ * The hash bucket lock must be held when this is called.
+ * Afterwards, the futex_q must not be accessed. Callers
+ * must ensure to later call wake_up_q() for the actual
+ * wakeups to occur.
+ */
+void futex_wake_mark(struct wake_q_head *wake_q, struct futex_q *q)
+{
+	struct task_struct *p = q->task;
+
+	get_task_struct(p);
+
+	if (!__futex_wake_mark(q)) {
+		put_task_struct(p);
+		return;
+	}
+
 	/*
 	 * Queue the task for later wakeup for after we've released
 	 * the hb->lock.
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 06/10] io_uring: add support for futex wake and wait
  2023-07-20 22:18 [PATCHSET v3 0/10] Add io_uring futex/futexv support Jens Axboe
                   ` (4 preceding siblings ...)
  2023-07-20 22:18 ` [PATCH 05/10] futex: abstract out a __futex_wake_mark() helper Jens Axboe
@ 2023-07-20 22:18 ` Jens Axboe
  2023-07-21 11:30   ` Peter Zijlstra
  2023-07-20 22:18 ` [PATCH 07/10] futex: add wake_data to struct futex_q Jens Axboe
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 22+ messages in thread
From: Jens Axboe @ 2023-07-20 22:18 UTC (permalink / raw)
  To: io-uring, linux-kernel; +Cc: peterz, andres, Jens Axboe

Add support for FUTEX_WAKE/WAIT primitives.

IORING_OP_FUTEX_WAKE is mix of FUTEX_WAKE and FUTEX_WAKE_BITSET, as
it does support passing in a bitset.

Similary, IORING_OP_FUTEX_WAIT is a mix of FUTEX_WAIT and
FUTEX_WAIT_BITSET.

FUTEX_WAKE is straight forward, as those can always be done directly from
the io_uring submission without needing async handling. For FUTEX_WAIT,
things are a bit more complicated. If the futex isn't ready, then we
rely on a callback via futex_queue->wake() when someone wakes up the
futex. From that calback, we queue up task_work with the original task,
which will post a CQE and wake it, if necessary.

Cancelations are supported, both from the application point-of-view,
but also to be able to cancel pending waits if the ring exits before
all events have occurred.

This is just the barebones wait/wake support. PI or REQUEUE support is
not added at this point, unclear if we might look into that later.

Likewise, explicit timeouts are not supported either. It is expected
that users that need timeouts would do so via the usual io_uring
mechanism to do that using linked timeouts.

Signed-off-by: Jens Axboe <[email protected]>
---
 include/linux/io_uring_types.h |   3 +
 include/uapi/linux/io_uring.h  |   3 +
 io_uring/Makefile              |   4 +-
 io_uring/cancel.c              |   5 +
 io_uring/cancel.h              |   4 +
 io_uring/futex.c               | 220 +++++++++++++++++++++++++++++++++
 io_uring/futex.h               |  34 +++++
 io_uring/io_uring.c            |   5 +
 io_uring/opdef.c               |  24 +++-
 9 files changed, 300 insertions(+), 2 deletions(-)
 create mode 100644 io_uring/futex.c
 create mode 100644 io_uring/futex.h

diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
index f04ce513fadb..a7f03d8d879f 100644
--- a/include/linux/io_uring_types.h
+++ b/include/linux/io_uring_types.h
@@ -273,6 +273,9 @@ struct io_ring_ctx {
 	struct io_wq_work_list	locked_free_list;
 	unsigned int		locked_free_nr;
 
+	struct hlist_head	futex_list;
+	struct io_alloc_cache	futex_cache;
+
 	const struct cred	*sq_creds;	/* cred used for __io_sq_thread() */
 	struct io_sq_data	*sq_data;	/* if using sq thread polling */
 
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index 36f9c73082de..3bd2d765f593 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -65,6 +65,7 @@ struct io_uring_sqe {
 		__u32		xattr_flags;
 		__u32		msg_ring_flags;
 		__u32		uring_cmd_flags;
+		__u32		futex_flags;
 	};
 	__u64	user_data;	/* data to be passed back at completion time */
 	/* pack this to avoid bogus arm OABI complaints */
@@ -235,6 +236,8 @@ enum io_uring_op {
 	IORING_OP_URING_CMD,
 	IORING_OP_SEND_ZC,
 	IORING_OP_SENDMSG_ZC,
+	IORING_OP_FUTEX_WAIT,
+	IORING_OP_FUTEX_WAKE,
 
 	/* this goes last, obviously */
 	IORING_OP_LAST,
diff --git a/io_uring/Makefile b/io_uring/Makefile
index 8cc8e5387a75..2e4779bc550c 100644
--- a/io_uring/Makefile
+++ b/io_uring/Makefile
@@ -7,5 +7,7 @@ obj-$(CONFIG_IO_URING)		+= io_uring.o xattr.o nop.o fs.o splice.o \
 					openclose.o uring_cmd.o epoll.o \
 					statx.o net.o msg_ring.o timeout.o \
 					sqpoll.o fdinfo.o tctx.o poll.o \
-					cancel.o kbuf.o rsrc.o rw.o opdef.o notif.o
+					cancel.o kbuf.o rsrc.o rw.o opdef.o \
+					notif.o
 obj-$(CONFIG_IO_WQ)		+= io-wq.o
+obj-$(CONFIG_FUTEX)		+= futex.o
diff --git a/io_uring/cancel.c b/io_uring/cancel.c
index 7b23607cf4af..3dba8ccb1cd8 100644
--- a/io_uring/cancel.c
+++ b/io_uring/cancel.c
@@ -15,6 +15,7 @@
 #include "tctx.h"
 #include "poll.h"
 #include "timeout.h"
+#include "futex.h"
 #include "cancel.h"
 
 struct io_cancel {
@@ -119,6 +120,10 @@ int io_try_cancel(struct io_uring_task *tctx, struct io_cancel_data *cd,
 	if (ret != -ENOENT)
 		return ret;
 
+	ret = io_futex_cancel(ctx, cd, issue_flags);
+	if (ret != -ENOENT)
+		return ret;
+
 	spin_lock(&ctx->completion_lock);
 	if (!(cd->flags & IORING_ASYNC_CANCEL_FD))
 		ret = io_timeout_cancel(ctx, cd);
diff --git a/io_uring/cancel.h b/io_uring/cancel.h
index fc98622e6166..c0a8e7c520b6 100644
--- a/io_uring/cancel.h
+++ b/io_uring/cancel.h
@@ -1,4 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
+#ifndef IORING_CANCEL_H
+#define IORING_CANCEL_H
 
 #include <linux/io_uring_types.h>
 
@@ -22,3 +24,5 @@ void init_hash_table(struct io_hash_table *table, unsigned size);
 
 int io_sync_cancel(struct io_ring_ctx *ctx, void __user *arg);
 bool io_cancel_req_match(struct io_kiocb *req, struct io_cancel_data *cd);
+
+#endif
diff --git a/io_uring/futex.c b/io_uring/futex.c
new file mode 100644
index 000000000000..0114fda797e1
--- /dev/null
+++ b/io_uring/futex.c
@@ -0,0 +1,220 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/fs.h>
+#include <linux/file.h>
+#include <linux/io_uring.h>
+
+#include <uapi/linux/io_uring.h>
+
+#include "../kernel/futex/futex.h"
+#include "io_uring.h"
+#include "rsrc.h"
+#include "futex.h"
+
+struct io_futex {
+	struct file	*file;
+	u32 __user	*uaddr;
+	unsigned int	futex_val;
+	unsigned int	futex_flags;
+	unsigned int	futex_mask;
+};
+
+struct io_futex_data {
+	union {
+		struct futex_q		q;
+		struct io_cache_entry	cache;
+	};
+	struct io_kiocb	*req;
+};
+
+void io_futex_cache_init(struct io_ring_ctx *ctx)
+{
+	io_alloc_cache_init(&ctx->futex_cache, IO_NODE_ALLOC_CACHE_MAX,
+				sizeof(struct io_futex_data));
+}
+
+static void io_futex_cache_entry_free(struct io_cache_entry *entry)
+{
+	kfree(container_of(entry, struct io_futex_data, cache));
+}
+
+void io_futex_cache_free(struct io_ring_ctx *ctx)
+{
+	io_alloc_cache_free(&ctx->futex_cache, io_futex_cache_entry_free);
+}
+
+static void io_futex_complete(struct io_kiocb *req, struct io_tw_state *ts)
+{
+	struct io_futex_data *ifd = req->async_data;
+	struct io_ring_ctx *ctx = req->ctx;
+
+	io_tw_lock(ctx, ts);
+	if (!io_alloc_cache_put(&ctx->futex_cache, &ifd->cache))
+		kfree(ifd);
+	req->async_data = NULL;
+	hlist_del_init(&req->hash_node);
+	io_req_task_complete(req, ts);
+}
+
+static bool __io_futex_cancel(struct io_ring_ctx *ctx, struct io_kiocb *req)
+{
+	struct io_futex_data *ifd = req->async_data;
+
+	/* futex wake already done or in progress */
+	if (!futex_unqueue(&ifd->q))
+		return false;
+
+	hlist_del_init(&req->hash_node);
+	io_req_set_res(req, -ECANCELED, 0);
+	req->io_task_work.func = io_futex_complete;
+	io_req_task_work_add(req);
+	return true;
+}
+
+int io_futex_cancel(struct io_ring_ctx *ctx, struct io_cancel_data *cd,
+		    unsigned int issue_flags)
+{
+	struct hlist_node *tmp;
+	struct io_kiocb *req;
+	int nr = 0;
+
+	if (cd->flags & (IORING_ASYNC_CANCEL_FD|IORING_ASYNC_CANCEL_FD_FIXED))
+		return -ENOENT;
+
+	io_ring_submit_lock(ctx, issue_flags);
+	hlist_for_each_entry_safe(req, tmp, &ctx->futex_list, hash_node) {
+		if (req->cqe.user_data != cd->data &&
+		    !(cd->flags & IORING_ASYNC_CANCEL_ANY))
+			continue;
+		if (__io_futex_cancel(ctx, req))
+			nr++;
+		if (!(cd->flags & IORING_ASYNC_CANCEL_ALL))
+			break;
+	}
+	io_ring_submit_unlock(ctx, issue_flags);
+
+	if (nr)
+		return nr;
+
+	return -ENOENT;
+}
+
+bool io_futex_remove_all(struct io_ring_ctx *ctx, struct task_struct *task,
+			 bool cancel_all)
+{
+	struct hlist_node *tmp;
+	struct io_kiocb *req;
+	bool found = false;
+
+	lockdep_assert_held(&ctx->uring_lock);
+
+	hlist_for_each_entry_safe(req, tmp, &ctx->futex_list, hash_node) {
+		if (!io_match_task_safe(req, task, cancel_all))
+			continue;
+		__io_futex_cancel(ctx, req);
+		found = true;
+	}
+
+	return found;
+}
+
+int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
+{
+	struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex);
+
+	if (unlikely(sqe->fd || sqe->addr2 || sqe->buf_index || sqe->addr3))
+		return -EINVAL;
+
+	iof->uaddr = u64_to_user_ptr(READ_ONCE(sqe->addr));
+	iof->futex_val = READ_ONCE(sqe->len);
+	iof->futex_mask = READ_ONCE(sqe->file_index);
+	iof->futex_flags = READ_ONCE(sqe->futex_flags);
+	if (iof->futex_flags & FUTEX_CMD_MASK)
+		return -EINVAL;
+
+	return 0;
+}
+
+static void io_futex_wake_fn(struct wake_q_head *wake_q, struct futex_q *q)
+{
+	struct io_futex_data *ifd = container_of(q, struct io_futex_data, q);
+	struct io_kiocb *req = ifd->req;
+
+	if (unlikely(!__futex_wake_mark(q)))
+		return;
+
+	io_req_set_res(req, 0, 0);
+	req->io_task_work.func = io_futex_complete;
+	io_req_task_work_add(req);
+}
+
+static struct io_futex_data *io_alloc_ifd(struct io_ring_ctx *ctx)
+{
+	struct io_cache_entry *entry;
+
+	entry = io_alloc_cache_get(&ctx->futex_cache);
+	if (entry)
+		return container_of(entry, struct io_futex_data, cache);
+
+	return kmalloc(sizeof(struct io_futex_data), GFP_NOWAIT);
+}
+
+int io_futex_wait(struct io_kiocb *req, unsigned int issue_flags)
+{
+	struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex);
+	struct io_ring_ctx *ctx = req->ctx;
+	struct io_futex_data *ifd = NULL;
+	struct futex_hash_bucket *hb;
+	int ret;
+
+	if (!iof->futex_mask) {
+		ret = -EINVAL;
+		goto done;
+	}
+
+	io_ring_submit_lock(ctx, issue_flags);
+	ifd = io_alloc_ifd(ctx);
+	if (!ifd) {
+		ret = -ENOMEM;
+		goto done_unlock;
+	}
+
+	req->async_data = ifd;
+	ifd->q = futex_q_init;
+	ifd->q.bitset = iof->futex_mask;
+	ifd->q.wake = io_futex_wake_fn;
+	ifd->req = req;
+
+	ret = futex_wait_setup(iof->uaddr, iof->futex_val,
+			       futex2_to_flags(iof->futex_flags), &ifd->q, &hb);
+	if (!ret) {
+		hlist_add_head(&req->hash_node, &ctx->futex_list);
+		io_ring_submit_unlock(ctx, issue_flags);
+
+		futex_queue(&ifd->q, hb);
+		return IOU_ISSUE_SKIP_COMPLETE;
+	}
+
+done_unlock:
+	io_ring_submit_unlock(ctx, issue_flags);
+done:
+	if (ret < 0)
+		req_set_fail(req);
+	io_req_set_res(req, ret, 0);
+	kfree(ifd);
+	return IOU_OK;
+}
+
+int io_futex_wake(struct io_kiocb *req, unsigned int issue_flags)
+{
+	struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex);
+	int ret;
+
+	ret = futex_wake(iof->uaddr, futex2_to_flags(iof->futex_flags),
+			 iof->futex_val, iof->futex_mask);
+	if (ret < 0)
+		req_set_fail(req);
+	io_req_set_res(req, ret, 0);
+	return IOU_OK;
+}
diff --git a/io_uring/futex.h b/io_uring/futex.h
new file mode 100644
index 000000000000..ddc9e0d73c52
--- /dev/null
+++ b/io_uring/futex.h
@@ -0,0 +1,34 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include "cancel.h"
+
+int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe);
+int io_futex_wait(struct io_kiocb *req, unsigned int issue_flags);
+int io_futex_wake(struct io_kiocb *req, unsigned int issue_flags);
+
+#if defined(CONFIG_FUTEX)
+int io_futex_cancel(struct io_ring_ctx *ctx, struct io_cancel_data *cd,
+		    unsigned int issue_flags);
+bool io_futex_remove_all(struct io_ring_ctx *ctx, struct task_struct *task,
+			 bool cancel_all);
+void io_futex_cache_init(struct io_ring_ctx *ctx);
+void io_futex_cache_free(struct io_ring_ctx *ctx);
+#else
+static inline int io_futex_cancel(struct io_ring_ctx *ctx,
+				  struct io_cancel_data *cd,
+				  unsigned int issue_flags)
+{
+	return 0;
+}
+static inline bool io_futex_remove_all(struct io_ring_ctx *ctx,
+				       struct task_struct *task, bool cancel_all)
+{
+	return false;
+}
+static inline void io_futex_cache_init(struct io_ring_ctx *ctx)
+{
+}
+static inline void io_futex_cache_free(struct io_ring_ctx *ctx)
+{
+}
+#endif
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 135da2fd0eda..e52cbdcb29b8 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -92,6 +92,7 @@
 #include "cancel.h"
 #include "net.h"
 #include "notif.h"
+#include "futex.h"
 
 #include "timeout.h"
 #include "poll.h"
@@ -330,6 +331,7 @@ static __cold struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
 			    sizeof(struct async_poll));
 	io_alloc_cache_init(&ctx->netmsg_cache, IO_ALLOC_CACHE_MAX,
 			    sizeof(struct io_async_msghdr));
+	io_futex_cache_init(ctx);
 	init_completion(&ctx->ref_comp);
 	xa_init_flags(&ctx->personalities, XA_FLAGS_ALLOC1);
 	mutex_init(&ctx->uring_lock);
@@ -349,6 +351,7 @@ static __cold struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
 	INIT_LIST_HEAD(&ctx->tctx_list);
 	ctx->submit_state.free_list.next = NULL;
 	INIT_WQ_LIST(&ctx->locked_free_list);
+	INIT_HLIST_HEAD(&ctx->futex_list);
 	INIT_DELAYED_WORK(&ctx->fallback_work, io_fallback_req_func);
 	INIT_WQ_LIST(&ctx->submit_state.compl_reqs);
 	return ctx;
@@ -2869,6 +2872,7 @@ static __cold void io_ring_ctx_free(struct io_ring_ctx *ctx)
 	io_eventfd_unregister(ctx);
 	io_alloc_cache_free(&ctx->apoll_cache, io_apoll_cache_free);
 	io_alloc_cache_free(&ctx->netmsg_cache, io_netmsg_cache_free);
+	io_futex_cache_free(ctx);
 	io_destroy_buffers(ctx);
 	mutex_unlock(&ctx->uring_lock);
 	if (ctx->sq_creds)
@@ -3281,6 +3285,7 @@ static __cold bool io_uring_try_cancel_requests(struct io_ring_ctx *ctx,
 	ret |= io_cancel_defer_files(ctx, task, cancel_all);
 	mutex_lock(&ctx->uring_lock);
 	ret |= io_poll_remove_all(ctx, task, cancel_all);
+	ret |= io_futex_remove_all(ctx, task, cancel_all);
 	mutex_unlock(&ctx->uring_lock);
 	ret |= io_kill_timeouts(ctx, task, cancel_all);
 	if (task)
diff --git a/io_uring/opdef.c b/io_uring/opdef.c
index 3b9c6489b8b6..c9f23c21a031 100644
--- a/io_uring/opdef.c
+++ b/io_uring/opdef.c
@@ -33,6 +33,7 @@
 #include "poll.h"
 #include "cancel.h"
 #include "rw.h"
+#include "futex.h"
 
 static int io_no_issue(struct io_kiocb *req, unsigned int issue_flags)
 {
@@ -426,11 +427,26 @@ const struct io_issue_def io_issue_defs[] = {
 		.issue			= io_sendmsg_zc,
 #else
 		.prep			= io_eopnotsupp_prep,
+#endif
+	},
+	[IORING_OP_FUTEX_WAIT] = {
+#if defined(CONFIG_FUTEX)
+		.prep			= io_futex_prep,
+		.issue			= io_futex_wait,
+#else
+		.prep			= io_eopnotsupp_prep,
+#endif
+	},
+	[IORING_OP_FUTEX_WAKE] = {
+#if defined(CONFIG_FUTEX)
+		.prep			= io_futex_prep,
+		.issue			= io_futex_wake,
+#else
+		.prep			= io_eopnotsupp_prep,
 #endif
 	},
 };
 
-
 const struct io_cold_def io_cold_defs[] = {
 	[IORING_OP_NOP] = {
 		.name			= "NOP",
@@ -648,6 +664,12 @@ const struct io_cold_def io_cold_defs[] = {
 		.fail			= io_sendrecv_fail,
 #endif
 	},
+	[IORING_OP_FUTEX_WAIT] = {
+		.name			= "FUTEX_WAIT",
+	},
+	[IORING_OP_FUTEX_WAKE] = {
+		.name			= "FUTEX_WAKE",
+	},
 };
 
 const char *io_uring_get_opcode(u8 opcode)
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 07/10] futex: add wake_data to struct futex_q
  2023-07-20 22:18 [PATCHSET v3 0/10] Add io_uring futex/futexv support Jens Axboe
                   ` (5 preceding siblings ...)
  2023-07-20 22:18 ` [PATCH 06/10] io_uring: add support for futex wake and wait Jens Axboe
@ 2023-07-20 22:18 ` Jens Axboe
  2023-07-20 22:18 ` [PATCH 08/10] futex: make futex_parse_waitv() available as a helper Jens Axboe
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 22+ messages in thread
From: Jens Axboe @ 2023-07-20 22:18 UTC (permalink / raw)
  To: io-uring, linux-kernel; +Cc: peterz, andres, Jens Axboe

With handling multiple futex_q for waitv, we cannot easily go from the
futex_q to data related to that request or queue. Add a wake_data
argument that belongs to the wake handler assigned.

Signed-off-by: Jens Axboe <[email protected]>
---
 kernel/futex/futex.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/futex/futex.h b/kernel/futex/futex.h
index 98d43e9efe65..292224816814 100644
--- a/kernel/futex/futex.h
+++ b/kernel/futex/futex.h
@@ -144,6 +144,7 @@ struct futex_q {
 	struct task_struct *task;
 	spinlock_t *lock_ptr;
 	futex_wake_fn *wake;
+	void *wake_data;
 	union futex_key key;
 	struct futex_pi_state *pi_state;
 	struct rt_mutex_waiter *rt_waiter;
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 08/10] futex: make futex_parse_waitv() available as a helper
  2023-07-20 22:18 [PATCHSET v3 0/10] Add io_uring futex/futexv support Jens Axboe
                   ` (6 preceding siblings ...)
  2023-07-20 22:18 ` [PATCH 07/10] futex: add wake_data to struct futex_q Jens Axboe
@ 2023-07-20 22:18 ` Jens Axboe
  2023-07-20 22:18 ` [PATCH 09/10] futex: make the vectored futex operations available Jens Axboe
  2023-07-20 22:18 ` [PATCH 10/10] io_uring: add support for vectored futex waits Jens Axboe
  9 siblings, 0 replies; 22+ messages in thread
From: Jens Axboe @ 2023-07-20 22:18 UTC (permalink / raw)
  To: io-uring, linux-kernel; +Cc: peterz, andres, Jens Axboe

To make it more generically useful, augment it with allowing the caller
to pass in the wake handler and wake data. Convert the futex_waitv()
syscall, passing in the default handlers.

Signed-off-by: Jens Axboe <[email protected]>
---
 kernel/futex/futex.h    |  5 +++++
 kernel/futex/syscalls.c | 14 ++++++++++----
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/kernel/futex/futex.h b/kernel/futex/futex.h
index 292224816814..3536d21adff9 100644
--- a/kernel/futex/futex.h
+++ b/kernel/futex/futex.h
@@ -327,6 +327,11 @@ struct futex_vector {
 	struct futex_q q;
 };
 
+extern int futex_parse_waitv(struct futex_vector *futexv,
+			     struct futex_waitv __user *uwaitv,
+			     unsigned int nr_futexes, futex_wake_fn *wake,
+			     void *wake_data);
+
 extern int futex_wait_multiple(struct futex_vector *vs, unsigned int count,
 			       struct hrtimer_sleeper *to);
 
diff --git a/kernel/futex/syscalls.c b/kernel/futex/syscalls.c
index 7234538a490d..a182ea44ea89 100644
--- a/kernel/futex/syscalls.c
+++ b/kernel/futex/syscalls.c
@@ -187,12 +187,15 @@ SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
  * @futexv:	Kernel side list of waiters to be filled
  * @uwaitv:     Userspace list to be parsed
  * @nr_futexes: Length of futexv
+ * @wake:	Wake to call when futex is woken
+ * @wake_data:	Data for the wake handler
  *
  * Return: Error code on failure, 0 on success
  */
-static int futex_parse_waitv(struct futex_vector *futexv,
-			     struct futex_waitv __user *uwaitv,
-			     unsigned int nr_futexes)
+int futex_parse_waitv(struct futex_vector *futexv,
+		      struct futex_waitv __user *uwaitv,
+		      unsigned int nr_futexes, futex_wake_fn *wake,
+		      void *wake_data)
 {
 	struct futex_waitv aux;
 	unsigned int i;
@@ -219,6 +222,8 @@ static int futex_parse_waitv(struct futex_vector *futexv,
 		futexv[i].w.val = aux.val;
 		futexv[i].w.uaddr = aux.uaddr;
 		futexv[i].q = futex_q_init;
+		futexv[i].q.wake = wake;
+		futexv[i].q.wake_data = wake_data;
 	}
 
 	return 0;
@@ -295,7 +300,8 @@ SYSCALL_DEFINE5(futex_waitv, struct futex_waitv __user *, waiters,
 		goto destroy_timer;
 	}
 
-	ret = futex_parse_waitv(futexv, waiters, nr_futexes);
+	ret = futex_parse_waitv(futexv, waiters, nr_futexes, futex_wake_mark,
+				NULL);
 	if (!ret)
 		ret = futex_wait_multiple(futexv, nr_futexes, timeout ? &to : NULL);
 
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 09/10] futex: make the vectored futex operations available
  2023-07-20 22:18 [PATCHSET v3 0/10] Add io_uring futex/futexv support Jens Axboe
                   ` (7 preceding siblings ...)
  2023-07-20 22:18 ` [PATCH 08/10] futex: make futex_parse_waitv() available as a helper Jens Axboe
@ 2023-07-20 22:18 ` Jens Axboe
  2023-07-20 22:18 ` [PATCH 10/10] io_uring: add support for vectored futex waits Jens Axboe
  9 siblings, 0 replies; 22+ messages in thread
From: Jens Axboe @ 2023-07-20 22:18 UTC (permalink / raw)
  To: io-uring, linux-kernel; +Cc: peterz, andres, Jens Axboe

Rename unqueue_multiple() as futex_unqueue_multiple(), and make both
that and futex_wait_multiple_setup() available for external users. This
is in preparation for wiring up vectored waits in io_uring.

Signed-off-by: Jens Axboe <[email protected]>
---
 kernel/futex/futex.h    |  5 +++++
 kernel/futex/waitwake.c | 10 +++++-----
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/kernel/futex/futex.h b/kernel/futex/futex.h
index 3536d21adff9..faf84915acb2 100644
--- a/kernel/futex/futex.h
+++ b/kernel/futex/futex.h
@@ -332,6 +332,11 @@ extern int futex_parse_waitv(struct futex_vector *futexv,
 			     unsigned int nr_futexes, futex_wake_fn *wake,
 			     void *wake_data);
 
+extern int futex_wait_multiple_setup(struct futex_vector *vs, int count,
+				     int *woken);
+
+extern int futex_unqueue_multiple(struct futex_vector *v, int count);
+
 extern int futex_wait_multiple(struct futex_vector *vs, unsigned int count,
 			       struct hrtimer_sleeper *to);
 
diff --git a/kernel/futex/waitwake.c b/kernel/futex/waitwake.c
index 86f67f652b95..6c8fb7300558 100644
--- a/kernel/futex/waitwake.c
+++ b/kernel/futex/waitwake.c
@@ -369,7 +369,7 @@ void futex_wait_queue(struct futex_hash_bucket *hb, struct futex_q *q,
 }
 
 /**
- * unqueue_multiple - Remove various futexes from their hash bucket
+ * futex_unqueue_multiple - Remove various futexes from their hash bucket
  * @v:	   The list of futexes to unqueue
  * @count: Number of futexes in the list
  *
@@ -379,7 +379,7 @@ void futex_wait_queue(struct futex_hash_bucket *hb, struct futex_q *q,
  *  - >=0 - Index of the last futex that was awoken;
  *  - -1  - No futex was awoken
  */
-static int unqueue_multiple(struct futex_vector *v, int count)
+int futex_unqueue_multiple(struct futex_vector *v, int count)
 {
 	int ret = -1, i;
 
@@ -407,7 +407,7 @@ static int unqueue_multiple(struct futex_vector *v, int count)
  *  -  0 - Success
  *  - <0 - -EFAULT, -EWOULDBLOCK or -EINVAL
  */
-static int futex_wait_multiple_setup(struct futex_vector *vs, int count, int *woken)
+int futex_wait_multiple_setup(struct futex_vector *vs, int count, int *woken)
 {
 	struct futex_hash_bucket *hb;
 	bool retry = false;
@@ -469,7 +469,7 @@ static int futex_wait_multiple_setup(struct futex_vector *vs, int count, int *wo
 		 * was woken, we don't return error and return this index to
 		 * userspace
 		 */
-		*woken = unqueue_multiple(vs, i);
+		*woken = futex_unqueue_multiple(vs, i);
 		if (*woken >= 0)
 			return 1;
 
@@ -554,7 +554,7 @@ int futex_wait_multiple(struct futex_vector *vs, unsigned int count,
 
 		__set_current_state(TASK_RUNNING);
 
-		ret = unqueue_multiple(vs, count);
+		ret = futex_unqueue_multiple(vs, count);
 		if (ret >= 0)
 			return ret;
 
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 10/10] io_uring: add support for vectored futex waits
  2023-07-20 22:18 [PATCHSET v3 0/10] Add io_uring futex/futexv support Jens Axboe
                   ` (8 preceding siblings ...)
  2023-07-20 22:18 ` [PATCH 09/10] futex: make the vectored futex operations available Jens Axboe
@ 2023-07-20 22:18 ` Jens Axboe
  9 siblings, 0 replies; 22+ messages in thread
From: Jens Axboe @ 2023-07-20 22:18 UTC (permalink / raw)
  To: io-uring, linux-kernel; +Cc: peterz, andres, Jens Axboe

This adds support for IORING_OP_FUTEX_WAITV, which allows registering a
notification for a number of futexes at once. If one of the futexes are
woken, then the request will complete with the index of the futex that got
woken as the result. This is identical to what the normal vectored futex
waitv operation does.

Use like IORING_OP_FUTEX_WAIT, except sqe->addr must now contain the a
pointer to a struct futex_waitv array, and sqe->off must now contain the
number of elements in that array.

Waiting on N futexes could be done with IORING_OP_FUTEX_WAIT as well,
but that punts a lot of the work to the application:

1) Application would need to submit N IORING_OP_FUTEX_WAIT requests,
   rather than just a single IORING_OP_FUTEX_WAITV.

2) When one futex is woken, application would need to cancel the
   remaining N-1 requests that didn't trigger.

While this is of course doable, having a single vectored futex wait
makes for much simpler application code.

Signed-off-by: Jens Axboe <[email protected]>
---
 include/uapi/linux/io_uring.h |   1 +
 io_uring/futex.c              | 164 +++++++++++++++++++++++++++++++---
 io_uring/futex.h              |   2 +
 io_uring/opdef.c              |  11 +++
 4 files changed, 168 insertions(+), 10 deletions(-)

diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index 3bd2d765f593..420f38675769 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -238,6 +238,7 @@ enum io_uring_op {
 	IORING_OP_SENDMSG_ZC,
 	IORING_OP_FUTEX_WAIT,
 	IORING_OP_FUTEX_WAKE,
+	IORING_OP_FUTEX_WAITV,
 
 	/* this goes last, obviously */
 	IORING_OP_LAST,
diff --git a/io_uring/futex.c b/io_uring/futex.c
index 0114fda797e1..93df54dffaa0 100644
--- a/io_uring/futex.c
+++ b/io_uring/futex.c
@@ -14,10 +14,15 @@
 
 struct io_futex {
 	struct file	*file;
-	u32 __user	*uaddr;
+	union {
+		u32 __user			*uaddr;
+		struct futex_waitv __user	*uwaitv;
+	};
 	unsigned int	futex_val;
 	unsigned int	futex_flags;
 	unsigned int	futex_mask;
+	unsigned int	futex_nr;
+	unsigned long	futexv_owned;
 };
 
 struct io_futex_data {
@@ -44,6 +49,13 @@ void io_futex_cache_free(struct io_ring_ctx *ctx)
 	io_alloc_cache_free(&ctx->futex_cache, io_futex_cache_entry_free);
 }
 
+static void __io_futex_complete(struct io_kiocb *req, struct io_tw_state *ts)
+{
+	req->async_data = NULL;
+	hlist_del_init(&req->hash_node);
+	io_req_task_complete(req, ts);
+}
+
 static void io_futex_complete(struct io_kiocb *req, struct io_tw_state *ts)
 {
 	struct io_futex_data *ifd = req->async_data;
@@ -52,22 +64,59 @@ static void io_futex_complete(struct io_kiocb *req, struct io_tw_state *ts)
 	io_tw_lock(ctx, ts);
 	if (!io_alloc_cache_put(&ctx->futex_cache, &ifd->cache))
 		kfree(ifd);
-	req->async_data = NULL;
-	hlist_del_init(&req->hash_node);
-	io_req_task_complete(req, ts);
+	__io_futex_complete(req, ts);
 }
 
-static bool __io_futex_cancel(struct io_ring_ctx *ctx, struct io_kiocb *req)
+static void io_futexv_complete(struct io_kiocb *req, struct io_tw_state *ts)
 {
-	struct io_futex_data *ifd = req->async_data;
+	struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex);
+	struct futex_vector *futexv = req->async_data;
+	struct io_ring_ctx *ctx = req->ctx;
+	int res = 0;
 
-	/* futex wake already done or in progress */
-	if (!futex_unqueue(&ifd->q))
+	io_tw_lock(ctx, ts);
+
+	res = futex_unqueue_multiple(futexv, iof->futex_nr);
+	if (res != -1)
+		io_req_set_res(req, res, 0);
+
+	kfree(req->async_data);
+	req->flags &= ~REQ_F_ASYNC_DATA;
+	__io_futex_complete(req, ts);
+}
+
+static bool io_futexv_claimed(struct io_futex *iof)
+{
+	return test_bit(0, &iof->futexv_owned);
+}
+
+static bool io_futexv_claim(struct io_futex *iof)
+{
+	if (test_bit(0, &iof->futexv_owned) ||
+	    test_and_set_bit(0, &iof->futexv_owned))
 		return false;
+	return true;
+}
+
+static bool __io_futex_cancel(struct io_ring_ctx *ctx, struct io_kiocb *req)
+{
+	/* futex wake already done or in progress */
+	if (req->opcode == IORING_OP_FUTEX_WAIT) {
+		struct io_futex_data *ifd = req->async_data;
+
+		if (!futex_unqueue(&ifd->q))
+			return false;
+		req->io_task_work.func = io_futex_complete;
+	} else {
+		struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex);
+
+		if (!io_futexv_claim(iof))
+			return false;
+		req->io_task_work.func = io_futexv_complete;
+	}
 
 	hlist_del_init(&req->hash_node);
 	io_req_set_res(req, -ECANCELED, 0);
-	req->io_task_work.func = io_futex_complete;
 	io_req_task_work_add(req);
 	return true;
 }
@@ -123,7 +172,7 @@ int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 {
 	struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex);
 
-	if (unlikely(sqe->fd || sqe->addr2 || sqe->buf_index || sqe->addr3))
+	if (unlikely(sqe->fd || sqe->buf_index || sqe->addr3))
 		return -EINVAL;
 
 	iof->uaddr = u64_to_user_ptr(READ_ONCE(sqe->addr));
@@ -133,6 +182,52 @@ int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 	if (iof->futex_flags & FUTEX_CMD_MASK)
 		return -EINVAL;
 
+	iof->futexv_owned = 0;
+	return 0;
+}
+
+static void io_futex_wakev_fn(struct wake_q_head *wake_q, struct futex_q *q)
+{
+	struct io_kiocb *req = q->wake_data;
+	struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex);
+
+	if (!io_futexv_claim(iof))
+		return;
+	if (unlikely(!__futex_wake_mark(q)))
+		return;
+
+	io_req_set_res(req, 0, 0);
+	req->io_task_work.func = io_futexv_complete;
+	io_req_task_work_add(req);
+}
+
+int io_futexv_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
+{
+	struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex);
+	struct futex_vector *futexv;
+	int ret;
+
+	ret = io_futex_prep(req, sqe);
+	if (ret)
+		return ret;
+
+	iof->futex_nr = READ_ONCE(sqe->off);
+	if (!iof->futex_nr || iof->futex_nr > FUTEX_WAITV_MAX)
+		return -EINVAL;
+
+	futexv = kcalloc(iof->futex_nr, sizeof(*futexv), GFP_KERNEL);
+	if (!futexv)
+		return -ENOMEM;
+
+	ret = futex_parse_waitv(futexv, iof->uwaitv, iof->futex_nr,
+				io_futex_wakev_fn, req);
+	if (ret) {
+		kfree(futexv);
+		return ret;
+	}
+
+	req->flags |= REQ_F_ASYNC_DATA;
+	req->async_data = futexv;
 	return 0;
 }
 
@@ -160,6 +255,55 @@ static struct io_futex_data *io_alloc_ifd(struct io_ring_ctx *ctx)
 	return kmalloc(sizeof(struct io_futex_data), GFP_NOWAIT);
 }
 
+int io_futexv_wait(struct io_kiocb *req, unsigned int issue_flags)
+{
+	struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex);
+	struct futex_vector *futexv = req->async_data;
+	struct io_ring_ctx *ctx = req->ctx;
+	int ret, woken = -1;
+
+	io_ring_submit_lock(ctx, issue_flags);
+
+	ret = futex_wait_multiple_setup(futexv, iof->futex_nr, &woken);
+
+	/*
+	 * The above call leaves us potentially non-running. This is fine
+	 * for the sync syscall as it'll be blocking unless we already got
+	 * one of the futexes woken, but it obviously won't work for an async
+	 * invocation. Mark is runnable again.
+	 */
+	__set_current_state(TASK_RUNNING);
+
+	/*
+	 * We got woken while setting up, let that side do the completion
+	 */
+	if (io_futexv_claimed(iof)) {
+skip:
+		io_ring_submit_unlock(ctx, issue_flags);
+		return IOU_ISSUE_SKIP_COMPLETE;
+	}
+
+	/*
+	 * 0 return means that we successfully setup the waiters, and that
+	 * nobody triggered a wakeup while we were doing so. < 0 or 1 return
+	 * is either an error or we got a wakeup while setting up.
+	 */
+	if (!ret) {
+		hlist_add_head(&req->hash_node, &ctx->futex_list);
+		goto skip;
+	}
+
+	io_ring_submit_unlock(ctx, issue_flags);
+	if (ret < 0)
+		req_set_fail(req);
+	else if (woken != -1)
+		ret = woken;
+	io_req_set_res(req, ret, 0);
+	kfree(futexv);
+	req->flags &= ~REQ_F_ASYNC_DATA;
+	return IOU_OK;
+}
+
 int io_futex_wait(struct io_kiocb *req, unsigned int issue_flags)
 {
 	struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex);
diff --git a/io_uring/futex.h b/io_uring/futex.h
index ddc9e0d73c52..0847e9e8a127 100644
--- a/io_uring/futex.h
+++ b/io_uring/futex.h
@@ -3,7 +3,9 @@
 #include "cancel.h"
 
 int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe);
+int io_futexv_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe);
 int io_futex_wait(struct io_kiocb *req, unsigned int issue_flags);
+int io_futexv_wait(struct io_kiocb *req, unsigned int issue_flags);
 int io_futex_wake(struct io_kiocb *req, unsigned int issue_flags);
 
 #if defined(CONFIG_FUTEX)
diff --git a/io_uring/opdef.c b/io_uring/opdef.c
index c9f23c21a031..b9e1e12cac9c 100644
--- a/io_uring/opdef.c
+++ b/io_uring/opdef.c
@@ -443,6 +443,14 @@ const struct io_issue_def io_issue_defs[] = {
 		.issue			= io_futex_wake,
 #else
 		.prep			= io_eopnotsupp_prep,
+#endif
+	},
+	[IORING_OP_FUTEX_WAITV] = {
+#if defined(CONFIG_FUTEX)
+		.prep			= io_futexv_prep,
+		.issue			= io_futexv_wait,
+#else
+		.prep			= io_eopnotsupp_prep,
 #endif
 	},
 };
@@ -670,6 +678,9 @@ const struct io_cold_def io_cold_defs[] = {
 	[IORING_OP_FUTEX_WAKE] = {
 		.name			= "FUTEX_WAKE",
 	},
+	[IORING_OP_FUTEX_WAITV] = {
+		.name			= "FUTEX_WAITV",
+	},
 };
 
 const char *io_uring_get_opcode(u8 opcode)
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH 06/10] io_uring: add support for futex wake and wait
  2023-07-20 22:18 ` [PATCH 06/10] io_uring: add support for futex wake and wait Jens Axboe
@ 2023-07-21 11:30   ` Peter Zijlstra
  2023-07-21 11:37     ` Peter Zijlstra
  0 siblings, 1 reply; 22+ messages in thread
From: Peter Zijlstra @ 2023-07-21 11:30 UTC (permalink / raw)
  To: Jens Axboe; +Cc: io-uring, linux-kernel, andres

On Thu, Jul 20, 2023 at 04:18:54PM -0600, Jens Axboe wrote:


> +struct io_futex {
> +	struct file	*file;
> +	u32 __user	*uaddr;
> +	unsigned int	futex_val;
> +	unsigned int	futex_flags;
> +	unsigned int	futex_mask;
> +};

So in the futex patches I just posted I went with 'unsigned long'
(syscall) or 'u64' (data structures) for the futex, such that, on 64bit
platforms, we might support 64bit futexes in the future (I still need to
audit the whole futex internals and convert u32 to unsigned long in
order to enable that).

So would something like:

struct io_futex {
	struct file	*file;
	void __user	*uaddr;
	u64		futex_val;
	u64		futex_mask;
	u32		futex_flags;
};

work to match the futex2 syscalls?



> +int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
> +{
> +	struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex);
> +
> +	if (unlikely(sqe->fd || sqe->addr2 || sqe->buf_index || sqe->addr3))
> +		return -EINVAL;
> +
> +	iof->uaddr = u64_to_user_ptr(READ_ONCE(sqe->addr));
> +	iof->futex_val = READ_ONCE(sqe->len);
> +	iof->futex_mask = READ_ONCE(sqe->file_index);
> +	iof->futex_flags = READ_ONCE(sqe->futex_flags);

sqe->addr,		u64
sqe->len,		u32
sqe->file_index,	u32
sqe->futex_flags,	u32

> +	if (iof->futex_flags & FUTEX_CMD_MASK)

		FUTEX2_MASK

(which would need lifting from syscall.c to kernel/futex/futex.h I
suppose)

> +		return -EINVAL;
> +
> +	return 0;
> +}

> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
> index 36f9c73082de..3bd2d765f593 100644
> --- a/include/uapi/linux/io_uring.h
> +++ b/include/uapi/linux/io_uring.h
> @@ -65,6 +65,7 @@ struct io_uring_sqe {
>  		__u32		xattr_flags;
>  		__u32		msg_ring_flags;
>  		__u32		uring_cmd_flags;
> +		__u32		futex_flags;
>  	};
>  	__u64	user_data;	/* data to be passed back at completion time */
>  	/* pack this to avoid bogus arm OABI complaints */

Perhaps extend it like so?


diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index 08720c7bd92f..c1d28bf64d11 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -35,6 +35,7 @@ struct io_uring_sqe {
 	union {
 		__u64	off;	/* offset into file */
 		__u64	addr2;
+		__u64	futex_val;
 		struct {
 			__u32	cmd_op;
 			__u32	__pad1;
@@ -65,6 +66,7 @@ struct io_uring_sqe {
 		__u32		xattr_flags;
 		__u32		msg_ring_flags;
 		__u32		uring_cmd_flags;
+		__u32		futex_flags;
 	};
 	__u64	user_data;	/* data to be passed back at completion time */
 	/* pack this to avoid bogus arm OABI complaints */
@@ -87,6 +89,7 @@ struct io_uring_sqe {
 	union {
 		struct {
 			__u64	addr3;
+			__u64	futex_mask;
 			__u64	__pad2[1];
 		};
 		/*


So that we can write something roughtly like:

	iof->uaddr = sqe->addr;
	iof->val   = sqe->futex_val;
	iof->mask  = sqe->futex_mask;
	iof->flags = sqe->futex_flags;

	if (iof->flags & ~FUTEX2_MASK)
		return -EINVAL;



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH 06/10] io_uring: add support for futex wake and wait
  2023-07-21 11:30   ` Peter Zijlstra
@ 2023-07-21 11:37     ` Peter Zijlstra
  2023-07-21 14:43       ` Jens Axboe
  0 siblings, 1 reply; 22+ messages in thread
From: Peter Zijlstra @ 2023-07-21 11:37 UTC (permalink / raw)
  To: Jens Axboe; +Cc: io-uring, linux-kernel, andres

On Fri, Jul 21, 2023 at 01:30:31PM +0200, Peter Zijlstra wrote:

Sorry, I was too quick..

	iof->uaddr = sqe->addr;
	iof->val   = sqe->futex_val;
	iof->mask  = sqe->futex_mask;
	flags      = sqe->futex_flags;

	if (flags & ~FUTEX2_MASK)
		return -EINVAL;

	iof->flags = futex2_to_flags(flags);
	if (!futex_flags_valid(iof->flags))
		return -EINVAL;

	if (!futex_validate_input(iof->flags, iof->val) ||
	    !futex_validate_input(iof->flags, iof->mask))
		return -EINVAL



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 06/10] io_uring: add support for futex wake and wait
  2023-07-21 11:37     ` Peter Zijlstra
@ 2023-07-21 14:43       ` Jens Axboe
  2023-07-21 15:29         ` Jens Axboe
  0 siblings, 1 reply; 22+ messages in thread
From: Jens Axboe @ 2023-07-21 14:43 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: io-uring, linux-kernel, andres

On 7/21/23 5:37?AM, Peter Zijlstra wrote:
> On Fri, Jul 21, 2023 at 01:30:31PM +0200, Peter Zijlstra wrote:
> 
> Sorry, I was too quick..
> 
> 	iof->uaddr = sqe->addr;
> 	iof->val   = sqe->futex_val;
> 	iof->mask  = sqe->futex_mask;
> 	flags      = sqe->futex_flags;
> 
> 	if (flags & ~FUTEX2_MASK)
> 		return -EINVAL;
> 
> 	iof->flags = futex2_to_flags(flags);
> 	if (!futex_flags_valid(iof->flags))
> 		return -EINVAL;
> 
> 	if (!futex_validate_input(iof->flags, iof->val) ||
> 	    !futex_validate_input(iof->flags, iof->mask))
> 		return -EINVAL

Something like that should work, with some variable names fixed up. I
just went with 'addr' for the futex address, addr2 for the value, and
addr3 for the mask.

Rebased on top of your first 4 updated patches, and added a single patch
that moves FUTEX2_MASK, will run some testing to validate it's all still
sane.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 06/10] io_uring: add support for futex wake and wait
  2023-07-21 14:43       ` Jens Axboe
@ 2023-07-21 15:29         ` Jens Axboe
  2023-07-25 13:00           ` Peter Zijlstra
  0 siblings, 1 reply; 22+ messages in thread
From: Jens Axboe @ 2023-07-21 15:29 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: io-uring, linux-kernel, andres

On 7/21/23 8:43?AM, Jens Axboe wrote:
> On 7/21/23 5:37?AM, Peter Zijlstra wrote:
>> On Fri, Jul 21, 2023 at 01:30:31PM +0200, Peter Zijlstra wrote:
>>
>> Sorry, I was too quick..
>>
>> 	iof->uaddr = sqe->addr;
>> 	iof->val   = sqe->futex_val;
>> 	iof->mask  = sqe->futex_mask;
>> 	flags      = sqe->futex_flags;
>>
>> 	if (flags & ~FUTEX2_MASK)
>> 		return -EINVAL;
>>
>> 	iof->flags = futex2_to_flags(flags);
>> 	if (!futex_flags_valid(iof->flags))
>> 		return -EINVAL;
>>
>> 	if (!futex_validate_input(iof->flags, iof->val) ||
>> 	    !futex_validate_input(iof->flags, iof->mask))
>> 		return -EINVAL
> 
> Something like that should work, with some variable names fixed up. I
> just went with 'addr' for the futex address, addr2 for the value, and
> addr3 for the mask.
> 
> Rebased on top of your first 4 updated patches, and added a single patch
> that moves FUTEX2_MASK, will run some testing to validate it's all still
> sane.

FWIW, here's the io_uring incremental after that rebase. Update the
liburing futex branch as well, updating the prep helpers to take 64 bit
values for mask/val and also add the flags argument that was missing as
well. Only other addition was adding those 4 new patches instead of the
old 3 ones, and adding single patch that just moves FUTEX2_MASK to
futex.h.

All checks out fine, tests pass and it works.


diff --git a/io_uring/futex.c b/io_uring/futex.c
index 93df54dffaa0..4c9f2c841b98 100644
--- a/io_uring/futex.c
+++ b/io_uring/futex.c
@@ -18,11 +18,11 @@ struct io_futex {
 		u32 __user			*uaddr;
 		struct futex_waitv __user	*uwaitv;
 	};
-	unsigned int	futex_val;
-	unsigned int	futex_flags;
-	unsigned int	futex_mask;
-	unsigned int	futex_nr;
+	unsigned long	futex_val;
+	unsigned long	futex_mask;
 	unsigned long	futexv_owned;
+	u32		futex_flags;
+	unsigned int	futex_nr;
 };
 
 struct io_futex_data {
@@ -171,15 +171,28 @@ bool io_futex_remove_all(struct io_ring_ctx *ctx, struct task_struct *task,
 int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 {
 	struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex);
+	u32 flags;
 
-	if (unlikely(sqe->fd || sqe->buf_index || sqe->addr3))
+	if (unlikely(sqe->fd || sqe->buf_index || sqe->file_index))
 		return -EINVAL;
 
 	iof->uaddr = u64_to_user_ptr(READ_ONCE(sqe->addr));
-	iof->futex_val = READ_ONCE(sqe->len);
-	iof->futex_mask = READ_ONCE(sqe->file_index);
-	iof->futex_flags = READ_ONCE(sqe->futex_flags);
-	if (iof->futex_flags & FUTEX_CMD_MASK)
+	iof->futex_val = READ_ONCE(sqe->addr2);
+	iof->futex_mask = READ_ONCE(sqe->addr3);
+	iof->futex_nr = READ_ONCE(sqe->len);
+	if (iof->futex_nr && req->opcode != IORING_OP_FUTEX_WAITV)
+		return -EINVAL;
+
+	flags = READ_ONCE(sqe->futex_flags);
+	if (flags & ~FUTEX2_MASK)
+		return -EINVAL;
+
+	iof->futex_flags = futex2_to_flags(flags);
+	if (!futex_flags_valid(iof->futex_flags))
+		return -EINVAL;
+
+	if (!futex_validate_input(iof->futex_flags, iof->futex_val) ||
+	    !futex_validate_input(iof->futex_flags, iof->futex_mask))
 		return -EINVAL;
 
 	iof->futexv_owned = 0;
@@ -211,7 +224,6 @@ int io_futexv_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 	if (ret)
 		return ret;
 
-	iof->futex_nr = READ_ONCE(sqe->off);
 	if (!iof->futex_nr || iof->futex_nr > FUTEX_WAITV_MAX)
 		return -EINVAL;
 

-- 
Jens Axboe


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH 06/10] io_uring: add support for futex wake and wait
  2023-07-21 15:29         ` Jens Axboe
@ 2023-07-25 13:00           ` Peter Zijlstra
  2023-07-25 13:48             ` Jens Axboe
  0 siblings, 1 reply; 22+ messages in thread
From: Peter Zijlstra @ 2023-07-25 13:00 UTC (permalink / raw)
  To: Jens Axboe; +Cc: io-uring, linux-kernel, andres

On Fri, Jul 21, 2023 at 09:29:14AM -0600, Jens Axboe wrote:

> FWIW, here's the io_uring incremental after that rebase. Update the
> liburing futex branch as well, updating the prep helpers to take 64 bit
> values for mask/val and also add the flags argument that was missing as
> well. Only other addition was adding those 4 new patches instead of the
> old 3 ones, and adding single patch that just moves FUTEX2_MASK to
> futex.h.
> 
> All checks out fine, tests pass and it works.
> 
> 
> diff --git a/io_uring/futex.c b/io_uring/futex.c
> index 93df54dffaa0..4c9f2c841b98 100644
> --- a/io_uring/futex.c
> +++ b/io_uring/futex.c
> @@ -18,11 +18,11 @@ struct io_futex {
>  		u32 __user			*uaddr;
>  		struct futex_waitv __user	*uwaitv;
>  	};
> +	unsigned long	futex_val;
> +	unsigned long	futex_mask;
>  	unsigned long	futexv_owned;
> +	u32		futex_flags;
> +	unsigned int	futex_nr;
>  };
>  
>  struct io_futex_data {
> @@ -171,15 +171,28 @@ bool io_futex_remove_all(struct io_ring_ctx *ctx, struct task_struct *task,
>  int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>  {
>  	struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex);
> +	u32 flags;
>  
> +	if (unlikely(sqe->fd || sqe->buf_index || sqe->file_index))
>  		return -EINVAL;
>  
>  	iof->uaddr = u64_to_user_ptr(READ_ONCE(sqe->addr));
> +	iof->futex_val = READ_ONCE(sqe->addr2);
> +	iof->futex_mask = READ_ONCE(sqe->addr3);
> +	iof->futex_nr = READ_ONCE(sqe->len);
> +	if (iof->futex_nr && req->opcode != IORING_OP_FUTEX_WAITV)
> +		return -EINVAL;
> +

Hmm, would something like:

	if (req->opcode == IORING_OP_FUTEX_WAITV) {
		if (iof->futex_val && iof->futex_mask)
			return -EINVAL;

		/* sys_futex_waitv() doesn't take @flags as of yet */
		if (iof->futex_flags)
			return -EINVAL;

		if (!iof->futex_nr)
			return -EINVAL;

	} else {
		/* sys_futex_{wake,wait}() don't take @nr */
		if (iof->futex_nr)
			return -EINVAL;

		/* both take @flags and @mask */
		flags = READ_ONCE(sqe->futex_flags);
		if (flags & ~FUTEX2_MASK)
			return -EINVAL;

		iof->futex_flags = futex2_to_flags(flags);
		if (!futex_flags_valid(iof->futex_flags))
			return -EINVAL;

		if (!futex_validate_input(iof->futex_flags, iof->futex_mask))
			return -EINVAL;

		/* sys_futex_wait() takes @val */
		if (req->iocode == IORING_OP_FUTEX_WAIT) {
			if (!futex_validate_input(iof->futex_flags, iof->futex_val))
				return -EINVAL;
		} else {
			if (iof->futex_val)
				return -EINVAL;
		}
	}

work? The waitv thing is significantly different from the other two.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 06/10] io_uring: add support for futex wake and wait
  2023-07-25 13:00           ` Peter Zijlstra
@ 2023-07-25 13:48             ` Jens Axboe
  2023-07-25 13:57               ` Jens Axboe
  2023-07-25 14:06               ` Peter Zijlstra
  0 siblings, 2 replies; 22+ messages in thread
From: Jens Axboe @ 2023-07-25 13:48 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: io-uring, linux-kernel, andres

On 7/25/23 7:00?AM, Peter Zijlstra wrote:
> On Fri, Jul 21, 2023 at 09:29:14AM -0600, Jens Axboe wrote:
> 
>> FWIW, here's the io_uring incremental after that rebase. Update the
>> liburing futex branch as well, updating the prep helpers to take 64 bit
>> values for mask/val and also add the flags argument that was missing as
>> well. Only other addition was adding those 4 new patches instead of the
>> old 3 ones, and adding single patch that just moves FUTEX2_MASK to
>> futex.h.
>>
>> All checks out fine, tests pass and it works.
>>
>>
>> diff --git a/io_uring/futex.c b/io_uring/futex.c
>> index 93df54dffaa0..4c9f2c841b98 100644
>> --- a/io_uring/futex.c
>> +++ b/io_uring/futex.c
>> @@ -18,11 +18,11 @@ struct io_futex {
>>  		u32 __user			*uaddr;
>>  		struct futex_waitv __user	*uwaitv;
>>  	};
>> +	unsigned long	futex_val;
>> +	unsigned long	futex_mask;
>>  	unsigned long	futexv_owned;
>> +	u32		futex_flags;
>> +	unsigned int	futex_nr;
>>  };
>>  
>>  struct io_futex_data {
>> @@ -171,15 +171,28 @@ bool io_futex_remove_all(struct io_ring_ctx *ctx, struct task_struct *task,
>>  int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>>  {
>>  	struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex);
>> +	u32 flags;
>>  
>> +	if (unlikely(sqe->fd || sqe->buf_index || sqe->file_index))
>>  		return -EINVAL;
>>  
>>  	iof->uaddr = u64_to_user_ptr(READ_ONCE(sqe->addr));
>> +	iof->futex_val = READ_ONCE(sqe->addr2);
>> +	iof->futex_mask = READ_ONCE(sqe->addr3);
>> +	iof->futex_nr = READ_ONCE(sqe->len);
>> +	if (iof->futex_nr && req->opcode != IORING_OP_FUTEX_WAITV)
>> +		return -EINVAL;
>> +
> 
> Hmm, would something like:
> 
> 	if (req->opcode == IORING_OP_FUTEX_WAITV) {
> 		if (iof->futex_val && iof->futex_mask)
> 			return -EINVAL;
> 
> 		/* sys_futex_waitv() doesn't take @flags as of yet */
> 		if (iof->futex_flags)
> 			return -EINVAL;
> 
> 		if (!iof->futex_nr)
> 			return -EINVAL;
> 
> 	} else {
> 		/* sys_futex_{wake,wait}() don't take @nr */
> 		if (iof->futex_nr)
> 			return -EINVAL;
> 
> 		/* both take @flags and @mask */
> 		flags = READ_ONCE(sqe->futex_flags);
> 		if (flags & ~FUTEX2_MASK)
> 			return -EINVAL;
> 
> 		iof->futex_flags = futex2_to_flags(flags);
> 		if (!futex_flags_valid(iof->futex_flags))
> 			return -EINVAL;
> 
> 		if (!futex_validate_input(iof->futex_flags, iof->futex_mask))
> 			return -EINVAL;
> 
> 		/* sys_futex_wait() takes @val */
> 		if (req->iocode == IORING_OP_FUTEX_WAIT) {
> 			if (!futex_validate_input(iof->futex_flags, iof->futex_val))
> 				return -EINVAL;
> 		} else {
> 			if (iof->futex_val)
> 				return -EINVAL;
> 		}
> 	}
> 
> work? The waitv thing is significantly different from the other two.

I think I'll just have prep and prepv totally separate. It only makes
sense to share parts of them if one is a subset of the other. That'll
get rid of the odd conditionals and sectioning of it.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 06/10] io_uring: add support for futex wake and wait
  2023-07-25 13:48             ` Jens Axboe
@ 2023-07-25 13:57               ` Jens Axboe
  2023-07-25 15:19                 ` Peter Zijlstra
  2023-07-25 14:06               ` Peter Zijlstra
  1 sibling, 1 reply; 22+ messages in thread
From: Jens Axboe @ 2023-07-25 13:57 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: io-uring, linux-kernel, andres

On 7/25/23 7:48?AM, Jens Axboe wrote:
> On 7/25/23 7:00?AM, Peter Zijlstra wrote:
>> On Fri, Jul 21, 2023 at 09:29:14AM -0600, Jens Axboe wrote:
>>
>>> FWIW, here's the io_uring incremental after that rebase. Update the
>>> liburing futex branch as well, updating the prep helpers to take 64 bit
>>> values for mask/val and also add the flags argument that was missing as
>>> well. Only other addition was adding those 4 new patches instead of the
>>> old 3 ones, and adding single patch that just moves FUTEX2_MASK to
>>> futex.h.
>>>
>>> All checks out fine, tests pass and it works.
>>>
>>>
>>> diff --git a/io_uring/futex.c b/io_uring/futex.c
>>> index 93df54dffaa0..4c9f2c841b98 100644
>>> --- a/io_uring/futex.c
>>> +++ b/io_uring/futex.c
>>> @@ -18,11 +18,11 @@ struct io_futex {
>>>  		u32 __user			*uaddr;
>>>  		struct futex_waitv __user	*uwaitv;
>>>  	};
>>> +	unsigned long	futex_val;
>>> +	unsigned long	futex_mask;
>>>  	unsigned long	futexv_owned;
>>> +	u32		futex_flags;
>>> +	unsigned int	futex_nr;
>>>  };
>>>  
>>>  struct io_futex_data {
>>> @@ -171,15 +171,28 @@ bool io_futex_remove_all(struct io_ring_ctx *ctx, struct task_struct *task,
>>>  int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>>>  {
>>>  	struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex);
>>> +	u32 flags;
>>>  
>>> +	if (unlikely(sqe->fd || sqe->buf_index || sqe->file_index))
>>>  		return -EINVAL;
>>>  
>>>  	iof->uaddr = u64_to_user_ptr(READ_ONCE(sqe->addr));
>>> +	iof->futex_val = READ_ONCE(sqe->addr2);
>>> +	iof->futex_mask = READ_ONCE(sqe->addr3);
>>> +	iof->futex_nr = READ_ONCE(sqe->len);
>>> +	if (iof->futex_nr && req->opcode != IORING_OP_FUTEX_WAITV)
>>> +		return -EINVAL;
>>> +
>>
>> Hmm, would something like:
>>
>> 	if (req->opcode == IORING_OP_FUTEX_WAITV) {
>> 		if (iof->futex_val && iof->futex_mask)
>> 			return -EINVAL;
>>
>> 		/* sys_futex_waitv() doesn't take @flags as of yet */
>> 		if (iof->futex_flags)
>> 			return -EINVAL;
>>
>> 		if (!iof->futex_nr)
>> 			return -EINVAL;
>>
>> 	} else {
>> 		/* sys_futex_{wake,wait}() don't take @nr */
>> 		if (iof->futex_nr)
>> 			return -EINVAL;
>>
>> 		/* both take @flags and @mask */
>> 		flags = READ_ONCE(sqe->futex_flags);
>> 		if (flags & ~FUTEX2_MASK)
>> 			return -EINVAL;
>>
>> 		iof->futex_flags = futex2_to_flags(flags);
>> 		if (!futex_flags_valid(iof->futex_flags))
>> 			return -EINVAL;
>>
>> 		if (!futex_validate_input(iof->futex_flags, iof->futex_mask))
>> 			return -EINVAL;
>>
>> 		/* sys_futex_wait() takes @val */
>> 		if (req->iocode == IORING_OP_FUTEX_WAIT) {
>> 			if (!futex_validate_input(iof->futex_flags, iof->futex_val))
>> 				return -EINVAL;
>> 		} else {
>> 			if (iof->futex_val)
>> 				return -EINVAL;
>> 		}
>> 	}
>>
>> work? The waitv thing is significantly different from the other two.
> 
> I think I'll just have prep and prepv totally separate. It only makes
> sense to share parts of them if one is a subset of the other. That'll
> get rid of the odd conditionals and sectioning of it.

Something like the below - totally untested, but just to show what I
mean. Will need to get split and folded into the two separate patches.
Will test and fold them later today.


diff --git a/io_uring/futex.c b/io_uring/futex.c
index 4c9f2c841b98..b0f90154d974 100644
--- a/io_uring/futex.c
+++ b/io_uring/futex.c
@@ -168,7 +168,7 @@ bool io_futex_remove_all(struct io_ring_ctx *ctx, struct task_struct *task,
 	return found;
 }
 
-int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
+static int __io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 {
 	struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex);
 	u32 flags;
@@ -179,9 +179,6 @@ int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 	iof->uaddr = u64_to_user_ptr(READ_ONCE(sqe->addr));
 	iof->futex_val = READ_ONCE(sqe->addr2);
 	iof->futex_mask = READ_ONCE(sqe->addr3);
-	iof->futex_nr = READ_ONCE(sqe->len);
-	if (iof->futex_nr && req->opcode != IORING_OP_FUTEX_WAITV)
-		return -EINVAL;
 
 	flags = READ_ONCE(sqe->futex_flags);
 	if (flags & ~FUTEX2_MASK)
@@ -191,14 +188,36 @@ int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 	if (!futex_flags_valid(iof->futex_flags))
 		return -EINVAL;
 
-	if (!futex_validate_input(iof->futex_flags, iof->futex_val) ||
-	    !futex_validate_input(iof->futex_flags, iof->futex_mask))
+	if (!futex_validate_input(iof->futex_flags, iof->futex_mask))
 		return -EINVAL;
 
-	iof->futexv_owned = 0;
 	return 0;
 }
 
+int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
+{
+	struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex);
+	int ret;
+
+	if (unlikely(sqe->len))
+		return -EINVAL;
+
+	ret = __io_futex_prep(req, sqe);
+	if (ret)
+		return ret;
+
+	/* sys_futex_wait() takes @val */
+	if (req->opcode == IORING_OP_FUTEX_WAIT) {
+		if (!futex_validate_input(iof->futex_flags, iof->futex_val))
+			return -EINVAL;
+	} else {
+		if (iof->futex_val)
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
 static void io_futex_wakev_fn(struct wake_q_head *wake_q, struct futex_q *q)
 {
 	struct io_kiocb *req = q->wake_data;
@@ -220,10 +239,15 @@ int io_futexv_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 	struct futex_vector *futexv;
 	int ret;
 
-	ret = io_futex_prep(req, sqe);
+	ret = __io_futex_prep(req, sqe);
 	if (ret)
 		return ret;
 
+	/* No flags supported for waitv */
+	if (iof->futex_flags)
+		return -EINVAL;
+
+	iof->futex_nr = READ_ONCE(sqe->len);
 	if (!iof->futex_nr || iof->futex_nr > FUTEX_WAITV_MAX)
 		return -EINVAL;
 
@@ -238,6 +262,7 @@ int io_futexv_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 		return ret;
 	}
 
+	iof->futexv_owned = 0;
 	req->flags |= REQ_F_ASYNC_DATA;
 	req->async_data = futexv;
 	return 0;


-- 
Jens Axboe


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH 06/10] io_uring: add support for futex wake and wait
  2023-07-25 13:48             ` Jens Axboe
  2023-07-25 13:57               ` Jens Axboe
@ 2023-07-25 14:06               ` Peter Zijlstra
  1 sibling, 0 replies; 22+ messages in thread
From: Peter Zijlstra @ 2023-07-25 14:06 UTC (permalink / raw)
  To: Jens Axboe; +Cc: io-uring, linux-kernel, andres

On Tue, Jul 25, 2023 at 07:48:30AM -0600, Jens Axboe wrote:

> I think I'll just have prep and prepv totally separate. It only makes
> sense to share parts of them if one is a subset of the other. That'll
> get rid of the odd conditionals and sectioning of it.

Ah, yes. Fair enough.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 06/10] io_uring: add support for futex wake and wait
  2023-07-25 13:57               ` Jens Axboe
@ 2023-07-25 15:19                 ` Peter Zijlstra
  2023-07-25 20:42                   ` Jens Axboe
  0 siblings, 1 reply; 22+ messages in thread
From: Peter Zijlstra @ 2023-07-25 15:19 UTC (permalink / raw)
  To: Jens Axboe; +Cc: io-uring, linux-kernel, andres

On Tue, Jul 25, 2023 at 07:57:28AM -0600, Jens Axboe wrote:

> Something like the below - totally untested, but just to show what I
> mean. Will need to get split and folded into the two separate patches.
> Will test and fold them later today.
> 
> 
> diff --git a/io_uring/futex.c b/io_uring/futex.c
> index 4c9f2c841b98..b0f90154d974 100644
> --- a/io_uring/futex.c
> +++ b/io_uring/futex.c
> @@ -168,7 +168,7 @@ bool io_futex_remove_all(struct io_ring_ctx *ctx, struct task_struct *task,
>  	return found;
>  }
>  
> -int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
> +static int __io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>  {
>  	struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex);
>  	u32 flags;
> @@ -179,9 +179,6 @@ int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>  	iof->uaddr = u64_to_user_ptr(READ_ONCE(sqe->addr));
>  	iof->futex_val = READ_ONCE(sqe->addr2);
>  	iof->futex_mask = READ_ONCE(sqe->addr3);
> -	iof->futex_nr = READ_ONCE(sqe->len);
> -	if (iof->futex_nr && req->opcode != IORING_OP_FUTEX_WAITV)
> -		return -EINVAL;
>  
>  	flags = READ_ONCE(sqe->futex_flags);
>  	if (flags & ~FUTEX2_MASK)
> @@ -191,14 +188,36 @@ int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>  	if (!futex_flags_valid(iof->futex_flags))
>  		return -EINVAL;
>  
> -	if (!futex_validate_input(iof->futex_flags, iof->futex_val) ||
> -	    !futex_validate_input(iof->futex_flags, iof->futex_mask))
> +	if (!futex_validate_input(iof->futex_flags, iof->futex_mask))
>  		return -EINVAL;
>  
> -	iof->futexv_owned = 0;
>  	return 0;
>  }

I think you can/should split more into io_futex_prep(), specifically
waitv should also have zero @val and @mask.

But yes, something like this makes sense.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 06/10] io_uring: add support for futex wake and wait
  2023-07-25 15:19                 ` Peter Zijlstra
@ 2023-07-25 20:42                   ` Jens Axboe
  2023-07-25 21:24                     ` Jens Axboe
  0 siblings, 1 reply; 22+ messages in thread
From: Jens Axboe @ 2023-07-25 20:42 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: io-uring, linux-kernel, andres

On 7/25/23 9:19?AM, Peter Zijlstra wrote:
> On Tue, Jul 25, 2023 at 07:57:28AM -0600, Jens Axboe wrote:
> 
>> Something like the below - totally untested, but just to show what I
>> mean. Will need to get split and folded into the two separate patches.
>> Will test and fold them later today.
>>
>>
>> diff --git a/io_uring/futex.c b/io_uring/futex.c
>> index 4c9f2c841b98..b0f90154d974 100644
>> --- a/io_uring/futex.c
>> +++ b/io_uring/futex.c
>> @@ -168,7 +168,7 @@ bool io_futex_remove_all(struct io_ring_ctx *ctx, struct task_struct *task,
>>  	return found;
>>  }
>>  
>> -int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>> +static int __io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>>  {
>>  	struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex);
>>  	u32 flags;
>> @@ -179,9 +179,6 @@ int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>>  	iof->uaddr = u64_to_user_ptr(READ_ONCE(sqe->addr));
>>  	iof->futex_val = READ_ONCE(sqe->addr2);
>>  	iof->futex_mask = READ_ONCE(sqe->addr3);
>> -	iof->futex_nr = READ_ONCE(sqe->len);
>> -	if (iof->futex_nr && req->opcode != IORING_OP_FUTEX_WAITV)
>> -		return -EINVAL;
>>  
>>  	flags = READ_ONCE(sqe->futex_flags);
>>  	if (flags & ~FUTEX2_MASK)
>> @@ -191,14 +188,36 @@ int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>>  	if (!futex_flags_valid(iof->futex_flags))
>>  		return -EINVAL;
>>  
>> -	if (!futex_validate_input(iof->futex_flags, iof->futex_val) ||
>> -	    !futex_validate_input(iof->futex_flags, iof->futex_mask))
>> +	if (!futex_validate_input(iof->futex_flags, iof->futex_mask))
>>  		return -EINVAL;
>>  
>> -	iof->futexv_owned = 0;
>>  	return 0;
>>  }
> 
> I think you can/should split more into io_futex_prep(), specifically
> waitv should also have zero @val and @mask.

Yep, I'll include that. Updating them now...

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 06/10] io_uring: add support for futex wake and wait
  2023-07-25 20:42                   ` Jens Axboe
@ 2023-07-25 21:24                     ` Jens Axboe
  0 siblings, 0 replies; 22+ messages in thread
From: Jens Axboe @ 2023-07-25 21:24 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: io-uring, linux-kernel, andres

On 7/25/23 2:42?PM, Jens Axboe wrote:
> On 7/25/23 9:19?AM, Peter Zijlstra wrote:
>> On Tue, Jul 25, 2023 at 07:57:28AM -0600, Jens Axboe wrote:
>>
>>> Something like the below - totally untested, but just to show what I
>>> mean. Will need to get split and folded into the two separate patches.
>>> Will test and fold them later today.
>>>
>>>
>>> diff --git a/io_uring/futex.c b/io_uring/futex.c
>>> index 4c9f2c841b98..b0f90154d974 100644
>>> --- a/io_uring/futex.c
>>> +++ b/io_uring/futex.c
>>> @@ -168,7 +168,7 @@ bool io_futex_remove_all(struct io_ring_ctx *ctx, struct task_struct *task,
>>>  	return found;
>>>  }
>>>  
>>> -int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>>> +static int __io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>>>  {
>>>  	struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex);
>>>  	u32 flags;
>>> @@ -179,9 +179,6 @@ int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>>>  	iof->uaddr = u64_to_user_ptr(READ_ONCE(sqe->addr));
>>>  	iof->futex_val = READ_ONCE(sqe->addr2);
>>>  	iof->futex_mask = READ_ONCE(sqe->addr3);
>>> -	iof->futex_nr = READ_ONCE(sqe->len);
>>> -	if (iof->futex_nr && req->opcode != IORING_OP_FUTEX_WAITV)
>>> -		return -EINVAL;
>>>  
>>>  	flags = READ_ONCE(sqe->futex_flags);
>>>  	if (flags & ~FUTEX2_MASK)
>>> @@ -191,14 +188,36 @@ int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>>>  	if (!futex_flags_valid(iof->futex_flags))
>>>  		return -EINVAL;
>>>  
>>> -	if (!futex_validate_input(iof->futex_flags, iof->futex_val) ||
>>> -	    !futex_validate_input(iof->futex_flags, iof->futex_mask))
>>> +	if (!futex_validate_input(iof->futex_flags, iof->futex_mask))
>>>  		return -EINVAL;
>>>  
>>> -	iof->futexv_owned = 0;
>>>  	return 0;
>>>  }
>>
>> I think you can/should split more into io_futex_prep(), specifically
>> waitv should also have zero @val and @mask.
> 
> Yep, I'll include that. Updating them now...

It ends up just being this incremental for the very last patch, moving
all the waitv related prep to the wait prep and not relying on the
non-vectored one at all.


diff --git a/io_uring/futex.c b/io_uring/futex.c
index 4c9f2c841b98..e885aac12df8 100644
--- a/io_uring/futex.c
+++ b/io_uring/futex.c
@@ -179,9 +179,6 @@ int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 	iof->uaddr = u64_to_user_ptr(READ_ONCE(sqe->addr));
 	iof->futex_val = READ_ONCE(sqe->addr2);
 	iof->futex_mask = READ_ONCE(sqe->addr3);
-	iof->futex_nr = READ_ONCE(sqe->len);
-	if (iof->futex_nr && req->opcode != IORING_OP_FUTEX_WAITV)
-		return -EINVAL;
 
 	flags = READ_ONCE(sqe->futex_flags);
 	if (flags & ~FUTEX2_MASK)
@@ -195,7 +192,6 @@ int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 	    !futex_validate_input(iof->futex_flags, iof->futex_mask))
 		return -EINVAL;
 
-	iof->futexv_owned = 0;
 	return 0;
 }
 
@@ -220,10 +216,13 @@ int io_futexv_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 	struct futex_vector *futexv;
 	int ret;
 
-	ret = io_futex_prep(req, sqe);
-	if (ret)
-		return ret;
+	/* No flags or mask supported for waitv */
+	if (unlikely(sqe->fd || sqe->buf_index || sqe->file_index ||
+		     sqe->addr2 || sqe->addr3))
+		return -EINVAL;
 
+	iof->uaddr = u64_to_user_ptr(READ_ONCE(sqe->addr));
+	iof->futex_nr = READ_ONCE(sqe->len);
 	if (!iof->futex_nr || iof->futex_nr > FUTEX_WAITV_MAX)
 		return -EINVAL;
 
@@ -238,6 +237,7 @@ int io_futexv_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 		return ret;
 	}
 
+	iof->futexv_owned = 0;
 	req->flags |= REQ_F_ASYNC_DATA;
 	req->async_data = futexv;
 	return 0;

-- 
Jens Axboe


^ permalink raw reply related	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2023-07-25 21:24 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-20 22:18 [PATCHSET v3 0/10] Add io_uring futex/futexv support Jens Axboe
2023-07-20 22:18 ` [PATCH 01/10] futex: Clarify FUTEX2 flags Jens Axboe
2023-07-20 22:18 ` [PATCH 02/10] futex: Extend the " Jens Axboe
2023-07-20 22:18 ` [PATCH 03/10] futex: Flag conversion Jens Axboe
2023-07-20 22:18 ` [PATCH 04/10] futex: factor out the futex wake handling Jens Axboe
2023-07-20 22:18 ` [PATCH 05/10] futex: abstract out a __futex_wake_mark() helper Jens Axboe
2023-07-20 22:18 ` [PATCH 06/10] io_uring: add support for futex wake and wait Jens Axboe
2023-07-21 11:30   ` Peter Zijlstra
2023-07-21 11:37     ` Peter Zijlstra
2023-07-21 14:43       ` Jens Axboe
2023-07-21 15:29         ` Jens Axboe
2023-07-25 13:00           ` Peter Zijlstra
2023-07-25 13:48             ` Jens Axboe
2023-07-25 13:57               ` Jens Axboe
2023-07-25 15:19                 ` Peter Zijlstra
2023-07-25 20:42                   ` Jens Axboe
2023-07-25 21:24                     ` Jens Axboe
2023-07-25 14:06               ` Peter Zijlstra
2023-07-20 22:18 ` [PATCH 07/10] futex: add wake_data to struct futex_q Jens Axboe
2023-07-20 22:18 ` [PATCH 08/10] futex: make futex_parse_waitv() available as a helper Jens Axboe
2023-07-20 22:18 ` [PATCH 09/10] futex: make the vectored futex operations available Jens Axboe
2023-07-20 22:18 ` [PATCH 10/10] io_uring: add support for vectored futex waits Jens Axboe

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox