on 12/2/2022 8:58 AM, Jens Axboe wrote: > 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(). Thanks for explanation, so the patch seems fine as comment is present already though it doesn't mention sting "waitqueue_active" directly. No bother anymore, this patch will be dropped as the fixed code is stale. Thanks again. -- Best wishes Kemeng Shi
