On 6/30/25 11:50 AM, Hannes Reinecke wrote:
>> +struct elevator_tags *blk_mq_alloc_sched_tags(struct blk_mq_tag_set *set,
>> + unsigned int nr_hw_queues)
>> +{
>> + unsigned int nr_tags;
>> + int i;
>> + struct elevator_tags *et;
>> + gfp_t gfp = GFP_NOIO | __GFP_ZERO | __GFP_NOWARN | __GFP_NORETRY;
>> +
>> + if (blk_mq_is_shared_tags(set->flags))
>> + nr_tags = 1;
>> + else
>> + nr_tags = nr_hw_queues;
>> +
>> + et = kmalloc(sizeof(struct elevator_tags) +
>> + nr_tags * sizeof(struct blk_mq_tags *), gfp);
>> + if (!et)
>> + return NULL;
>> + /*
>> + * Default to double of smaller one between hw queue_depth and
>> + * 128, since we don't split into sync/async like the old code
>> + * did. Additionally, this is a per-hw queue depth.
>> + */
>> + et->nr_requests = 2 * min_t(unsigned int, set->queue_depth,
>> + BLKDEV_DEFAULT_RQ);
>> + et->nr_hw_queues = nr_hw_queues;
>> +
>> + if (blk_mq_is_shared_tags(set->flags)) {
>> + /* Shared tags are stored at index 0 in @tags. */
>> + et->tags[0] = blk_mq_alloc_map_and_rqs(set, BLK_MQ_NO_HCTX_IDX,
>> + MAX_SCHED_RQ);
>> + if (!et->tags[0])
>> + goto out;
>> + } else {
>> + for (i = 0; i < et->nr_hw_queues; i++) {
>> + et->tags[i] = blk_mq_alloc_map_and_rqs(set, i,
>> + et->nr_requests);
>> + if (!et->tags[i])
>> + goto out_unwind;
>> + }
>> + }
>> +
>> + return et;
>> +out_unwind:
>> + while (--i >= 0)
>> + blk_mq_free_map_and_rqs(set, et->tags[i], i);
>> +out:
>> + kfree(et);
>> + return NULL;
>> +}
>> +
>
> As smatch stated, the unwind pattern is a bit odd.
> Maybe move the unwind into the 'else' branch, and us a conditional
> to invoke it:
>
> if (i < et->nr_hw_queues)
> while (--i >= 0)
> blk_mq_free_map_and_request()
>
I believe the 'if (i < et->nr_hw_queues)' check is unnecessary here. When
we jump to the @out_unwind label, @i is always less than @et->nr_hw_queues
because the for loop exits early (on allocation failure) before reaching
the upper bound. If @i had reached @et->nr_hw_queues, the loop would have
completed and we wouldn't jump to @out_unwind at all — we’d simply return
@et instead.
The Smatch flagged the unwind loop due to the use of an unsigned @i in the
previous patch. In that case, if the first allocation (i == 0) fails, then
'--i' underflows to UINT_MAX, and the condition '--i >= 0' is always true —
hence the warning.
This patch corrects that by declaring @i as a signed int, so that
'--i >= 0' behaves as expected and avoids the Smatch warning.
So, I don't think an extra condition like 'if (i < et->nr_hw_queues)' is
needed around the unwind loop. Agreed?
Thnaks,
--Nilay
