linux-kernel - Re: [PATCH] sbitmap: Use single per-bitmap counting to wake up queued tags

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20221115102447.tg3tx5dofu3qalhq@quack3>
Date:   Tue, 15 Nov 2022 11:24:47 +0100
From:   Jan Kara <jack@...e.cz>
To:     Gabriel Krisman Bertazi <krisman@...e.de>
Cc:     Jan Kara <jack@...e.cz>, axboe@...nel.dk,
        linux-kernel@...r.kernel.org, linux-block@...r.kernel.org,
        Hugh Dickins <hughd@...gle.com>,
        Keith Busch <kbusch@...nel.org>,
        Liu Song <liusong@...ux.alibaba.com>
Subject: Re: [PATCH] sbitmap: Use single per-bitmap counting to wake up
 queued tags

On Mon 14-11-22 22:52:32, Gabriel Krisman Bertazi wrote:
> Jan Kara <jack@...e.cz> writes:
> 
> > Now this may be also problematic - when we were checking the number of woken
> > waiters in the older version of the patch (for others: internal version of
> > the patch) this was fine but now it may happen that the 'ws' we have
> > selected has no waiters anymore. And in that case we need to find another
> > waitqueue because otherwise we'd be loosing too many wakeups and we could
> > deadlock. So I think this rather needs to be something like:
> >
> > 	do {
> > 		if (atomic_read(&sbq->completion_cnt) - wakeups < wake_batch)
> > 			return;
> > 	} while (!atomic_try_cmpxchg(&sbq->wakeup_cnt,
> > 				     &wakeups, wakeups + wake_batch));
> >
> > 	do {
> > 		ws = sbq_wake_ptr(sbq);
> > 		if (!ws)
> > 			return;
> 
> Does this really solve it? There is no guarantee there will be another
> waiter in the queues when we check here.  So, once again we could not
> wake up anyone and return it this if leg.  If that is the case, don't we
> end up overshooting wakeups and end up again with less completions than
> required to wake up an incoming io?

Well, if we don't find any waiter in any of the wait queues, it sure does
not matter we just discard wakeups? And sure, these checks are racy as the
waiters can be constantly added but the invariant is: if some waiter is
added after the atomic_try_cmpxchg(), then all tags are used so as many
completions as there are tags are coming. So that is enough to wake the
new waiter (due to batch size). So all what matters is: If there's any
waiter in the waitqueue by the time atomic_try_cmpxchg() is called, we'll
consider it in the wakeup loop. And that is fulfilled by the code AFAICT.

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR