Re: [PATCH v3 1/3] bitmap: generalize node_random()

[Andrew Morton]

On Tue, 17 Jun 2025 16:08:51 -0400 Yury Norov <yury.norov@xxxxxxxxx> wrote:

> From: "Yury Norov [NVIDIA]" <yury.norov@xxxxxxxxx>
> 
> Generalize node_random() and make it available to general bitmaps and
> cpumasks users.
> 
> Notice, find_first_bit() is generally faster than find_nth_bit(), and we
> employ it when there's a single set bit in the bitmap.
> 
> See commit 3e061d924fe9c7b4 ("lib/nodemask: optimize node_random for
> nodemask with single NUMA node").
> 
> ...
>
> --- a/include/linux/nodemask.h
> +++ b/include/linux/nodemask.h
> @@ -492,21 +492,7 @@ static __always_inline int num_node_state(enum node_states state)
>  static __always_inline int node_random(const nodemask_t *maskp)
>  {
>  #if defined(CONFIG_NUMA) && (MAX_NUMNODES > 1)
> -	int w, bit;
> -
> -	w = nodes_weight(*maskp);
> -	switch (w) {
> -	case 0:
> -		bit = NUMA_NO_NODE;

If the mask has no bits set, return -1.

> -		break;
> -	case 1:
> -		bit = first_node(*maskp);
> -		break;
> -	default:
> -		bit = find_nth_bit(maskp->bits, MAX_NUMNODES, get_random_u32_below(w));
> -		break;
> -	}
> -	return bit;
> +	return find_random_bit(maskp->bits, MAX_NUMNODES);
>
> ...
>
> +unsigned long find_random_bit(const unsigned long *addr, unsigned long size)
> +{
> +	int w = bitmap_weight(addr, size);
> +
> +	switch (w) {
> +	case 0:
> +		return size;

If the mask has no bits set, return the mask's size.

> +	case 1:
> +		/* Performance trick for single-bit bitmaps */
> +		return find_first_bit(addr, size);
> +	default:
> +		return find_nth_bit(addr, size, get_random_u32_below(w));
> +	}
> +}

I'm not seeing how this is correct?