Re: [PATCH 09/13] io_uring: add submission polling

On 1/28/19 8:09 AM, Christoph Hellwig wrote:

On Wed, Jan 23, 2019 at 08:35:22AM -0700, Jens Axboe wrote:

quoted

Proof of concept.

Is that still true?

I guess I can remove it now, dates back to when it was initially just
a test. But it should be solid, I'll kill that part.

quoted

1) Maybe have smarter backoff. Busy loop for X time, then go to
   monitor/mwait, finally the schedule we have now after an idle
   second. Might not be worth the complexity.

2) Probably want the application to pass in the appropriate grace
   period, not hard code it at 1 second.

2) actually sounds really useful.  Should we look into it ASAP?

I think so. Question is what kind of granularity we need for this. I
think we can go pretty coarse and keep it in msec, using a short to
pass this in like we do for the thread CPU. That gives us 0..65535 msec,
which should be plenty of range.

quoted

 	struct {
 		/* CQ ring */

@@ -264,6 +267,9 @@ static void __io_cqring_add_event(struct io_ring_ctx *ctx, u64 ki_user_data,
 
 	if (waitqueue_active(&ctx->wait))
 		wake_up(&ctx->wait);
+	if ((ctx->flags & IORING_SETUP_SQPOLL) &&
+	    waitqueue_active(&ctx->sqo_wait))

waitqueue_active is really cheap and sqo_wait should not otherwise
by active.  Do we really need the flags check here?

Probably not, I'll kill it.

quoted

+			/*
+			 * Normal IO, just pretend everything completed.
+			 * We don't have to poll completions for that.
+			 */
+			if (ctx->flags & IORING_SETUP_IOPOLL) {
+				/*
+				 * App should not use IORING_ENTER_GETEVENTS
+				 * with thread polling, but if it does, then
+				 * ensure we are mutually exclusive.

Should we just return an error early on in this case instead?

I think that'd make it awkward, since it's out-of-line. If the app is doing
things it shouldn't in this case, its own io_uring_enter() would most likely
fail occasionally with -EBUSY anyway.

quoted

 	if (to_submit) {
+		if (ctx->flags & IORING_SETUP_SQPOLL) {
+			wake_up(&ctx->sqo_wait);
+			ret = to_submit;

Do these semantics really make sense?  Maybe we should have an
IORING_ENTER_WAKE_SQ instead of overloading the to_submit argument?
Especially as we don't really care about returning the number passed in.

I like that change, I'll add IORING_ENTER_SQ_WAKEUP instead of using
'to_submit' for this. We can't validate the number anyway.

quoted

+	if (ctx->sqo_thread) {
+		kthread_park(ctx->sqo_thread);

Can you explain why we need the whole kthread_park game?  It is only
intended to deal with pausing a thread, and if need it to shut down
a thread we have a bug somewhere.

It is working around a bug in shutting down a thread that is affinitized
to a single CPU, I just didn't want to deal with hunting that down right
now.

quoted

 static void io_sq_offload_stop(struct io_ring_ctx *ctx)
 {
+	if (ctx->sqo_thread) {
+		kthread_park(ctx->sqo_thread);
+		kthread_stop(ctx->sqo_thread);
+		ctx->sqo_thread = NULL;

Also there isn't really much of a point in setting pointers to NULL
just before freeing the containing structure.  In the best case this
now papers over bugs that poisoning or kasan would otherwise find.

Removed.

-- 
Jens Axboe