Re: [PATCH 1/2] io_uring: refactor io_sq_thread() handling

[Jens Axboe <axboe@kernel.dk>]

On 10/20/20 2:23 AM, Xiaoguang Wang wrote:
> @@ -6638,24 +6578,52 @@ static int io_sq_thread(void *data)
>  			}
>  			io_sq_thread_associate_blkcg(ctx, &cur_css);
>  
> -			ret |= __io_sq_thread(ctx, start_jiffies, cap_entries);
> +			ret = __io_sq_thread(ctx, cap_entries);
> +			if (!sqt_spin && (ret > 0 || !list_empty(&ctx->iopoll_list)))
> +				sqt_spin = true;
> +		}
>  
> -			io_sq_thread_drop_mm();
> +		if (sqt_spin) {
> +			io_run_task_work();
> +			cond_resched();
> +			continue;
>  		}
>  
> -		if (ret & SQT_SPIN) {
> +		if (!time_after(jiffies, timeout)) {
>  			io_run_task_work();
>  			cond_resched();
> -		} else if (ret == SQT_IDLE) {
> -			if (kthread_should_park())
> -				continue;
> +			io_sq_thread_drop_mm();
> +			continue;
> +		}
> +
> +		needs_sched = true;
> +		prepare_to_wait(&sqd->wait, &wait, TASK_INTERRUPTIBLE);
> +		list_for_each_entry(ctx, &sqd->ctx_list, sqd_list) {
> +			if ((ctx->flags & IORING_SETUP_IOPOLL) &&
> +			    !list_empty_careful(&ctx->iopoll_list)) {
> +				needs_sched = false;
> +				break;
> +			}
> +			to_submit = io_sqring_entries(ctx);
> +			if (to_submit) {
> +				needs_sched = false;
> +				break;
> +			}
> +		}
> +
> +		if (needs_sched) {
>  			list_for_each_entry(ctx, &sqd->ctx_list, sqd_list)
>  				io_ring_set_wakeup_flag(ctx);
> +		}
> +
> +		if (needs_sched) {
>  			schedule();
> -			start_jiffies = jiffies;
>  			list_for_each_entry(ctx, &sqd->ctx_list, sqd_list)
>  				io_ring_clear_wakeup_flag(ctx);
> -		}
> +			finish_wait(&sqd->wait, &wait);
> +		} else
> +			finish_wait(&sqd->wait, &wait);
> +		timeout = jiffies + sqd->sq_thread_idle;
>  	}

Not sure why, but you have two

if (needs_sched) {
}

right after each other, that should be folded into one.

And I'd get rid of the to_submit, just do:

if (io_sqring_entries(ctx)) {
	needs_sched = false;
	break;
}

as we're not going to be using that value anyway.

As a minor style thing, always includes bracket on both cases if one has
it, don't do this:

if (foo) {
	...
	...
} else
	bar();


-- 
Jens Axboe