| Message ID | 20241228184949.31582-7-yury.norov@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | cpumask: cleanup cpumask_next_wrap() implementation and usage | expand |
On Sat, Dec 28, 2024 at 10:49:38AM -0800, Yury Norov wrote: > cpumask_next_wrap_old() has two additional parameters, comparing to it's > analogue in linux/find.h find_next_bit_wrap(). The reason for that is > historical. s/it's/its/ Personally I think cscope/tags/git grep make "find_next_bit_wrap()" enough even without mentioning "linux/find.h". > + * cpumask_next_and_wrap - get the next cpu in *src1p & *src2p, starting from > + * @n and wrapping around, if needed > + * @n: the cpu prior to the place to search (i.e. return will be > @n) Is the return really > @n if it wraps?
On Sat, Dec 28, 2024 at 10:49:38AM -0800, Yury Norov wrote: Hi Yury, > cpumask_next_wrap_old() has two additional parameters, comparing to it's > analogue in linux/find.h find_next_bit_wrap(). The reason for that is > historical. > > Before 4fe49b3b97c262 ("lib/bitmap: introduce for_each_set_bit_wrap() > macro"), cpumask_next_wrap() was used to implement for_each_cpu_wrap() > iterator. Now that the iterator is an alias to generic > for_each_set_bit_wrap(), the additional parameters aren't used and may > confuse readers. > > All existing users call cpumask_next_wrap() in a way that makes it > possible to turn it to straight and simple alias to find_next_bit_wrap(). > > In a couple places kernel users opencode missing cpumask_next_and_wrap(). > Add it as well. > > Signed-off-by: Yury Norov <yury.norov@gmail.com> > --- > include/linux/cpumask.h | 37 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 37 insertions(+) > > diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h > index b267a4f6a917..18c9908d50c4 100644 > --- a/include/linux/cpumask.h > +++ b/include/linux/cpumask.h > @@ -284,6 +284,43 @@ unsigned int cpumask_next_and(int n, const struct cpumask *src1p, > small_cpumask_bits, n + 1); > } > > +/** > + * cpumask_next_and_wrap - get the next cpu in *src1p & *src2p, starting from > + * @n and wrapping around, if needed > + * @n: the cpu prior to the place to search (i.e. return will be > @n) > + * @src1p: the first cpumask pointer > + * @src2p: the second cpumask pointer > + * > + * Return: >= nr_cpu_ids if no further cpus set in both. > + */ > +static __always_inline > +unsigned int cpumask_next_and_wrap(int n, const struct cpumask *src1p, > + const struct cpumask *src2p) > +{ > + /* -1 is a legal arg here. */ > + if (n != -1) > + cpumask_check(n); > + return find_next_and_bit_wrap(cpumask_bits(src1p), cpumask_bits(src2p), > + small_cpumask_bits, n + 1); > +} > + > +/* > + * cpumask_next_wrap - get the next cpu in *src, starting from > + * @n and wrapping around, if needed Does it mean the search wraps a cpumask and starts from the beginning if the bit is not found and returns >= nr_cpu_ids if @n crosses itself? > + * @n: the cpu prior to the place to search > + * @src: cpumask pointer > + * > + * Return: >= nr_cpu_ids if no further cpus set in both. It looks like Return is a cpumask_next_and_wrap() comment leftover. > + */ > +static __always_inline > +unsigned int cpumask_next_wrap(int n, const struct cpumask *src) > +{ > + /* -1 is a legal arg here. */ > + if (n != -1) > + cpumask_check(n); > + return find_next_bit_wrap(cpumask_bits(src), small_cpumask_bits, n + 1); > +} > + > /** > * for_each_cpu - iterate over every cpu in a mask > * @cpu: the (optionally unsigned) integer iterator Thanks!
On Tue, Jan 07, 2025 at 02:28:31PM +0100, Alexander Gordeev wrote: > On Sat, Dec 28, 2024 at 10:49:38AM -0800, Yury Norov wrote: > > Hi Yury, > > > cpumask_next_wrap_old() has two additional parameters, comparing to it's > > analogue in linux/find.h find_next_bit_wrap(). The reason for that is > > historical. > > > > Before 4fe49b3b97c262 ("lib/bitmap: introduce for_each_set_bit_wrap() > > macro"), cpumask_next_wrap() was used to implement for_each_cpu_wrap() > > iterator. Now that the iterator is an alias to generic > > for_each_set_bit_wrap(), the additional parameters aren't used and may > > confuse readers. > > > > All existing users call cpumask_next_wrap() in a way that makes it > > possible to turn it to straight and simple alias to find_next_bit_wrap(). > > > > In a couple places kernel users opencode missing cpumask_next_and_wrap(). > > Add it as well. > > > > Signed-off-by: Yury Norov <yury.norov@gmail.com> > > --- > > include/linux/cpumask.h | 37 +++++++++++++++++++++++++++++++++++++ > > 1 file changed, 37 insertions(+) > > > > diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h > > index b267a4f6a917..18c9908d50c4 100644 > > --- a/include/linux/cpumask.h > > +++ b/include/linux/cpumask.h > > @@ -284,6 +284,43 @@ unsigned int cpumask_next_and(int n, const struct cpumask *src1p, > > small_cpumask_bits, n + 1); > > } > > > > +/** > > + * cpumask_next_and_wrap - get the next cpu in *src1p & *src2p, starting from > > + * @n and wrapping around, if needed > > + * @n: the cpu prior to the place to search (i.e. return will be > @n) > > + * @src1p: the first cpumask pointer > > + * @src2p: the second cpumask pointer > > + * > > + * Return: >= nr_cpu_ids if no further cpus set in both. > > + */ > > +static __always_inline > > +unsigned int cpumask_next_and_wrap(int n, const struct cpumask *src1p, > > + const struct cpumask *src2p) > > +{ > > + /* -1 is a legal arg here. */ > > + if (n != -1) > > + cpumask_check(n); > > + return find_next_and_bit_wrap(cpumask_bits(src1p), cpumask_bits(src2p), > > + small_cpumask_bits, n + 1); > > +} > > + > > +/* > > + * cpumask_next_wrap - get the next cpu in *src, starting from > > + * @n and wrapping around, if needed > > Does it mean the search wraps a cpumask and starts from the beginning > if the bit is not found and returns >= nr_cpu_ids if @n crosses itself? > > > + * @n: the cpu prior to the place to search > > + * @src: cpumask pointer > > + * > > + * Return: >= nr_cpu_ids if no further cpus set in both. > > It looks like Return is a cpumask_next_and_wrap() comment leftover. > > > + */ > > +static __always_inline > > +unsigned int cpumask_next_wrap(int n, const struct cpumask *src) > > +{ > > + /* -1 is a legal arg here. */ > > + if (n != -1) > > + cpumask_check(n); > > + return find_next_bit_wrap(cpumask_bits(src), small_cpumask_bits, n + 1); > > +} > > + > > /** > > * for_each_cpu - iterate over every cpu in a mask > > * @cpu: the (optionally unsigned) integer iterator > > Thanks! Thanks, I'll update the comments.
On Fri, Jan 03, 2025 at 11:44:32AM -0600, Bjorn Helgaas wrote: > On Sat, Dec 28, 2024 at 10:49:38AM -0800, Yury Norov wrote: > > cpumask_next_wrap_old() has two additional parameters, comparing to it's > > analogue in linux/find.h find_next_bit_wrap(). The reason for that is > > historical. > > s/it's/its/ > > Personally I think cscope/tags/git grep make "find_next_bit_wrap()" > enough even without mentioning "linux/find.h". > > > + * cpumask_next_and_wrap - get the next cpu in *src1p & *src2p, starting from > > + * @n and wrapping around, if needed > > + * @n: the cpu prior to the place to search (i.e. return will be > @n) > > Is the return really > @n if it wraps? No, this is a copy-paste error. Will fix in v2. Thanks, Yury
diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h index b267a4f6a917..18c9908d50c4 100644 --- a/include/linux/cpumask.h +++ b/include/linux/cpumask.h @@ -284,6 +284,43 @@ unsigned int cpumask_next_and(int n, const struct cpumask *src1p, small_cpumask_bits, n + 1); } +/** + * cpumask_next_and_wrap - get the next cpu in *src1p & *src2p, starting from + * @n and wrapping around, if needed + * @n: the cpu prior to the place to search (i.e. return will be > @n) + * @src1p: the first cpumask pointer + * @src2p: the second cpumask pointer + * + * Return: >= nr_cpu_ids if no further cpus set in both. + */ +static __always_inline +unsigned int cpumask_next_and_wrap(int n, const struct cpumask *src1p, + const struct cpumask *src2p) +{ + /* -1 is a legal arg here. */ + if (n != -1) + cpumask_check(n); + return find_next_and_bit_wrap(cpumask_bits(src1p), cpumask_bits(src2p), + small_cpumask_bits, n + 1); +} + +/* + * cpumask_next_wrap - get the next cpu in *src, starting from + * @n and wrapping around, if needed + * @n: the cpu prior to the place to search + * @src: cpumask pointer + * + * Return: >= nr_cpu_ids if no further cpus set in both. + */ +static __always_inline +unsigned int cpumask_next_wrap(int n, const struct cpumask *src) +{ + /* -1 is a legal arg here. */ + if (n != -1) + cpumask_check(n); + return find_next_bit_wrap(cpumask_bits(src), small_cpumask_bits, n + 1); +} + /** * for_each_cpu - iterate over every cpu in a mask * @cpu: the (optionally unsigned) integer iterator
cpumask_next_wrap_old() has two additional parameters, comparing to it's analogue in linux/find.h find_next_bit_wrap(). The reason for that is historical. Before 4fe49b3b97c262 ("lib/bitmap: introduce for_each_set_bit_wrap() macro"), cpumask_next_wrap() was used to implement for_each_cpu_wrap() iterator. Now that the iterator is an alias to generic for_each_set_bit_wrap(), the additional parameters aren't used and may confuse readers. All existing users call cpumask_next_wrap() in a way that makes it possible to turn it to straight and simple alias to find_next_bit_wrap(). In a couple places kernel users opencode missing cpumask_next_and_wrap(). Add it as well. Signed-off-by: Yury Norov <yury.norov@gmail.com> --- include/linux/cpumask.h | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)