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

[Yu Kuai]

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. */
>
> .