From: Jens Axboe <axboe@kernel.dk>
To: "jianchao.wang" <jianchao.w.wang@oracle.com>,
Anchal Agarwal <anchalag@amzn.com>
Cc: fllinden@amazon.com,
"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] blk-wbt: get back the missed wakeup from __wbt_done
Date: Mon, 27 Aug 2018 08:51:44 -0600 [thread overview]
Message-ID: <f88cd48d-e140-fa61-76f7-8985ca2c2e08@kernel.dk> (raw)
In-Reply-To: <3876c36c-fec6-6586-435f-dc177dc38c76@oracle.com>
On 8/27/18 12:15 AM, jianchao.wang wrote:
>
>
> On 08/27/2018 11:52 AM, jianchao.wang wrote:
>> Hi Jens
>>
>> On 08/25/2018 11:41 PM, Jens Axboe wrote:
>>> do {
>>> - set_current_state(TASK_UNINTERRUPTIBLE);
>>> + if (test_bit(0, &data.flags))
>>> + break;
>>>
>>> - if (!has_sleeper && rq_wait_inc_below(rqw, get_limit(rwb, rw)))
>>> + WARN_ON_ONCE(list_empty(&data.wq.entry));
>>> +
>>> + if (!has_sleeper &&
>>> + rq_wait_inc_below(rqw, get_limit(rwb, rw))) {
>>> + finish_wait(&rqw->wait, &data.wq);
>>> +
>>> + /*
>>> + * We raced with wbt_wake_function() getting a token,
>>> + * which means we now have two. Put ours and wake
>>> + * anyone else potentially waiting for one.
>>> + */
>>> + if (test_bit(0, &data.flags))
>>> + wbt_rqw_done(rwb, rqw, wb_acct);
>>> break;
>>
>> Just use 'bool' variable should be OK
>> After finish_wait, no one could race with us here.
>>
>>> + }
>>>
>>> if (lock) {
>>> spin_unlock_irq(lock);
>>> @@ -511,11 +569,11 @@ static void __wbt_wait(struct rq_wb *rwb, enum wbt_flags wb_acct,
>>> spin_lock_irq(lock);
>>> } else
>>> io_schedule();
>>> +
>>> has_sleeper = false;
>>> } while (1);
>>
>> I cannot get the point of "since we can't rely on just being woken from the ->func handler
>> we set".
>> Do you mean there could be someone else could wake up this task ?
Yeah, you don't know for a fact that the wbt wait queue is the only
guy waking us up. Any sleep like this needs a loop. It was quite
easy to reproduce for me, and as expected, you'll get list corruption
on the wait queue since we leave it on the list and the stack goes
away.
> If we do need a recheck after the io_schedule, we could do as following:
>
> static void __wbt_wait(struct rq_wb *rwb, enum wbt_flags wb_acct,
> unsigned long rw, spinlock_t *lock)
> __releases(lock)
> __acquires(lock)
> {
> struct rq_wait *rqw = get_rq_wait(rwb, wb_acct);
> struct wbt_wait_data data = {
> .wq = {
> .func = wbt_wake_function,
> .entry = LIST_HEAD_INIT(data.wq.entry),
> },
> .curr = current,
> .rwb = rwb,
> .rqw = rqw,
> .rw = rw,
> };
> bool has_sleeper;
> bool got = false;
>
> retry:
> has_sleeper = wq_has_sleeper(&rqw->wait);
> if (!has_sleeper && rq_wait_inc_below(rqw, get_limit(rwb, rw)))
> return;
>
> prepare_to_wait_exclusive(&rqw->wait, &data.wq, TASK_UNINTERRUPTIBLE);
>
> if (!has_sleeper && rq_wait_inc_below(rqw, get_limit(rwb, rw))) {
> got = true;
> goto out;
> }
>
> if (lock) {
> spin_unlock_irq(lock);
> io_schedule();
> spin_lock_irq(lock);
> } else
> io_schedule();
>
> out:
> finish_wait(&rqw->wait, &data.wq);
>
> /*
> * We raced with wbt_wake_function() getting a token,
> * which means we now have two. Put ours and wake
> * anyone else potentially waiting for one.
> */
> if (data.got && got)
> wbt_rqw_done(rwb, rqw, wb_acct);
> else if (!data.got && !got)
> goto retry;
I think the other variant is cleaner and easier to read. This is just
a natural loop, I don't think we need to use goto's here.
FWIW, I split it into two patches, current version is here:
http://git.kernel.dk/cgit/linux-block/log/?h=for-linus
--
Jens Axboe
next prev parent reply other threads:[~2018-08-27 14:51 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-23 13:08 [PATCH] blk-wbt: get back the missed wakeup from __wbt_done Jianchao Wang
2018-08-23 15:37 ` Jens Axboe
2018-08-23 16:24 ` van der Linden, Frank
[not found] ` <20180823210144.GB5624@kaos-source-ops-60001.pdx1.amazon.com>
2018-08-23 23:03 ` Jens Axboe
2018-08-23 23:14 ` Jens Axboe
2018-08-24 5:55 ` jianchao.wang
2018-08-24 16:40 ` van der Linden, Frank
2018-08-24 16:44 ` Jens Axboe
[not found] ` <20180824181223.GA9049@kaos-source-ops-60001.pdx1.amazon.com>
2018-08-24 18:50 ` Jens Axboe
[not found] ` <20180824203305.GA4690@kaos-source-ops-60001.pdx1.amazon.com>
2018-08-24 20:41 ` Jens Axboe
2018-08-25 15:41 ` Jens Axboe
2018-08-27 3:52 ` jianchao.wang
2018-08-27 6:15 ` jianchao.wang
2018-08-27 14:51 ` Jens Axboe [this message]
2018-08-28 2:52 ` jianchao.wang
2018-08-27 15:37 ` Jens Axboe
2018-08-23 15:42 ` Jens Axboe
2018-08-24 2:06 ` jianchao.wang
2018-08-24 14:40 ` Jens Axboe
2018-08-24 14:58 ` Jens Axboe
2018-08-24 17:14 ` Eduardo Valentin
2018-08-24 17:17 ` 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 \
--in-reply-to=f88cd48d-e140-fa61-76f7-8985ca2c2e08@kernel.dk \
--to=axboe@kernel.dk \
--cc=anchalag@amzn.com \
--cc=fllinden@amazon.com \
--cc=jianchao.w.wang@oracle.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/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;
as well as URLs for NNTP newsgroup(s).