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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jan 23, 2019 at 08:35:22AM -0700, Jens Axboe wrote:
> Proof of concept.

Is that still true?

> 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?

>  
>  	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?

> +			/*
> +			 * 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?

>  	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.

> +	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.

>  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.



[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux