On 1/28/19 8:09 AM, Christoph Hellwig wrote:
> On Wed, Jan 23, 2019 at 08:35:22AM -0700, Jens Axboe wrote:
>> 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.
>> 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.
>> 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.
>> + /*
>> + * 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.
>> 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.
>> + 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.
>> 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
