public inbox for [email protected]
 help / color / mirror / Atom feed
From: Pavel Begunkov <[email protected]>
To: Jens Axboe <[email protected]>, io-uring <[email protected]>
Subject: Re: [PATCH] io_uring: use task_work for links if possible
Date: Sat, 27 Jun 2020 13:57:04 +0300	[thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>

On 27/06/2020 04:45, Jens Axboe wrote:
> On 6/26/20 3:20 PM, Pavel Begunkov wrote:
>>>>> +		tsk = io_wq_get_task(req->ctx->io_wq);
>>>>> +		task_work_add(tsk, &req->task_work, true);
>>>>> +	}
>>>>> +	wake_up_process(tsk);
>>>>> +}
>>>>> +
>>>>>  static void io_free_req(struct io_kiocb *req)
>>>>>  {
>>>>>  	struct io_kiocb *nxt = NULL;
>>>>> @@ -1671,8 +1758,12 @@ static void io_free_req(struct io_kiocb *req)
>>>>>  	io_req_find_next(req, &nxt);
>>>>>  	__io_free_req(req);
>>>>>  
>>>>> -	if (nxt)
>>>>> -		io_queue_async_work(nxt);
>>>>> +	if (nxt) {
>>>>> +		if (nxt->flags & REQ_F_WORK_INITIALIZED)
>>>>> +			io_queue_async_work(nxt);
>>>>
>>>> Don't think it will work. E.g. io_close_prep() may have set
>>>> REQ_F_WORK_INITIALIZED but without io_req_work_grab_env().
>>>
>>> This really doesn't change the existing path, it just makes sure we
>>> don't do io_req_task_queue() on something that has already modified
>>> ->work (and hence, ->task_work). This might miss cases where we have
>>> only cleared it and done nothing else, but that just means we'll have
>>> cases that we could potentially improve the effiency of down the line.
>>
>> Before the patch it was always initialising linked reqs, and that would
>> work ok, if not this lazy grab_env().
>>
>> E.g. req1 -> close_req
>>
>> It calls, io_req_defer_prep(__close_req__, sqe, __false__)
>> which doesn't do grab_env() because of for_async=false,
>> but calls io_close_prep() which sets REQ_F_WORK_INITIALIZED.
>>
>> Then, after completion of req1 it will follow added lines
>>
>> if (nxt)
>> 	if (nxt->flags & REQ_F_WORK_INITIALIZED)
>> 		io_queue_async_work(nxt);
>>
>> Ending up in
>>
>> io_queue_async_work()
>> 	-> grab_env()
>>
>> And that's who knows from which context.
>> E.g. req1 was an rw completed in an irq.
> 
> Hmm yes, good point, that is a problem. I don't have a good immediate
> solution for this. Do you have any suggestions on how best to handle
> this?

I certainly don't want another REQ_F_GRABBED_ENV flag :)

From the start I was planning to move all grab_env() calls to
io_queue_async_work() just before we're doing punting. like

io_queue_async_work(req) {
	// simplified
	for_each_in_link(req)
		grab_env();
	...
}

If done right, this can solve a lot of problems and simplify
lifetime management. There are much more problems, I'll send
a patchset with quick fixes, and then we can do it right
without hurry.

> 
>> Not sure it's related, but fallocate shows the log below, and some
>> other tests hang the kernel as well.
> 
> Yeah, that's indeed that very thing.

Turns out it's not.

> 
>>> True, that could be false instead.
>>>
>>> Since these are just minor things, we can do a fix on top. I don't want
>>> to reshuffle this unless I have to.
>>
>> Agree, I have a pile on top myself.
> 
> Fire away :-)

I prefer to have a working branch first.


-- 
Pavel Begunkov

      reply	other threads:[~2020-06-27 10:58 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-25 18:27 [PATCH] io_uring: use task_work for links if possible Jens Axboe
2020-06-25 20:28 ` Pavel Begunkov
2020-06-25 21:37   ` Jens Axboe
2020-06-26  9:41     ` Pavel Begunkov
2020-06-26 20:27 ` Pavel Begunkov
2020-06-26 20:43   ` Jens Axboe
2020-06-26 21:20     ` Pavel Begunkov
2020-06-27  1:45       ` Jens Axboe
2020-06-27 10:57         ` Pavel Begunkov [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox