On 5/16/20 3:23 AM, Xiaoguang Wang wrote: > hi, > >> hi, >> >>> On 5/13/20 6:37 AM, Xiaoguang Wang wrote: >>>> When IORING_SETUP_SQPOLL is enabled, io_ring_ctx_wait_and_kill() will wait >>>> for sq thread to idle by busy loop: >>>> while (ctx->sqo_thread && !wq_has_sleeper(&ctx->sqo_wait)) >>>> cond_resched(); >>>> Above codes are not friendly, indeed I think this busy loop will introduce a >>>> cpu burst in current cpu, though it maybe short. >>>> >>>> In this patch, if ctx->refs is dying, we forbids sq_thread from submitting >>>> sqes anymore, just discard leftover sqes. >>> >>> I don't think this really changes anything. What happens if: >>> >>>> @@ -6051,7 +6053,8 @@ static int io_sq_thread(void *data) >>>> } >>>> mutex_lock(&ctx->uring_lock); >>>> - ret = io_submit_sqes(ctx, to_submit, NULL, -1, true); >>>> + if (likely(!percpu_ref_is_dying(&ctx->refs))) >>>> + ret = io_submit_sqes(ctx, to_submit, NULL, -1, true); >>>> mutex_unlock(&ctx->uring_lock); >>>> timeout = jiffies + ctx->sq_thread_idle; >>> >>> You check for dying here, but that could change basically while you're >>> checking it. So you're still submitting sqes with a ref that's going >>> away. You've only reduced the window, you haven't eliminated it. >> Look at codes, we call percpu_ref_kill() under uring_lock, so isn't it safe >> to check the refs' dying status? Thanks. > Cloud you please have a look at my explanation again? Thanks. Sorry for the delay - you are right, we only kill it inside the ring mutex, so should be safe to check. Can we get by with just the very last check instead of checking all of the cases? -- Jens Axboe
