Re: [PATCH v3 1/4] lib/find_bit: introduce FIND_FIRST_BIT() macro

[Valentin Schneider]

On 27/08/22 10:58, Yury Norov wrote:
> Now that we have many flavors of find_first_bit(), and expect even more,
> it's better to have one macro that generates optimal code for all and makes
> maintaining of slightly different functions simpler.
>
> The logic common to all versions is moved to the new macro, and all the
> flavors are generated by providing an FETCH macro-parameter, like
> in this example:
>
>   #define FIND_FIRST_BIT(FETCH, MUNGE, size) ...
>
>   find_first_ornot_and_bit(addr1, addr2, addr3, size)
>   {
>         return FIND_NEXT_BIT(addr1[idx] | ~addr2[idx] & addr3[idx], /* nop */, size);
>   }
>
> The FETCH may be of any complexity, as soon as it only refers
> the bitmap(s) and an iterator idx.
>
> MUNGE is here to support _le code generation for BE builds. May be
> empty.
>
> Suggested-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Yury Norov <yury.norov@xxxxxxxxx>

Just one small comment below about the /* nop */, regardless:

Reviewed-by: Valentin Schneider <vschneid@xxxxxxxxxx>

>  unsigned long _find_first_bit(const unsigned long *addr, unsigned long size)
>  {
> -	unsigned long idx;
> -
> -	for (idx = 0; idx * BITS_PER_LONG < size; idx++) {
> -		if (addr[idx])
> -			return min(idx * BITS_PER_LONG + __ffs(addr[idx]), size);
> -	}
> -
> -	return size;
> +	return FIND_FIRST_BIT(addr[idx], /* nop */, size);

FWIW I thought passing an explicit identity-mapping macro would make things
a bit clearer, but not really since you have to hunt for where that macro
is defined (an inline "lambda x : x" would have been perfect :-)), so I
think what you've gone for is the lesser evil.

>  }
>  EXPORT_SYMBOL(_find_first_bit);