Received-SPF: pass (google.com: domain of linux-kernel+bounces-142858-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240412105839.2896281-1-dawei.li@shingroup.cn> <20240412105839.2896281-2-dawei.li@shingroup.cn>
In-Reply-To: <20240412105839.2896281-2-dawei.li@shingroup.cn>
From: Yury Norov <yury.norov@gmail.com>
Date: Fri, 12 Apr 2024 07:40:30 -0700
Message-ID: <CAAH8bW_97sChHOMCEY22jG0jvxZO-qa8syfotz9sF9T6oCr_4Q@mail.gmail.com>
Subject: Re: [PATCH 1/6] cpumask: introduce cpumask_first_and_and()
To: Dawei Li <dawei.li@shingroup.cn>
Cc: tglx@linutronix.de, akpm@linux-foundation.org, maz@kernel.org, 
	florian.fainelli@broadcom.com, chenhuacai@kernel.org, jiaxun.yang@flygoat.com, 
	anup@brainfault.org, palmer@dabbelt.com, samuel.holland@sifive.com, 
	linux@rasmusvillemoes.dk, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Apr 12, 2024 at 3:59=E2=80=AFAM Dawei Li <dawei.li@shingroup.cn> wr=
ote:
>
> For some cases, it's required to make intersection between 3 cpumasks
> and return possible cpu bit. Current implementation for these cases are

/s/possible cpu bit/set cpu/

And sometimes you just need an intersection cpumask, and don't need to retu=
rn
any set cpu. This patch introduces new API, so description should be as
common as possible.

We've already have some 3-bitmap functions. Can you look at commit messages
there and align your wording?

I'll be OK if you just skip this part and go straight to "Introduce
.. to get rid of"

> allocating a temporary cpumask var(sometimes on stack) storing intermedia=
te
> calculation result.
>
> Introduce cpumask_first_and_and() to get rid of this intermediate orinted

what the 'orinted' is?

> approach. Instead, cpumask_first_and_and() works in-place with all inputs
> and produce desired output directly.

/s/produce/procuces/

>
> Signed-off-by: Dawei Li <dawei.li@shingroup.cn>
> ---
>  include/linux/cpumask.h | 17 +++++++++++++++++
>  include/linux/find.h    | 29 +++++++++++++++++++++++++++++
>  lib/find_bit.c          | 14 ++++++++++++++
>  3 files changed, 60 insertions(+)
>
> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> index 1c29947db848..c46f9e9e1d66 100644
> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -187,6 +187,23 @@ unsigned int cpumask_first_and(const struct cpumask =
*srcp1, const struct cpumask
>         return find_first_and_bit(cpumask_bits(srcp1), cpumask_bits(srcp2=
), small_cpumask_bits);
>  }
>
> +/**
> + * cpumask_first_and_and - return the first cpu from *srcp1 & *srcp2 & *=
srcp3
> + * @srcp1: the first input
> + * @srcp2: the second input
> + * @srcp3: the third input
> + *
> + * Return: >=3D nr_cpu_ids if no cpus set in all.
> + */
> +static inline
> +unsigned int cpumask_first_and_and(const struct cpumask *srcp1,
> +                                  const struct cpumask *srcp2,
> +                                  const struct cpumask *srcp3)
> +{
> +       return find_first_and_and_bit(cpumask_bits(srcp1), cpumask_bits(s=
rcp2),
> +                                     cpumask_bits(srcp3), small_cpumask_=
bits);
> +}
> +
>  /**
>   * cpumask_last - get the last CPU in a cpumask
>   * @srcp:      - the cpumask pointer
> diff --git a/include/linux/find.h b/include/linux/find.h
> index c69598e383c1..047081c6b9f7 100644
> --- a/include/linux/find.h
> +++ b/include/linux/find.h
> @@ -29,6 +29,8 @@ unsigned long __find_nth_and_andnot_bit(const unsigned =
long *addr1, const unsign
>                                         unsigned long n);
>  extern unsigned long _find_first_and_bit(const unsigned long *addr1,
>                                          const unsigned long *addr2, unsi=
gned long size);
> +unsigned long _find_first_and_and_bit(const unsigned long *addr1, const =
unsigned long *addr2,
> +                                     const unsigned long *addr3, unsigne=
d long size);
>  extern unsigned long _find_first_zero_bit(const unsigned long *addr, uns=
igned long size);
>  extern unsigned long _find_last_bit(const unsigned long *addr, unsigned =
long size);
>
> @@ -345,6 +347,33 @@ unsigned long find_first_and_bit(const unsigned long=
 *addr1,
>  }
>  #endif
>
> +#ifndef find_first_and_and_bit

Don't need to protect new API unless you've got an actual arch implementati=
on.

> +/**
> + * find_first_and_and_bit - find the first set bit in 3 memory regions
> + * @addr1: The first address to base the search on
> + * @addr2: The second address to base the search on
> + * @addr3: The third address to base the search on
> + * @size: The bitmap size in bits
> + *
> + * Returns the bit number for the first set bit
> + * If no bits are set, returns @size.
> + */
> +static inline
> +unsigned long find_first_and_and_bit(const unsigned long *addr1,
> +                                    const unsigned long *addr2,
> +                                    const unsigned long *addr3,
> +                                    unsigned long size)
> +{
> +       if (small_const_nbits(size)) {
> +               unsigned long val =3D *addr1 & *addr2 & *addr3 & GENMASK(=
size - 1, 0);
> +
> +               return val ? __ffs(val) : size;
> +       }
> +
> +       return _find_first_and_and_bit(addr1, addr2, addr3, size);
> +}
> +#endif
> +
>  #ifndef find_first_zero_bit
>  /**
>   * find_first_zero_bit - find the first cleared bit in a memory region
> diff --git a/lib/find_bit.c b/lib/find_bit.c
> index 32f99e9a670e..fdc5c4117e8b 100644
> --- a/lib/find_bit.c
> +++ b/lib/find_bit.c
> @@ -116,6 +116,20 @@ unsigned long _find_first_and_bit(const unsigned lon=
g *addr1,
>  EXPORT_SYMBOL(_find_first_and_bit);
>  #endif
>
> +#ifndef find_first_and_and_bit
> +/*
> + * Find the first set bit in three memory regions.
> + */
> +unsigned long _find_first_and_and_bit(const unsigned long *addr1,
> +                                     const unsigned long *addr2,
> +                                     const unsigned long *addr3,
> +                                     unsigned long size)
> +{
> +       return FIND_FIRST_BIT(addr1[idx] & addr2[idx] & addr3[idx], /* no=
p */, size);
> +}
> +EXPORT_SYMBOL(_find_first_and_and_bit);
> +#endif
> +
>  #ifndef find_first_zero_bit
>  /*
>   * Find the first cleared bit in a memory region.
> --
> 2.27.0
>