public inbox for [email protected]
 help / color / mirror / Atom feed
From: Jens Axboe <[email protected]>
To: Pavel Begunkov <[email protected]>,
	io-uring <[email protected]>
Subject: Re: [PATCH v2] io-wq: handle hashed writes in chains
Date: Sun, 22 Mar 2020 11:08:36 -0600	[thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>

On 3/22/20 10:09 AM, Pavel Begunkov wrote:
>>  		/* not hashed, can run anytime */
>>  		if (!io_wq_is_hashed(work)) {
>> -			wq_node_del(&wqe->work_list, node, prev);
>> -			return work;
>> +			/* already have hashed work, let new worker get this */
>> +			if (ret) {
>> +				struct io_wqe_acct *acct;
>> +
>> +				/* get new worker for unhashed, if none now */
>> +				acct = io_work_get_acct(wqe, work);
>> +				if (!atomic_read(&acct->nr_running))
>> +					io_wqe_wake_worker(wqe, acct);
>> +				break;
>> +			}
>> +			wq_node_del(&wqe->work_list, &work->list);
>> +			ret = work;
>> +			break;
>>  		}
>>  
>>  		/* hashed, can run if not already running */
>> -		hash = io_get_work_hash(work);
>> -		if (!(wqe->hash_map & BIT(hash))) {
>> +		new_hash = io_get_work_hash(work);
>> +		if (wqe->hash_map & BIT(new_hash))
>> +			break;
> 
> This will always break for subsequent hashed, as the @hash_map bit is set.
> Isn't it? And anyway, it seems it doesn't optimise not-contiguous same-hashed
> requests, e.g.
> 
> 0: Link(hash=0)
> 1: Link(hash=1)
> 2: Link(hash=0)
> 3: Link(not_hashed)
> 4: Link(hash=0)
> ...

Yeah, I think I shuffled a bit too much there, should only break on that
if hash != new_hash. Will fix!

>> @@ -530,6 +565,24 @@ static void io_worker_handle_work(struct io_worker *worker)
>>  				work = NULL;
>>  			}
>>  			if (hash != -1U) {
>> +				/*
>> +				 * If the local list is non-empty, then we
>> +				 * have work that hashed to the same key.
>> +				 * No need for a lock round-trip, or fiddling
>> +				 * the the free/busy state of the worker, or
>> +				 * clearing the hashed state. Just process the
>> +				 * next one.
>> +				 */
>> +				if (!work) {
>> +					work = wq_first_entry(&list,
>> +							      struct io_wq_work,
>> +							      list);
> 
> Wouldn't it just drop a linked request? Probably works because of the
> comment above.

Only drops if if we always override 'work', if it's already set we use
'work' and don't grab from the list. So that should work for links.

>> +					if (work) {
>> +						wq_node_del(&list, &work->list);
> 
> There is a bug, apparently from one of my commits, where it do
> io_assign_current_work() but then re-enqueue and reassign new work,
> though there is a gap for cancel to happen, which would screw
> everything up.
> 
> I'll send a patch, so it'd be more clear. However, this is a good
> point to look after for this as well.

Saw it, I'll apply when you have the Fixes line. I'll respin this one
after.

-- 
Jens Axboe


  parent reply	other threads:[~2020-03-22 17:08 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-19 18:56 [PATCH v2] io-wq: handle hashed writes in chains Jens Axboe
2020-03-22 16:09 ` Pavel Begunkov
2020-03-22 16:24   ` Pavel Begunkov
2020-03-22 17:08     ` Jens Axboe
2020-03-22 18:54     ` Pavel Begunkov
2020-03-22 19:51       ` Jens Axboe
2020-03-22 20:05         ` Jens Axboe
2020-03-22 20:15           ` Jens Axboe
2020-03-22 20:20             ` Pavel Begunkov
2020-03-22 21:16               ` Pavel Begunkov
2020-03-22 21:31                 ` Pavel Begunkov
2020-03-22 20:25         ` Pavel Begunkov
2020-03-23  1:37           ` Jens Axboe
2020-03-23  8:38             ` Pavel Begunkov
2020-03-23 14:26               ` Jens Axboe
2020-03-22 17:08   ` Jens Axboe [this message]
2020-03-22 17:37     ` Pavel Begunkov
2020-03-22 20:56 ` Pavel Begunkov
  -- strict thread matches above, loose matches on Subject: below --
2020-03-23 19:57 Pavel Begunkov
2020-03-24  2:31 ` 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] \
    /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