Optimise io_uring completion waiting

From: Pavel Begunkov <[email protected]>

Add wait_threshold -- a custom wait_event derivative, that waits until a value is equal to or greater than the specified threshold.

Quoted 4 lines · Pavel Begunkov (Silence)

From: Pavel Begunkov <[email protected]>

Add wait_threshold -- a custom wait_event derivative, that waits until a value is equal to or greater than the specified threshold.

This is quite insufficient justification for this monster... what exact semantics do you want?

Why can't you do this exact same with a slightly more complicated @cond ?

Just in case duplicating a mail from the cover-letter thread:

It could be done with @cond indeed, that's how it works for now. However, this addresses performance issues only.

The problem with wait_event_*() is that, if we have a counter and are trying to wake up tasks after each increment, it would schedule each waiting task O(threshold) times just for it to spuriously check @cond and go back to sleep. All that overhead (memory barriers, registers save/load, accounting, etc) turned out to be enough for some workloads to slow down the system.

With this specialisation it still traverses a wait list and makes indirect calls to the checker callback, but the list supposedly is fairly small, so performance there shouldn't be a problem, at least for now.

Regarding semantics; It should wake a task when a value passed to wake_up_threshold() is greater or equal then a task's threshold, that is specified individually for each task in wait_threshold_*().

In pseudo code:

```
def wake_up_threshold(n, wait_queue):
	for waiter in wait_queue:
		waiter.wake_up_if(n >= waiter.threshold);
```

Any thoughts how to do it better? Ideas are very welcome.

BTW, this monster is mostly a copy-paste from wait_event_*(), wait_bit_*(). We could try to extract some common parts from these three, but that's another topic.

Also, is anything actually using wait_queue_entry::private ? I'm not finding any in a hurry.

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

Quoted 1 line · Pavel Begunkov

Just in case duplicating a mail from the cover-letter thread:

Just because every patch should have a self contained and coherent Changelog.

Quoted 4 lines

BTW, this monster is mostly a copy-paste from wait_event_*(), wait_bit_*(). We could try to extract some common parts from these three, but that's another topic.

I don't think that is another topic at all. It is a quality of implementation issue. We already have too many copies of all that (3).

So basically you want to fudge the wake function to do the/a @cond test, not unlike what wait_bit already does, but differenly.

I'm really rather annoyed with C for not having proper lambda functions; that would make all this so much easier. Anyway, let me have a poke at this in the morning, it's late already.

Also, is anything actually using wait_queue_entry::private ? I'm not finding any in a hurry.

From: Pavel Begunkov <[email protected]>

There could be a lot of overhead within generic wait_event_*() used for waiting for large number of completions. The patchset removes much of it by using custom wait event (wait_threshold).

Synthetic test showed ~40% performance boost. (see patch 2)

Quoted 6 lines · Pavel Begunkov (Silence)

From: Pavel Begunkov <[email protected]>

There could be a lot of overhead within generic wait_event_*() used for waiting for large number of completions. The patchset removes much of it by using custom wait event (wait_threshold).

Quoted 1 line

Synthetic test showed ~40% performance boost. (see patch 2)

I'm fine with the io_uring side of things, but to queue this up we really need Peter or Ingo to sign off on the core wakeup bits...

Peter?

Quoted 7 lines · Jens Axboe

Quoted 6 lines · Pavel Begunkov (Silence)

From: Pavel Begunkov <[email protected]>

There could be a lot of overhead within generic wait_event_*() used for waiting for large number of completions. The patchset removes much of it by using custom wait event (wait_threshold).

Quoted 6 lines

Quoted 1 line

Synthetic test showed ~40% performance boost. (see patch 2)

I'm fine with the io_uring side of things, but to queue this up we really need Peter or Ingo to sign off on the core wakeup bits...

Peter?

I'm not sure an extension is needed for such a special interface, why not just put a ->threshold value next to the ctx->wait field and use either the regular wait_event() APIs with the proper condition, or wait_event_cmd() style APIs if you absolutely need something more complex to happen inside?

Should result in a much lower linecount and no scheduler changes. :-)

Thanks,

Ingo

Quoted 6 lines

I'm not sure an extension is needed for such a special interface, why not just put a ->threshold value next to the ctx->wait field and use either the regular wait_event() APIs with the proper condition, or wait_event_cmd() style APIs if you absolutely need something more complex to happen inside?

Quoted 7 lines

Should result in a much lower linecount and no scheduler changes. :-)

Thanks,

Ingo

Sorry, mixed the threads.

Quoted 6 lines

Quoted 6 lines

I'm not sure an extension is needed for such a special interface, why not just put a ->threshold value next to the ctx->wait field and use either the regular wait_event() APIs with the proper condition, or wait_event_cmd() style APIs if you absolutely need something more complex to happen inside?

Ingo,
io_uring works well without this patch just using wait_event_*() with proper condition, but there are performance issues with spurious wakeups. Detailed description in the previous mail.
Am I missing something?

Quoted 8 lines · Pavel Begunkov

Sorry, mixed the threads.

Quoted 6 lines

Quoted 6 lines

I'm not sure an extension is needed for such a special interface, why not just put a ->threshold value next to the ctx->wait field and use either the regular wait_event() APIs with the proper condition, or wait_event_cmd() style APIs if you absolutely need something more complex to happen inside?

Quoted 5 lines

Ingo,
io_uring works well without this patch just using wait_event_*() with proper condition, but there are performance issues with spurious wakeups. Detailed description in the previous mail.
Am I missing something?

I think we can do the same thing, just wrapping the waitqueue in a structure with a count in it, on the stack. Got some flight time coming up later today, let me try and cook up a patch.

Quoted 3 lines

I think we can do the same thing, just wrapping the waitqueue in a structure with a count in it, on the stack. Got some flight time coming up later today, let me try and cook up a patch.

Totally untested, and sent out 5 min before departure... But something like this.

fs/io_uring.c +50 -10

index ca7570aca430..c2f9e1da26dd 100644
@@ -2768,6 +2768,37 @@ static int io_ring_submit(struct io_ring_ctx *ctx, unsigned int to_submit,
 	return submit;
 }
 
+struct io_wait_queue {
+	struct wait_queue_entry wq;
+	struct io_ring_ctx *ctx;
+	struct task_struct *task;
+	unsigned to_wait;
+	unsigned nr_timeouts;
+};
+
+static inline bool io_should_wake(struct io_wait_queue *iowq)
+{
+	struct io_ring_ctx *ctx = iowq->ctx;
+
+	return io_cqring_events(ctx->rings) >= iowq->to_wait ||
+			atomic_read(&ctx->cq_timeouts) != iowq->nr_timeouts;
+}
+
+static int io_wake_function(struct wait_queue_entry *curr, unsigned int mode,
+			    int wake_flags, void *key)
+{
+	struct io_wait_queue *iowq = container_of(curr, struct io_wait_queue,
+							wq);
+
+	if (io_should_wake(iowq)) {
+		list_del_init(&curr->entry);
+		wake_up_process(iowq->task);
+		return 1;
+	}
+
+	return -1;
+}
+
 /*
  * Wait until events become available, if we don't already have some. The
  * application must reap them itself, as they reside on the shared cq ring.

@@ -2775,8 +2806,16 @@ static int io_ring_submit(struct io_ring_ctx *ctx, unsigned int to_submit,
 static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events,
 			  const sigset_t __user *sig, size_t sigsz)
 {
+	struct io_wait_queue iowq = {
+		.wq = {
+			.func	= io_wake_function,
+			.entry	= LIST_HEAD_INIT(iowq.wq.entry),
+		},
+		.task		= current,
+		.ctx		= ctx,
+		.to_wait	= min_events,
+	};
 	struct io_rings *rings = ctx->rings;
-	unsigned nr_timeouts;
 	int ret;
 
 	if (io_cqring_events(rings) >= min_events)

@@ -2795,15 +2834,16 @@ static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events,
 			return ret;
 	}
 
-	nr_timeouts = atomic_read(&ctx->cq_timeouts);
-	/*
-	 * Return if we have enough events, or if a timeout occured since
-	 * we started waiting. For timeouts, we always want to return to
-	 * userspace.
-	 */
-	ret = wait_event_interruptible(ctx->wait,
-				io_cqring_events(rings) >= min_events ||
-				atomic_read(&ctx->cq_timeouts) != nr_timeouts);
+	iowq.nr_timeouts = atomic_read(&ctx->cq_timeouts);
+	prepare_to_wait_exclusive(&ctx->wait, &iowq.wq, TASK_INTERRUPTIBLE);
+	do {
+		if (io_should_wake(&iowq))
+			break;
+		schedule();
+		set_current_state(TASK_INTERRUPTIBLE);
+	} while (1);
+	finish_wait(&ctx->wait, &iowq.wq);
+
 	restore_saved_sigmask_unless(ret == -ERESTARTSYS);
 	if (ret == -ERESTARTSYS)
 		ret = -EINTR;

Quoted 4 lines · Jens Axboe

Quoted 3 lines

I think we can do the same thing, just wrapping the waitqueue in a structure with a count in it, on the stack. Got some flight time coming up later today, let me try and cook up a patch.

Quoted 2 lines

Totally untested, and sent out 5 min before departure... But something like this.

Hmm, reminds me my first version. Basically that's the same thing but with macroses inlined. I wanted to make it reusable and self-contained, though.

If you don't think it could be useful in other places, sure, we could do something like that. Is that so?

Quoted 5 lines · Pavel Begunkov

Quoted 4 lines · Jens Axboe

Quoted 3 lines

I think we can do the same thing, just wrapping the waitqueue in a structure with a count in it, on the stack. Got some flight time coming up later today, let me try and cook up a patch.

Quoted 8 lines

Quoted 2 lines

Totally untested, and sent out 5 min before departure... But something like this.

Hmm, reminds me my first version. Basically that's the same thing but with macroses inlined. I wanted to make it reusable and self-contained, though.

If you don't think it could be useful in other places, sure, we could do something like that. Is that so?

I totally agree it could be useful in other places. Maybe formalized and used with wake_up_nr() instead of adding a new primitive? Haven't looked into that, I may be talking nonsense.

In any case, I did get a chance to test it and it works for me. Here's the "finished" version, slightly cleaned up and with a comment added for good measure.

Quoted 15 lines · Jens Axboe

Quoted 5 lines · Pavel Begunkov

Quoted 4 lines · Jens Axboe

Quoted 3 lines

I think we can do the same thing, just wrapping the waitqueue in a structure with a count in it, on the stack. Got some flight time coming up later today, let me try and cook up a patch.

Quoted 8 lines

Quoted 2 lines

Totally untested, and sent out 5 min before departure... But something like this.

Hmm, reminds me my first version. Basically that's the same thing but with macroses inlined. I wanted to make it reusable and self-contained, though.

If you don't think it could be useful in other places, sure, we could do something like that. Is that so?

Quoted 7 lines

I totally agree it could be useful in other places. Maybe formalized and used with wake_up_nr() instead of adding a new primitive? Haven't looked into that, I may be talking nonsense.

In any case, I did get a chance to test it and it works for me. Here's the "finished" version, slightly cleaned up and with a comment added for good measure.

Notes:

