From: Jan Kara <jack@suse.cz>
To: Hugh Dickins <hughd@google.com>
Cc: Keith Busch <kbusch@kernel.org>, Jens Axboe <axboe@kernel.dk>,
Yu Kuai <yukuai1@huaweicloud.com>, Jan Kara <jack@suse.cz>,
Liu Song <liusong@linux.alibaba.com>,
linux-block@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH next] sbitmap: fix lockup while swapping
Date: Fri, 23 Sep 2022 16:43:03 +0200 [thread overview]
Message-ID: <20220923144303.fywkmgnkg6eken4x@quack3> (raw)
In-Reply-To: <20220921164012.s7lvklp2qk6occcg@quack3>
On Wed 21-09-22 18:40:12, Jan Kara wrote:
> On Mon 19-09-22 16:01:39, Hugh Dickins wrote:
> > On Mon, 19 Sep 2022, Keith Busch wrote:
> > > On Sun, Sep 18, 2022 at 02:10:51PM -0700, Hugh Dickins wrote:
> > > > I have almost no grasp of all the possible sbitmap races, and their
> > > > consequences: but using the same !waitqueue_active() check as used
> > > > elsewhere, fixes the lockup and shows no adverse consequence for me.
> > >
> > >
> > > > Fixes: 4acb83417cad ("sbitmap: fix batched wait_cnt accounting")
> > > > Signed-off-by: Hugh Dickins <hughd@google.com>
> > > > ---
> > > >
> > > > lib/sbitmap.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > --- a/lib/sbitmap.c
> > > > +++ b/lib/sbitmap.c
> > > > @@ -620,7 +620,7 @@ static bool __sbq_wake_up(struct sbitmap
> > > > * function again to wakeup a new batch on a different 'ws'.
> > > > */
> > > > if (cur == 0)
> > > > - return true;
> > > > + return !waitqueue_active(&ws->wait);
> > >
> > > If it's 0, that is supposed to mean another thread is about to make it not zero
> > > as well as increment the wakestate index. That should be happening after patch
> > > 48c033314f37 was included, at least.
> >
> > I believe that the thread about to make wait_cnt not zero (and increment the
> > wakestate index) is precisely this interrupted thread: the backtrace shows
> > that it had just done its wakeups, so has not yet reached making wait_cnt
> > not zero; and I suppose that either its wakeups did not empty the waitqueue
> > completely, or another waiter got added as soon as it dropped the spinlock.
I was trying to wrap my head around this but I am failing to see how we
could have wait_cnt == 0 for long enough to cause any kind of stall let
alone a lockup in sbitmap_queue_wake_up() as you describe. I can understand
we have:
CPU1 CPU2
sbitmap_queue_wake_up()
ws = sbq_wake_ptr(sbq);
cur = atomic_read(&ws->wait_cnt);
do {
...
wait_cnt = cur - sub; /* this will be 0 */
} while (!atomic_try_cmpxchg(&ws->wait_cnt, &cur, wait_cnt));
...
/* Gets the same waitqueue */
ws = sbq_wake_ptr(sbq);
cur = atomic_read(&ws->wait_cnt);
do {
if (cur == 0)
return true; /* loop */
wake_up_nr(&ws->wait, wake_batch);
smp_mb__before_atomic();
sbq_index_atomic_inc(&sbq->wake_index);
atomic_set(&ws->wait_cnt, wake_batch); /* This stops looping on CPU2 */
So until CPU1 reaches the atomic_set(), CPU2 can be looping. But how come
this takes so long that is causes a hang as you describe? Hum... So either
CPU1 takes really long to get to atomic_set():
- can CPU1 get preempted? Likely not at least in the context you show in
your message
- can CPU1 spend so long in wake_up_nr()? Maybe the waitqueue lock is
contended but still...
or CPU2 somehow sees cur==0 for longer than it should. The whole sequence
executed in a loop on CPU2 does not contain anything that would force CPU2
to refresh its cache and get new ws->wait_cnt value so we are at the mercy
of CPU cache coherency mechanisms to stage the write on CPU1 and propagate
it to other CPUs. But still I would not expect that to take significantly
long. Any other ideas?
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
next prev parent reply other threads:[~2022-09-23 14:43 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-18 21:10 [PATCH next] sbitmap: fix lockup while swapping Hugh Dickins
2022-09-19 21:22 ` Keith Busch
2022-09-19 23:01 ` Hugh Dickins
2022-09-21 16:40 ` Jan Kara
2022-09-23 14:43 ` Jan Kara [this message]
2022-09-23 15:13 ` Keith Busch
2022-09-23 16:16 ` Hugh Dickins
2022-09-23 19:07 ` Keith Busch
2022-09-23 21:29 ` Hugh Dickins
2022-09-23 23:15 ` Hugh Dickins
2022-09-26 11:44 ` Jan Kara
2022-09-26 14:08 ` Yu Kuai
2022-09-27 3:39 ` Hugh Dickins
2022-09-27 10:31 ` Jan Kara
2022-09-28 3:56 ` Hugh Dickins
2022-09-28 3:59 ` [PATCH next v2] " Hugh Dickins
2022-09-28 4:07 ` Hugh Dickins
2022-09-29 8:39 ` Jan Kara
2022-09-29 19:50 ` [PATCH next v3] " Hugh Dickins
2022-09-29 19:56 ` Keith Busch
2022-09-29 23:58 ` Jens Axboe
[not found] ` <20220924023047.1410-1-hdanton@sina.com>
2022-09-27 4:02 ` [PATCH next] " Hugh Dickins
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=20220923144303.fywkmgnkg6eken4x@quack3 \
--to=jack@suse.cz \
--cc=axboe@kernel.dk \
--cc=hughd@google.com \
--cc=kbusch@kernel.org \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=liusong@linux.alibaba.com \
--cc=yukuai1@huaweicloud.com \
/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).