On 12/1/22 12:21?AM, Kemeng Shi wrote: > > > on 12/1/2022 12:54 PM, Kemeng Shi wrote: >> If we decremented queue without waiters, we should not decremente freed >> bits number "nr", or all "nr" could be consumed in a empty queue and no >> wakeup will be called. >> Currently, for case "wait_cnt > 0", "nr" will not be decremented if we >> decremented queue without watiers and retry is returned to avoid lost >> wakeups. However for case "wait_cnt == 0", "nr" will be decremented >> unconditionally and maybe decremented to zero. Although retry is >> returned by active state of queue, it's not actually executed for "nr" >> is zero. >> >> Fix this by only decrementing "nr" for active queue when "wait_cnt == >> 0". After this fix, "nr" will always be non-zero when we decremented >> inactive queue for case "wait_cnt == 0", so the need to retry could >> be returned by "nr" and active state of waitqueue returned for the same >> purpose is not needed. >> >> Signed-off-by: Kemeng Shi <shikemeng@xxxxxxxxxx> >> --- >> lib/sbitmap.c | 13 ++++++------- >> 1 file changed, 6 insertions(+), 7 deletions(-) >> >> diff --git a/lib/sbitmap.c b/lib/sbitmap.c >> index 7280ae8ca88c..e40759bcf821 100644 >> --- a/lib/sbitmap.c >> +++ b/lib/sbitmap.c >> @@ -604,7 +604,6 @@ static bool __sbq_wake_up(struct sbitmap_queue *sbq, int *nr) >> struct sbq_wait_state *ws; >> unsigned int wake_batch; >> int wait_cnt, cur, sub; >> - bool ret; >> >> if (*nr <= 0) >> return false; >> @@ -632,15 +631,15 @@ static bool __sbq_wake_up(struct sbitmap_queue *sbq, int *nr) >> if (wait_cnt > 0) >> return !waitqueue_active(&ws->wait); >> >> - *nr -= sub; >> - >> /* >> * When wait_cnt == 0, we have to be particularly careful as we are >> * responsible to reset wait_cnt regardless whether we've actually >> - * woken up anybody. But in case we didn't wakeup anybody, we still >> - * need to retry. >> + * woken up anybody. But in case we didn't wakeup anybody, we should >> + * not consume nr and need to retry to avoid lost wakeups. >> */ >> - ret = !waitqueue_active(&ws->wait); > There is a warnning reported by checkpatch.pl which is > "WARNING:waitqueue_active without comment" but I don't know why. Most likely because waitqueue_active() could be racy, so a comment is warranted on why it's safe rather than using wq_has_sleeper(). -- Jens Axboe