This version gets the ordering right, you need exclusive waits to get fifo ordering on the waitqueue.

Both versions (yours and mine) suffer from the problem of potentially waking too many. I don't think this is a real issue, as generally we don't do threaded access to the io_urings. But if you had the following tasks wait on the cqring:

[min_events = 32], [min_events = 8], [min_events = 8]

and we reach the io_cqring_events() == threshold, we'll wake all three. I don't see a good solution to this, so I suspect we just live with until proven an issue. Both versions are much better than what we have now.

Quoted 36 lines · Jens Axboe

Quoted 15 lines · Jens Axboe

Quoted 5 lines · Pavel Begunkov

Quoted 4 lines · Jens Axboe

Quoted 3 lines

I think we can do the same thing, just wrapping the waitqueue in a structure with a count in it, on the stack. Got some flight time coming up later today, let me try and cook up a patch.

Quoted 8 lines

Quoted 2 lines

Totally untested, and sent out 5 min before departure... But something like this.

Hmm, reminds me my first version. Basically that's the same thing but with macroses inlined. I wanted to make it reusable and self-contained, though.

If you don't think it could be useful in other places, sure, we could do something like that. Is that so?

Quoted 7 lines

I totally agree it could be useful in other places. Maybe formalized and used with wake_up_nr() instead of adding a new primitive? Haven't looked into that, I may be talking nonsense.

In any case, I did get a chance to test it and it works for me. Here's the "finished" version, slightly cleaned up and with a comment added for good measure.

Notes:

This version gets the ordering right, you need exclusive waits to get fifo ordering on the waitqueue.

Both versions (yours and mine) suffer from the problem of potentially waking too many. I don't think this is a real issue, as generally we don't do threaded access to the io_urings. But if you had the following tasks wait on the cqring:

[min_events = 32], [min_events = 8], [min_events = 8]

Quoted 4 lines

and we reach the io_cqring_events() == threshold, we'll wake all three. I don't see a good solution to this, so I suspect we just live with until proven an issue. Both versions are much better than what we have now.

Forgot an issue around signal handling, version below adds the right check for that too.

Curious what your test case was for this?

fs/io_uring.c +57 -10

index ca7570aca430..3fbab5692f14 100644
@@ -2768,6 +2768,42 @@ static int io_ring_submit(struct io_ring_ctx *ctx, unsigned int to_submit,
 	return submit;
 }
 
+struct io_wait_queue {
+	struct wait_queue_entry wq;
+	struct io_ring_ctx *ctx;
+	struct task_struct *task;
+	unsigned to_wait;
+	unsigned nr_timeouts;
+};
+
+static inline bool io_should_wake(struct io_wait_queue *iowq)
+{
+	struct io_ring_ctx *ctx = iowq->ctx;
+
+	/*
+	 * Wake up if we have enough events, or if a timeout occured since we
+	 * started waiting. For timeouts, we always want to return to userspace,
+	 * regardless of event count.
+	 */
+	return io_cqring_events(ctx->rings) >= iowq->to_wait ||
+			atomic_read(&ctx->cq_timeouts) != iowq->nr_timeouts;
+}
+
+static int io_wake_function(struct wait_queue_entry *curr, unsigned int mode,
+			    int wake_flags, void *key)
+{
+	struct io_wait_queue *iowq = container_of(curr, struct io_wait_queue,
+							wq);
+
+	if (io_should_wake(iowq)) {
+		list_del_init(&curr->entry);
+		wake_up_process(iowq->task);
+		return 1;
+	}
+
+	return -1;
+}
+
 /*
  * Wait until events become available, if we don't already have some. The
  * application must reap them itself, as they reside on the shared cq ring.

@@ -2775,8 +2811,16 @@ static int io_ring_submit(struct io_ring_ctx *ctx, unsigned int to_submit,
 static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events,
 			  const sigset_t __user *sig, size_t sigsz)
 {
+	struct io_wait_queue iowq = {
+		.wq = {
+			.func	= io_wake_function,
+			.entry	= LIST_HEAD_INIT(iowq.wq.entry),
+		},
+		.task		= current,
+		.ctx		= ctx,
+		.to_wait	= min_events,
+	};
 	struct io_rings *rings = ctx->rings;
-	unsigned nr_timeouts;
 	int ret;
 
 	if (io_cqring_events(rings) >= min_events)

@@ -2795,15 +2839,18 @@ static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events,
 			return ret;
 	}
 
-	nr_timeouts = atomic_read(&ctx->cq_timeouts);
-	/*
-	 * Return if we have enough events, or if a timeout occured since
-	 * we started waiting. For timeouts, we always want to return to
-	 * userspace.
-	 */
-	ret = wait_event_interruptible(ctx->wait,
-				io_cqring_events(rings) >= min_events ||
-				atomic_read(&ctx->cq_timeouts) != nr_timeouts);
+	iowq.nr_timeouts = atomic_read(&ctx->cq_timeouts);
+	prepare_to_wait_exclusive(&ctx->wait, &iowq.wq, TASK_INTERRUPTIBLE);
+	do {
+		if (io_should_wake(&iowq))
+			break;
+		schedule();
+		if (signal_pending(current))
+			break;
+		set_current_state(TASK_INTERRUPTIBLE);
+	} while (1);
+	finish_wait(&ctx->wait, &iowq.wq);
+
 	restore_saved_sigmask_unless(ret == -ERESTARTSYS);
 	if (ret == -ERESTARTSYS)
 		ret = -EINTR;

Quoted 42 lines · Jens Axboe

Quoted 40 lines · Jens Axboe

Quoted 15 lines · Jens Axboe

Quoted 5 lines · Pavel Begunkov

Quoted 4 lines · Jens Axboe

Quoted 3 lines

I think we can do the same thing, just wrapping the waitqueue in a structure with a count in it, on the stack. Got some flight time coming up later today, let me try and cook up a patch.

Quoted 8 lines

Quoted 2 lines

Totally untested, and sent out 5 min before departure... But something like this.

Hmm, reminds me my first version. Basically that's the same thing but with macroses inlined. I wanted to make it reusable and self-contained, though.

If you don't think it could be useful in other places, sure, we could do something like that. Is that so?

Quoted 7 lines

I totally agree it could be useful in other places. Maybe formalized and used with wake_up_nr() instead of adding a new primitive? Haven't looked into that, I may be talking nonsense.

In any case, I did get a chance to test it and it works for me. Here's the "finished" version, slightly cleaned up and with a comment added for good measure.

Notes:

This version gets the ordering right, you need exclusive waits to get fifo ordering on the waitqueue.

Both versions (yours and mine) suffer from the problem of potentially waking too many. I don't think this is a real issue, as generally we don't do threaded access to the io_urings. But if you had the following tasks wait on the cqring:

[min_events = 32], [min_events = 8], [min_events = 8]

and we reach the io_cqring_events() == threshold, we'll wake all three. I don't see a good solution to this, so I suspect we just live with until proven an issue. Both versions are much better than what we have now.

Quoted 2 lines

Forgot an issue around signal handling, version below adds the right check for that too.

It seems to be a good reason to not keep reimplementing "prepare_to_wait*() + wait loop" every time, but keep it in sched :)

Quoted 1 line

Curious what your test case was for this?

You mean a performance test case? It's briefly described in a comment for the second patch. That's just rewritten io_uring-bench, with
1. a thread generating 1 request per call in a loop
2. and the second thread waiting for ~128 events.
Both are pinned to the same core.

fs/io_uring.c +16 -0

+	unsigned to_wait;
+	unsigned nr_timeouts;
+};
+
+static inline bool io_should_wake(struct io_wait_queue *iowq)
+{
+	struct io_ring_ctx *ctx = iowq->ctx;
+
+	/*
+	 * Wake up if we have enough events, or if a timeout occured since we
+	 * started waiting. For timeouts, we always want to return to userspace,
+	 * regardless of event count.
+	 */
+	return io_cqring_events(ctx->rings) >= iowq->to_wait ||
+			atomic_read(&ctx->cq_timeouts) != iowq->nr_timeouts;
+}

fs/io_uring.c static int io_ring_submit(struct io_ring_ctx *ctx, unsigned int to_submit, · line 2774

Quoted 4 lines · Jens Axboe

fs/io_uring.c +4 -0

+struct io_wait_queue {
+	struct wait_queue_entry wq;
+	struct io_ring_ctx *ctx;
+	struct task_struct *task;

wq.private is where the normal waitqueue stores the task pointer.

(I'm going to rename that)

Quoted 16 lines

fs/io_uring.c +16 -0

+	unsigned to_wait;
+	unsigned nr_timeouts;
+};
+
+static inline bool io_should_wake(struct io_wait_queue *iowq)
+{
+	struct io_ring_ctx *ctx = iowq->ctx;
+
+	/*
+	 * Wake up if we have enough events, or if a timeout occured since we
+	 * started waiting. For timeouts, we always want to return to userspace,
+	 * regardless of event count.
+	 */
+	return io_cqring_events(ctx->rings) >= iowq->to_wait ||
+			atomic_read(&ctx->cq_timeouts) != iowq->nr_timeouts;
+}

fs/io_uring.c static int io_ring_submit(struct io_ring_ctx *ctx, unsigned int to_submit, · line 2800

Quoted 10 lines

fs/io_uring.c +10 -0

+
+static int io_wake_function(struct wait_queue_entry *curr, unsigned int mode,
+			    int wake_flags, void *key)
+{
+	struct io_wait_queue *iowq = container_of(curr, struct io_wait_queue,
+							wq);
+
+	if (io_should_wake(iowq)) {
+		list_del_init(&curr->entry);
+		wake_up_process(iowq->task);

Then you can use autoremove_wake_function() here.

fs/io_uring.c static int io_ring_submit(struct io_ring_ctx *ctx, unsigned int to_submit, · line 2805

Quoted 5 lines

fs/io_uring.c +5 -0

+		return 1;
+	}
+
+	return -1;
+}

Ideally we'd get wait_event()'s @cond in a custom wake function. Then we can _always_ do this.

This is one I'd love to have lambda functions for. It would actually work with GCC nested functions, because the wake function will always be in scope, but we can't use those in the kernel for other reasons :/

Quoted 9 lines · Peter Zijlstra

fs/io_uring.c static int io_ring_submit(struct io_ring_ctx *ctx, unsigned int to_submit, · line 2774

Quoted 4 lines · Jens Axboe

fs/io_uring.c +4 -0

+struct io_wait_queue {
+	struct wait_queue_entry wq;
+	struct io_ring_ctx *ctx;
+	struct task_struct *task;

wq.private is where the normal waitqueue stores the task pointer.

Quoted 1 line

(I'm going to rename that)

If you do that, then we can just base the io_uring parts on that.

Quoted 10 lines · Jens Axboe

Quoted 9 lines · Peter Zijlstra

Review

Quoted 4 lines · Jens Axboe

(quoted diff) +4 -0

+struct io_wait_queue {
+	struct wait_queue_entry wq;
+	struct io_ring_ctx *ctx;
+	struct task_struct *task;

wq.private is where the normal waitqueue stores the task pointer.

Quoted 3 lines

Quoted 1 line

(I'm going to rename that)

If you do that, then we can just base the io_uring parts on that.

Just took a quick look at it, and ran into block/kyber-iosched.c that actually uses the private pointer for something that isn't a task struct...

Quoted 13 lines · Jens Axboe

Quoted 12 lines · Jens Axboe

Quoted 10 lines · Peter Zijlstra

fs/io_uring.c static int io_ring_submit(struct io_ring_ctx *ctx, unsigned int to_submit,

Quoted 4 lines · Jens Axboe

fs/io_uring.c +4 -0

+struct io_wait_queue {
+	struct wait_queue_entry wq;
+	struct io_ring_ctx *ctx;
+	struct task_struct *task;

wq.private is where the normal waitqueue stores the task pointer.

(I'm going to rename that)

Quoted 6 lines

Quoted 1 line

If you do that, then we can just base the io_uring parts on that.

Just took a quick look at it, and ran into block/kyber-iosched.c that actually uses the private pointer for something that isn't a task struct...

Let reuse autoremove_wake_function anyway. Changed a bit your patch:

Quoted 21 lines · Pavel Begunkov

Quoted 19 lines · Jens Axboe

Quoted 10 lines · Jens Axboe

Quoted 9 lines · Peter Zijlstra

fs/io_uring.c static int io_ring_submit(struct io_ring_ctx *ctx, unsigned int to_submit,

Quoted 4 lines · Jens Axboe

fs/io_uring.c +4 -0

+struct io_wait_queue {
+	struct wait_queue_entry wq;
+	struct io_ring_ctx *ctx;
+	struct task_struct *task;

wq.private is where the normal waitqueue stores the task pointer.

Quoted 3 lines

Quoted 1 line

(I'm going to rename that)

If you do that, then we can just base the io_uring parts on that.

Just took a quick look at it, and ran into block/kyber-iosched.c that actually uses the private pointer for something that isn't a task struct...

Quoted 1 line

Let reuse autoremove_wake_function anyway. Changed a bit your patch:

Yep that should do it, and saves 8 bytes of stack as well.

BTW, did you test my patch, this one or the previous? Just curious if it worked for you.

Quoted 5 lines

Yep that should do it, and saves 8 bytes of stack as well.

BTW, did you test my patch, this one or the previous? Just curious if it worked for you.

Not yet, going to do that tonight

Quoted 6 lines · Pavel Begunkov

Quoted 5 lines

Yep that should do it, and saves 8 bytes of stack as well.

BTW, did you test my patch, this one or the previous? Just curious if it worked for you.

Not yet, going to do that tonight

Thanks! For reference, the final version is below. There was still a signal mishap in there, now it should all be correct afaict.

fs/io_uring.c +18 -0

index 9b84232e5cc4..d2a86164d520 100644
@@ -2768,6 +2768,38 @@ static int io_ring_submit(struct io_ring_ctx *ctx, unsigned int to_submit,
 	return submit;
 }
 
+struct io_wait_queue {
+	struct wait_queue_entry wq;
+	struct io_ring_ctx *ctx;
+	unsigned to_wait;
+	unsigned nr_timeouts;
+};
+
+static inline bool io_should_wake(struct io_wait_queue *iowq)
+{
+	struct io_ring_ctx *ctx = iowq->ctx;
+
+	/*
+	 * Wake up if we have enough events, or if a timeout occured since we
+	 * started waiting. For timeouts, we always want to return to userspace,
+	 * regardless of event count.
+	 */
+	return io_cqring_events(ctx->rings) >= iowq->to_wait ||
+			atomic_read(&ctx->cq_timeouts) != iowq->nr_timeouts;

fs/io_uring.c +27 -10

+
+	return autoremove_wake_function(curr, mode, wake_flags, key);
+}
+
 /*
  * Wait until events become available, if we don't already have some. The
  * application must reap them itself, as they reside on the shared cq ring.

@@ -2775,8 +2807,16 @@ static int io_ring_submit(struct io_ring_ctx *ctx, unsigned int to_submit,
 static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events,
 			  const sigset_t __user *sig, size_t sigsz)
 {
+	struct io_wait_queue iowq = {
+		.wq = {
+			.private	= current,
+			.func		= io_wake_function,
+			.entry		= LIST_HEAD_INIT(iowq.wq.entry),
+		},
+		.ctx		= ctx,
+		.to_wait	= min_events,
+	};
 	struct io_rings *rings = ctx->rings;
-	unsigned nr_timeouts;
 	int ret;
 
 	if (io_cqring_events(rings) >= min_events)

@@ -2795,15 +2835,20 @@ static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events,
 			return ret;
 	}
 
-	nr_timeouts = atomic_read(&ctx->cq_timeouts);
-	/*
-	 * Return if we have enough events, or if a timeout occured since
-	 * we started waiting. For timeouts, we always want to return to
-	 * userspace.
-	 */
-	ret = wait_event_interruptible(ctx->wait,
-				io_cqring_events(rings) >= min_events ||
-				atomic_read(&ctx->cq_timeouts) != nr_timeouts);
+	iowq.nr_timeouts = atomic_read(&ctx->cq_timeouts);
+	do {
+		prepare_to_wait_exclusive(&ctx->wait, &iowq.wq,
+						TASK_INTERRUPTIBLE);
+		if (io_should_wake(&iowq))
+			break;
+		schedule();
+		if (signal_pending(current)) {
+			ret = -ERESTARTSYS;
+			break;
+		}
+	} while (1);
+	finish_wait(&ctx->wait, &iowq.wq);
+
 	restore_saved_sigmask_unless(ret == -ERESTARTSYS);
 	if (ret == -ERESTARTSYS)
 		ret = -EINTR;

Quoted 45 lines · Jens Axboe

Quoted 6 lines · Pavel Begunkov

Quoted 5 lines

Yep that should do it, and saves 8 bytes of stack as well.

BTW, did you test my patch, this one or the previous? Just curious if it worked for you.

Not yet, going to do that tonight

Thanks! For reference, the final version is below. There was still a signal mishap in there, now it should all be correct afaict.

fs/io_uring.c +25 -0

index 9b84232e5cc4..d2a86164d520 100644
@@ -2768,6 +2768,38 @@ static int io_ring_submit(struct io_ring_ctx *ctx, unsigned int to_submit,
  	return submit;
  }
 
+struct io_wait_queue {
+	struct wait_queue_entry wq;
+	struct io_ring_ctx *ctx;
+	unsigned to_wait;
+	unsigned nr_timeouts;
+};
+
+static inline bool io_should_wake(struct io_wait_queue *iowq)
+{
+	struct io_ring_ctx *ctx = iowq->ctx;
+
+	/*
+	 * Wake up if we have enough events, or if a timeout occured since we
+	 * started waiting. For timeouts, we always want to return to userspace,
+	 * regardless of event count.
+	 */
+	return io_cqring_events(ctx->rings) >= iowq->to_wait ||
+			atomic_read(&ctx->cq_timeouts) != iowq->nr_timeouts;
+}
+
+static int io_wake_function(struct wait_queue_entry *curr, unsigned int mode,
+			    int wake_flags, void *key)
+{
+	struct io_wait_queue *iowq = container_of(curr, struct io_wait_queue,
+							wq);

fs/io_uring.c static int io_ring_submit(struct io_ring_ctx *ctx, unsigned int to_submit, · line 2798

Quoted 3 lines

fs/io_uring.c +3 -0

+
+	if (!io_should_wake(iowq))
+		return -1;

It would try to schedule only the first task in the wait list. Is that the semantic you want?
E.g. for waiters=[32,8] and nr_events == 8, io_wake_function() returns after @32, and won't wake up the second one.

Quoted 46 lines · Pavel Begunkov

Quoted 45 lines · Jens Axboe

Quoted 6 lines · Pavel Begunkov

Quoted 5 lines

Yep that should do it, and saves 8 bytes of stack as well.

BTW, did you test my patch, this one or the previous? Just curious if it worked for you.

Not yet, going to do that tonight

Thanks! For reference, the final version is below. There was still a signal mishap in there, now it should all be correct afaict.

fs/io_uring.c +25 -0

index 9b84232e5cc4..d2a86164d520 100644
@@ -2768,6 +2768,38 @@ static int io_ring_submit(struct io_ring_ctx *ctx, unsigned int to_submit,
  	return submit;
  }
 
+struct io_wait_queue {
+	struct wait_queue_entry wq;
+	struct io_ring_ctx *ctx;
+	unsigned to_wait;
+	unsigned nr_timeouts;
+};
+
+static inline bool io_should_wake(struct io_wait_queue *iowq)
+{
+	struct io_ring_ctx *ctx = iowq->ctx;
+
+	/*
+	 * Wake up if we have enough events, or if a timeout occured since we
+	 * started waiting. For timeouts, we always want to return to userspace,
+	 * regardless of event count.
+	 */
+	return io_cqring_events(ctx->rings) >= iowq->to_wait ||
+			atomic_read(&ctx->cq_timeouts) != iowq->nr_timeouts;
+}
+
+static int io_wake_function(struct wait_queue_entry *curr, unsigned int mode,
+			    int wake_flags, void *key)
+{
+	struct io_wait_queue *iowq = container_of(curr, struct io_wait_queue,
+							wq);

Quoted 8 lines

fs/io_uring.c static int io_ring_submit(struct io_ring_ctx *ctx, unsigned int to_submit, · line 2798

Quoted 3 lines

fs/io_uring.c +3 -0

+
+	if (!io_should_wake(iowq))
+		return -1;

It would try to schedule only the first task in the wait list. Is that the semantic you want?
E.g. for waiters=[32,8] and nr_events == 8, io_wake_function() returns after @32, and won't wake up the second one.

Right, those are the semantics I want. We keep the list ordered by using the exclusive wait addition. Which means that for the case you list, waiters=32 came first, and we should not wake others before that task gets the completions it wants. Otherwise we could potentially starve higher count waiters, if we always keep going and new waiters come in.

Quoted 56 lines · Jens Axboe

Quoted 46 lines · Pavel Begunkov

Quoted 45 lines · Jens Axboe

Quoted 6 lines · Pavel Begunkov

Quoted 5 lines

Yep that should do it, and saves 8 bytes of stack as well.

BTW, did you test my patch, this one or the previous? Just curious if it worked for you.

Not yet, going to do that tonight

Thanks! For reference, the final version is below. There was still a signal mishap in there, now it should all be correct afaict.

fs/io_uring.c +25 -0

index 9b84232e5cc4..d2a86164d520 100644
@@ -2768,6 +2768,38 @@ static int io_ring_submit(struct io_ring_ctx *ctx, unsigned int to_submit,
   	return submit;
   }
 
+struct io_wait_queue {
+	struct wait_queue_entry wq;
+	struct io_ring_ctx *ctx;
+	unsigned to_wait;
+	unsigned nr_timeouts;
+};
+
+static inline bool io_should_wake(struct io_wait_queue *iowq)
+{
+	struct io_ring_ctx *ctx = iowq->ctx;
+
+	/*
+	 * Wake up if we have enough events, or if a timeout occured since we
+	 * started waiting. For timeouts, we always want to return to userspace,
+	 * regardless of event count.
+	 */
+	return io_cqring_events(ctx->rings) >= iowq->to_wait ||
+			atomic_read(&ctx->cq_timeouts) != iowq->nr_timeouts;
+}
+
+static int io_wake_function(struct wait_queue_entry *curr, unsigned int mode,
+			    int wake_flags, void *key)
+{
+	struct io_wait_queue *iowq = container_of(curr, struct io_wait_queue,
+							wq);

Quoted 8 lines

fs/io_uring.c static int io_ring_submit(struct io_ring_ctx *ctx, unsigned int to_submit, · line 2798

Quoted 3 lines

fs/io_uring.c +3 -0

+
+	if (!io_should_wake(iowq))
+		return -1;

It would try to schedule only the first task in the wait list. Is that the semantic you want?
E.g. for waiters=[32,8] and nr_events == 8, io_wake_function() returns after @32, and won't wake up the second one.

Quoted 6 lines

Right, those are the semantics I want. We keep the list ordered by using the exclusive wait addition. Which means that for the case you list, waiters=32 came first, and we should not wake others before that task gets the completions it wants. Otherwise we could potentially starve higher count waiters, if we always keep going and new waiters come in.

Yes. I think It would better to be documented in userspace API. I could imagine some crazy case deadlocking userspace. E.g. thread 1: wait_events(8), reap_events
thread 2: wait_events(32), wait(thread 1), reap_events

works well

Reviewed-by Pavel Begunkov <[email protected]>

Tested-by Pavel Begunkov <[email protected]>

BTW, I searched for wait_event*(), and it seems there are plenty of similar use cases. So, generic case would be useful, but this is for later.

fs/io_uring.c +1 -0

@@ -2717,15 +2757,18 @@ static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events,
 			return ret;
 	}
 
+	iowq.nr_timeouts = atomic_read(&ctx->cq_timeouts);

Quoted 5 lines · Pavel Begunkov

fs/io_uring.c +1 -0

@@ -2717,15 +2757,18 @@ static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events,
  			return ret;
  	}
 
+	iowq.nr_timeouts = atomic_read(&ctx->cq_timeouts);

fs/io_uring.c static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events, · line 2770

Quoted 10 lines

fs/io_uring.c +10 -0

+	prepare_to_wait_exclusive(&ctx->wait, &iowq.wq, TASK_INTERRUPTIBLE);
+	do {
+		if (io_should_wake(&iowq))
+			break;
+		schedule();
+		if (signal_pending(current))
+			break;
+		set_current_state(TASK_INTERRUPTIBLE);
+	} while (1);
+	finish_wait(&ctx->wait, &iowq.wq);

It it likely OK, but for paranoia, I'd prefer this form:

	for (;;) {
		prepare_to_wait_exclusive(&ctx->wait, &iowq.wq, TASK_INTERRUPTIBLE);
		if (io_should_wake(&iowq))
			break;
		schedule();
		if (signal_pending(current))
			break;
	}
	finish_wait(&ctx->wait, &iowq.wq);

The thing is, if we ever succeed with io_wake_function() (that CPU observes io_should_wake()), but when waking here, we do not observe is_wake_function() and go sleep again, we might never wake up if we don't put ourselves back on the wait-list again.

Just took a quick look at it, and ran into block/kyber-iosched.c that actually uses the private pointer for something that isn't a task struct...

Quoted 15 lines · Jens Axboe

Quoted 5 lines · Pavel Begunkov

Quoted 4 lines · Jens Axboe

Quoted 3 lines

I think we can do the same thing, just wrapping the waitqueue in a structure with a count in it, on the stack. Got some flight time coming up later today, let me try and cook up a patch.

Quoted 8 lines

Quoted 2 lines

Totally untested, and sent out 5 min before departure... But something like this.

Hmm, reminds me my first version. Basically that's the same thing but with macroses inlined. I wanted to make it reusable and self-contained, though.

If you don't think it could be useful in other places, sure, we could do something like that. Is that so?

Quoted 7 lines

I totally agree it could be useful in other places. Maybe formalized and used with wake_up_nr() instead of adding a new primitive? Haven't looked into that, I may be talking nonsense.

In any case, I did get a chance to test it and it works for me. Here's the "finished" version, slightly cleaned up and with a comment added for good measure.

Notes:

This version gets the ordering right, you need exclusive waits to get fifo ordering on the waitqueue.

Both versions (yours and mine) suffer from the problem of potentially waking too many. I don't think this is a real issue, as generally we don't do threaded access to the io_urings. But if you had the following tasks wait on the cqring:

[min_events = 32], [min_events = 8], [min_events = 8]

and we reach the io_cqring_events() == threshold, we'll wake all three. I don't see a good solution to this, so I suspect we just live with until proven an issue. Both versions are much better than what we have now.

Quoted 36 lines · Jens Axboe

Quoted 15 lines · Jens Axboe

Quoted 5 lines · Pavel Begunkov

Quoted 4 lines · Jens Axboe

Quoted 3 lines

I think we can do the same thing, just wrapping the waitqueue in a structure with a count in it, on the stack. Got some flight time coming up later today, let me try and cook up a patch.

Quoted 8 lines

Quoted 2 lines

Totally untested, and sent out 5 min before departure... But something like this.

Hmm, reminds me my first version. Basically that's the same thing but with macroses inlined. I wanted to make it reusable and self-contained, though.

If you don't think it could be useful in other places, sure, we could do something like that. Is that so?

Quoted 7 lines

I totally agree it could be useful in other places. Maybe formalized and used with wake_up_nr() instead of adding a new primitive? Haven't looked into that, I may be talking nonsense.

In any case, I did get a chance to test it and it works for me. Here's the "finished" version, slightly cleaned up and with a comment added for good measure.

Notes:

This version gets the ordering right, you need exclusive waits to get fifo ordering on the waitqueue.

Both versions (yours and mine) suffer from the problem of potentially waking too many. I don't think this is a real issue, as generally we don't do threaded access to the io_urings. But if you had the following tasks wait on the cqring:

[min_events = 32], [min_events = 8], [min_events = 8]

Quoted 5 lines

and we reach the io_cqring_events() == threshold, we'll wake all three. I don't see a good solution to this, so I suspect we just live with until proven an issue. Both versions are much better than what we have now.

If io_cqring_events() == 8, only the last two would be woken up in both implementations, as to_wait/threshold is specified per waiter. Isn't it?

Agree with waiting, I don't see a good real-life case for that, that couldn't be done efficiently in userspace.

Quoted 11 lines · Pavel Begunkov

Quoted 6 lines · Jens Axboe

Quoted 3 lines

I think we can do the same thing, just wrapping the waitqueue in a structure with a count in it, on the stack. Got some flight time coming up later today, let me try and cook up a patch.

Totally untested, and sent out 5 min before departure... But something like this.

Hmm, reminds me my first version. Basically that's the same thing but with macroses inlined. I wanted to make it reusable and self-contained, though.

Quoted 2 lines

If you don't think it could be useful in other places, sure, we could do something like that. Is that so?

I totally agree it could be useful in other places. Maybe formalized and used with wake_up_nr() instead of adding a new primitive? Haven't looked into that, I may be talking nonsense.

In any case, I did get a chance to test it and it works for me. Here's the "finished" version, slightly cleaned up and with a comment added for good measure.

fs/io_uring.c +55 -10

index ca7570aca430..14fae454cf75 100644
@@ -2768,6 +2768,42 @@ static int io_ring_submit(struct io_ring_ctx *ctx, unsigned int to_submit,
 	return submit;
 }
 
+struct io_wait_queue {
+	struct wait_queue_entry wq;
+	struct io_ring_ctx *ctx;
+	struct task_struct *task;
+	unsigned to_wait;
+	unsigned nr_timeouts;
+};
+
+static inline bool io_should_wake(struct io_wait_queue *iowq)
+{
+	struct io_ring_ctx *ctx = iowq->ctx;
+
+	/*
+	 * Wake up if we have enough events, or if a timeout occured since we
+	 * started waiting. For timeouts, we always want to return to userspace,
+	 * regardless of event count.
+	 */
+	return io_cqring_events(ctx->rings) >= iowq->to_wait ||
+			atomic_read(&ctx->cq_timeouts) != iowq->nr_timeouts;
+}
+
+static int io_wake_function(struct wait_queue_entry *curr, unsigned int mode,
+			    int wake_flags, void *key)
+{
+	struct io_wait_queue *iowq = container_of(curr, struct io_wait_queue,
+							wq);
+
+	if (io_should_wake(iowq)) {
+		list_del_init(&curr->entry);
+		wake_up_process(iowq->task);
+		return 1;
+	}
+
+	return -1;
+}
+
 /*
  * Wait until events become available, if we don't already have some. The
  * application must reap them itself, as they reside on the shared cq ring.

@@ -2775,8 +2811,16 @@ static int io_ring_submit(struct io_ring_ctx *ctx, unsigned int to_submit,
 static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events,
 			  const sigset_t __user *sig, size_t sigsz)
 {
+	struct io_wait_queue iowq = {
+		.wq = {
+			.func	= io_wake_function,
+			.entry	= LIST_HEAD_INIT(iowq.wq.entry),
+		},
+		.task		= current,
+		.ctx		= ctx,
+		.to_wait	= min_events,
+	};
 	struct io_rings *rings = ctx->rings;
-	unsigned nr_timeouts;
 	int ret;
 
 	if (io_cqring_events(rings) >= min_events)

@@ -2795,15 +2839,16 @@ static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events,
 			return ret;
 	}
 
-	nr_timeouts = atomic_read(&ctx->cq_timeouts);
-	/*
-	 * Return if we have enough events, or if a timeout occured since
-	 * we started waiting. For timeouts, we always want to return to
-	 * userspace.
-	 */
-	ret = wait_event_interruptible(ctx->wait,
-				io_cqring_events(rings) >= min_events ||
-				atomic_read(&ctx->cq_timeouts) != nr_timeouts);
+	iowq.nr_timeouts = atomic_read(&ctx->cq_timeouts);
+	prepare_to_wait_exclusive(&ctx->wait, &iowq.wq, TASK_INTERRUPTIBLE);
+	do {
+		if (io_should_wake(&iowq))
+			break;
+		schedule();
+		set_current_state(TASK_INTERRUPTIBLE);
+	} while (1);
+	finish_wait(&ctx->wait, &iowq.wq);
+
 	restore_saved_sigmask_unless(ret == -ERESTARTSYS);
 	if (ret == -ERESTARTSYS)
 		ret = -EINTR;

Quoted 15 lines · Jens Axboe

Quoted 13 lines · Pavel Begunkov

Quoted 6 lines · Jens Axboe

Quoted 3 lines

I think we can do the same thing, just wrapping the waitqueue in a structure with a count in it, on the stack. Got some flight time coming up later today, let me try and cook up a patch.

Totally untested, and sent out 5 min before departure... But something like this.

Hmm, reminds me my first version. Basically that's the same thing but with macroses inlined. I wanted to make it reusable and self-contained, though.

If you don't think it could be useful in other places, sure, we could do something like that. Is that so?

Quoted 3 lines

I totally agree it could be useful in other places. Maybe formalized and used with wake_up_nr() instead of adding a new primitive? Haven't looked into that, I may be talking nonsense.

@nr there is about number of tasks to wake up. AFAIK doesn't solve the problem.