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@xxxxxxxxxx>
> > > > ---
> > > >
> > > > 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@xxxxxxxx>
SUSE Labs, CR
