public inbox for [email protected]
 help / color / mirror / Atom feed
From: Pavel Begunkov <[email protected]>
To: Jens Axboe <[email protected]>,
	[email protected], [email protected]
Subject: Re: [PATCH v2 4/4] io_uring: get next req on subm ref drop
Date: Tue, 3 Mar 2020 13:46:06 +0300	[thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>

On 3/3/2020 9:54 AM, Pavel Begunkov wrote:
> On 03/03/2020 07:26, Jens Axboe wrote:
>> On 3/2/20 1:45 PM, Pavel Begunkov wrote:
>>> Get next request when dropping the submission reference. However, if
>>> there is an asynchronous counterpart (i.e. read/write, timeout, etc),
>>> that would be dangerous to do, so ignore them using new
>>> REQ_F_DONT_STEAL_NEXT flag.
>>
>> Hmm, not so sure I like this one. It's not quite clear to me where we
>> need REQ_F_DONT_STEAL_NEXT. If we have an async component, then we set
>> REQ_F_DONT_STEAL_NEXT. So this is generally the case where our
>> io_put_req() for submit is not the last drop. And for the other case,
>> the put is generally in the caller anyway. So I don't really see what
>> this extra flag buys us?
> 
> Because io_put_work() holds a reference, no async handler can achive req->refs
> == 0, so it won't return next upon dropping the submission ref (i.e. by
> put_find_nxt()). And I want to have next before io_put_work(), to, instead of as
> currently:
> 
> run_work(work);
> assign_cur_work(NULL); // spinlock + unlock worker->lock
> new_work = put_work(work);
> assign_cur_work(new_work); // the second time
> 
> do:
> 
> new_work = run_work(work);
> assign_cur_work(new_work); // need new_work here
> put_work(work);
> 
> 
> The other way:
> 
> io_wq_submit_work() // for all async handlers
> {
> 	...
> 	// Drop submission reference.
> 	// One extra ref will be put in io_put_work() right
> 	// after return, and it'll be done in the same thread
> 	if (atomic_dec_and_get(req) == 1)
> 		steal_next(req);
> }
> 
> Maybe cleaner, but looks fragile as well. Would you prefer it?

Any chance you've measured your next-work fix? I wonder how much does it
hurt performance, and whether we need a terse patch for 5.6.


>> Few more comments below.
>>
>>> +static void io_put_req_async_submission(struct io_kiocb *req,
>>> +					struct io_wq_work **workptr)
>>> +{
>>> +	static struct io_kiocb *nxt;
>>> +
>>> +	nxt = io_put_req_submission(req);
>>> +	if (nxt)
>>> +		io_wq_assign_next(workptr, nxt);
>>> +}
>>
>> This really should be called io_put_req_async_completion() since it's
>> called on completion. The naming is confusing.
> 
> Ok
> 
>>> @@ -2581,14 +2598,11 @@ static void __io_fsync(struct io_kiocb *req)
>>>  static void io_fsync_finish(struct io_wq_work **workptr)
>>>  {
>>>  	struct io_kiocb *req = container_of(*workptr, struct io_kiocb, work);
>>> -	struct io_kiocb *nxt = NULL;
>>>  
>>>  	if (io_req_cancelled(req))
>>>  		return;
>>>  	__io_fsync(req);
>>> -	io_put_req(req); /* drop submission reference */
>>> -	if (nxt)
>>> -		io_wq_assign_next(workptr, nxt);
>>> +	io_put_req_async_submission(req, workptr);
>>>  }
>>>  
>>>  static int io_fsync(struct io_kiocb *req, bool force_nonblock)
>>> @@ -2617,14 +2631,11 @@ static void __io_fallocate(struct io_kiocb *req)
>>>  static void io_fallocate_finish(struct io_wq_work **workptr)
>>>  {
>>>  	struct io_kiocb *req = container_of(*workptr, struct io_kiocb, work);
>>> -	struct io_kiocb *nxt = NULL;
>>>  
>>>  	if (io_req_cancelled(req))
>>>  		return;
>>>  	__io_fallocate(req);
>>> -	io_put_req(req); /* drop submission reference */
>>> -	if (nxt)
>>> -		io_wq_assign_next(workptr, nxt);
>>> +	io_put_req_async_submission(req, workptr);
>>>  }
>>
>> All of these cleanups are nice (except the naming, as mentioned).
>>
>>> @@ -3943,7 +3947,10 @@ static int io_poll_add(struct io_kiocb *req)
>>>  	if (mask) {
>>>  		io_cqring_ev_posted(ctx);
>>>  		io_put_req(req);
>>> +	} else {
>>> +		req->flags |= REQ_F_DONT_STEAL_NEXT;
>>>  	}
>>> +
>>>  	return ipt.error;
>>>  }
>>
>> Is this racy? I guess it doesn't matter since we're still holding the
>> completion reference.
> 
> It's done by the same thread, that uses it. There could be a race if the async
> counterpart is going to change req->flags, but we tolerate false negative (i.e.
> put_req() will handle it).
> 

-- 
Pavel Begunkov

  reply	other threads:[~2020-03-03 10:46 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-02 20:45 [PATCH v2 0/4] nxt propagation Pavel Begunkov
2020-03-02 20:45 ` [PATCH v2 1/4] io_uring: clean up io_close Pavel Begunkov
2020-03-02 20:45 ` [PATCH v2 2/4] io_uring: make submission ref putting consistent Pavel Begunkov
2020-03-02 20:45 ` [PATCH v2 3/4] io_uring: remove @nxt from handlers Pavel Begunkov
2020-03-02 20:45 ` [PATCH v2 4/4] io_uring: get next req on subm ref drop Pavel Begunkov
2020-03-03  4:26   ` Jens Axboe
2020-03-03  6:54     ` Pavel Begunkov
2020-03-03 10:46       ` Pavel Begunkov [this message]
2020-03-03 16:04         ` Jens Axboe
2020-03-03 16:29           ` Jens Axboe
2020-03-03 16:03       ` Jens Axboe

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] \
    [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