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.

Reply via email to