Re: [PATCH v4] block: plug attempts to batch allocate tags multiple times

[Yu Kuai <yukuai1@huaweicloud.com>]

Hi,

在 2025/10/14 16:31, Xue He 写道:
> Hi Yu Kuai,
> 
> On 2025/10/14 15:28, Yu Kuai wrote:
>> On 2025/10/10 14:35, Xue He wrote:
>>>    static unsigned int __map_depth_with_shallow(const struct sbitmap *sb,
>>>    					     int index,
>>>    					     unsigned int shallow_depth)
>>> @@ -534,26 +560,22 @@ unsigned long __sbitmap_queue_get_batch(struct sbitmap_queue *sbq, int nr_tags,
>>>    		unsigned int map_depth = __map_depth(sb, index);
>>>    		unsigned long val;
>>>    
>>> -		sbitmap_deferred_clear(map, 0, 0, 0);
>>> -		val = READ_ONCE(map->word);
>>> -		if (val == (1UL << (map_depth - 1)) - 1)
>>> +		nr = sbitmap_find_bits_in_word(map, 0, 0, 0, &val, nr_tags, map_depth);
>>> +		if (nr == -1UL)
>>>    			goto next;
>>>    
>>> -		nr = find_first_zero_bit(&val, map_depth);
>>> -		if (nr + nr_tags <= map_depth) {
>>> -			atomic_long_t *ptr = (atomic_long_t *) &map->word;
>>> -
>>> -			get_mask = ((1UL << nr_tags) - 1) << nr;
>>> -			while (!atomic_long_try_cmpxchg(ptr, &val,
>>> -							  get_mask | val))
>>> -				;
>>> -			get_mask = (get_mask & ~val) >> nr;
>>> -			if (get_mask) {
>>> -				*offset = nr + (index << sb->shift);
>>> -				update_alloc_hint_after_get(sb, depth, hint,
>>> -							*offset + nr_tags - 1);
>>> -				return get_mask;
>>> -			}
>>> +		atomic_long_t *ptr = (atomic_long_t *) &map->word;
>>> +
>>> +		get_mask = ((1UL << nr_tags) - 1) << nr;
>>> +		while (!atomic_long_try_cmpxchg(ptr, &val,
>>> +						  get_mask | val))
>>> +			;
>>> +		get_mask = (get_mask & ~val) >> nr;
>>> +		if (get_mask) {
>>> +			*offset = nr + (index << sb->shift);
>>> +			update_alloc_hint_after_get(sb, depth, hint,
>>> +						*offset + nr_tags - 1);
>>> +			return get_mask;
>>
>> I feel it's better to fold in above into the helper and return get_mask
>> directly.
> Hi Yukuai, sorry I'm not sure that my understanding is correct, does it
> mean modifying it in a way similar to the following? If so, I will repackage
> the patch accordingly.
> 
> static unsigned long sbitmap_find_bits_in_word(unsigned long index,
> 					struct sbitmap *sb, unsigned int *offset,
> 					unsigned int hint, unsigned int depth, int nr_tags)
> {
> 	unsigned long val, nr, get_mask;
> 	struct sbitmap_word *map = &sb->map[index];
> 	unsigned int map_depth = __map_depth(sb, index);
> 
> 	while (1) {
> 		val = READ_ONCE(map->word);
> 		if (val == (1UL << (map_depth - 1)) - 1) {
> 			if (!sbitmap_deferred_clear(map, 0, 0, 0))
> 				return 0;
> 			continue;
> 		}
> 
> 		nr = find_first_zero_bit(&val, map_depth);
> 		if (nr != map_depth)
> 			break;
> 
> 		if (!sbitmap_deferred_clear(map, 0, 0, 0))
> 			return 0;
> 	};
> 
> 	atomic_long_t *ptr = (atomic_long_t *) &map->word;
> 	
> 	get_mask = ((1UL << nr_tags) - 1) << nr;

Be careful, since the checking nr + nr_tags <= map_depth is removed,
nr_tags should be updated to:

max(nr_tags, map_depth - nr)

> 	while (!atomic_long_try_cmpxchg(ptr, &val,
> 					  get_mask | val))
> 		;
> 	get_mask = (get_mask & ~val) >> nr;
> 	if (get_mask){
> 		*offset = nr + (index << sb->shift);
> 		update_alloc_hint_after_get(sb, depth, hint,
> 					*offset + nr_tags - 1);

Look to me you're still pass in depth and hint for this, and I think
it's cleaner to move this to the caller :) Use the old nr_tags from
caller is fine, hint will be set to 0 if passed in nr >= depth -1.

Thanks,
Kuai


> 	}
> 	return get_mask;
> }
> 
> and upper like
> 
> unsigned long __sbitmap_queue_get_batch(struct sbitmap_queue *sbq, int nr_tags,
> 						unsigned int *offset)
> {
> 	struct sbitmap *sb = &sbq->sb;
> 	unsigned int hint, depth;
> 	unsigned long get_mask, index;
> 	int i;
> 
> 	if (unlikely(sb->round_robin))
> 		return 0;
> 
> 	depth = READ_ONCE(sb->depth);
> 	hint = update_alloc_hint_before_get(sb, depth);
> 
> 	index = SB_NR_TO_INDEX(sb, hint);
> 
> 	for (i = 0; i < sb->map_nr; i++) {
> 		get_mask = sbitmap_find_bits_in_word(index, sb, offset, hint,
> 							depth, nr_tags);
> 		if (get_mask) {
> 			return get_mask;
> 		}
> 
> 		/* Jump to next index. */
> 		if (++index >= sb->map_nr)
> 			index = 0;
> 	}
> 
> 	return 0;
> }
> 
> Am I missing something?
> 
> Thanks,
> Xue
>>
>> BTW, the sbitmap and blk-mq changes are not quite related, and you can
>> split them into seperate patches.
>>
>> Thanks,
>> Kuai
>>
>>>    		}
>>>    next:
>>>    		/* Jump to next index. */
>>>
> 
> .
>