Re: [PATCH V3 02/14] sbitmap: introduce __sbitmap_for_each_set()

[Ming Lei <ming.lei@redhat.com>]

On Wed, Aug 30, 2017 at 03:55:13PM +0000, Bart Van Assche wrote:
> On Sun, 2017-08-27 at 00:33 +0800, Ming Lei wrote:
> >  /**
> >   * sbitmap_for_each_set() - Iterate over each set bit in a &struct sbitmap.
> > + * @start: Where to start the iteration
> 
> Thanks for having changed the name of this argument ...
> 
> > -static inline void sbitmap_for_each_set(struct sbitmap *sb, sb_for_each_fn fn,
> > -					void *data)
> > +static inline void __sbitmap_for_each_set(struct sbitmap *sb,
> > +					  unsigned int start,
> > +					  sb_for_each_fn fn, void *data)
> >  {
> > -	unsigned int i;
> > +	unsigned int index = SB_NR_TO_INDEX(sb, start);
> > +	unsigned int nr = SB_NR_TO_BIT(sb, start);
> > +	unsigned int scanned = 0;
> 
> ... but I'm still missing a check here whether or not index >= sb->map_nr.

The reason I didn't do that is because the only one user is always to
pass correct 'start'.

OK, will add it.

>  
> > -	for (i = 0; i < sb->map_nr; i++) {
> > -		struct sbitmap_word *word = &sb->map[i];
> > -		unsigned int off, nr;
> > +	while (1) {
> > +		struct sbitmap_word *word = &sb->map[index];
> > +		unsigned int depth = min_t(unsigned int, word->depth - nr,
> > +					   sb->depth - scanned);
> >  
> > +		scanned += depth;
> >  		if (!word->word)
> > -			continue;
> > +			goto next;
> >  
> > -		nr = 0;
> > -		off = i << sb->shift;
> > +		depth += nr;
> > +		start = index << sb->shift;
> 
> The above statement reuses the argument 'start' for a new purpose. This is
> confusing - please don't do this. Why not to keep the name 'off'? That will
> keep the changes minimal.

OK, that will rename the parameter of 'start' as 'off', so that we can
save one local variable.

-- 
Ming