2020-04-15 21:41:11

by Qais Yousef

[permalink] [raw]
Subject: [PATCH 3/4] cpumask: Convert cpumask_any_but() to the new random function

cpumask_any() and cpumask_any_and() are now truly, move the
cpumask_any_but() to behave in a similar manner.

Signed-off-by: Qais Yousef <[email protected]>
CC: Juri Lelli <[email protected]>
CC: Vincent Guittot <[email protected]>
CC: Dietmar Eggemann <[email protected]>
CC: Steven Rostedt <[email protected]>
CC: Ben Segall <[email protected]>
CC: Mel Gorman <[email protected]>
CC: Andrew Morton <[email protected]>
CC: Thomas Gleixner <[email protected]>
CC: Yury Norov <[email protected]>
CC: Paul Turner <[email protected]>
CC: Alexey Dobriyan <[email protected]>
CC: Josh Don <[email protected]>
CC: Pavan Kondeti <[email protected]>
CC: [email protected]
---
include/linux/cpumask.h | 2 +-
lib/cpumask.c | 44 ++++++++++++++++++++++-------------------
2 files changed, 25 insertions(+), 21 deletions(-)

diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index 7fb25d256043..f04e983e9dd0 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -254,10 +254,10 @@ static inline unsigned int cpumask_next_zero(int n, const struct cpumask *srcp)
}

int cpumask_next_and(int n, const struct cpumask *, const struct cpumask *);
-int cpumask_any_but(const struct cpumask *mask, unsigned int cpu);
unsigned int cpumask_local_spread(unsigned int i, int node);
int cpumask_any(const struct cpumask *srcp);
int cpumask_any_and(const struct cpumask *src1p, const struct cpumask *src2p);
+int cpumask_any_but(const struct cpumask *srcp, unsigned int cpu);

/**
* for_each_cpu - iterate over every cpu in a mask
diff --git a/lib/cpumask.c b/lib/cpumask.c
index bcac63e45374..0295cf5486ab 100644
--- a/lib/cpumask.c
+++ b/lib/cpumask.c
@@ -42,26 +42,6 @@ int cpumask_next_and(int n, const struct cpumask *src1p,
}
EXPORT_SYMBOL(cpumask_next_and);

-/**
- * cpumask_any_but - return a "random" in a cpumask, but not this one.
- * @mask: the cpumask to search
- * @cpu: the cpu to ignore.
- *
- * Often used to find any cpu but smp_processor_id() in a mask.
- * Returns >= nr_cpu_ids if no cpus set.
- */
-int cpumask_any_but(const struct cpumask *mask, unsigned int cpu)
-{
- unsigned int i;
-
- cpumask_check(cpu);
- for_each_cpu(i, mask)
- if (i != cpu)
- break;
- return i;
-}
-EXPORT_SYMBOL(cpumask_any_but);
-
/**
* cpumask_next_wrap - helper to implement for_each_cpu_wrap
* @n: the cpu prior to the place to search
@@ -283,3 +263,27 @@ int cpumask_any(const struct cpumask *srcp)
return next;
}
EXPORT_SYMBOL(cpumask_any);
+
+/**
+ * cpumask_any_but - return a "random" in a cpumask, but not this one.
+ * @srcp: the cpumask to search
+ * @cpu: the cpu to ignore.
+ *
+ * Often used to find any cpu but smp_processor_id() in a mask.
+ * Returns >= nr_cpu_ids if no cpus set.
+ */
+int cpumask_any_but(const struct cpumask *srcp, unsigned int cpu)
+{
+ unsigned int i;
+
+ cpumask_check(cpu);
+
+ for_each_cpu(i, srcp) {
+ i = cpumask_any(srcp);
+ if (i != cpu)
+ return i;
+ }
+
+ return nr_cpu_ids;
+}
+EXPORT_SYMBOL(cpumask_any_but);
--
2.17.1


2020-04-15 21:47:02

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH 3/4] cpumask: Convert cpumask_any_but() to the new random function

On Tue, 14 Apr 2020 16:05:55 +0100
Qais Yousef <[email protected]> wrote:

> +int cpumask_any_but(const struct cpumask *srcp, unsigned int cpu)
> +{
> + unsigned int i;
> +
> + cpumask_check(cpu);
> +
> + for_each_cpu(i, srcp) {
> + i = cpumask_any(srcp);

Hmm, if the current CPU is the last CPU in the mask, and cpumask_any()
happens to return it, what happens?

> + if (i != cpu)
> + return i;

We loop again, and wouldn't i being the last CPU in the mask cause this
loop to exit, and return nr_cpu_ids?

-- Steve

> + }
> +
> + return nr_cpu_ids;
> +}

2020-04-20 15:51:01

by Qais Yousef

[permalink] [raw]
Subject: Re: [PATCH 3/4] cpumask: Convert cpumask_any_but() to the new random function

On 04/14/20 12:28, Steven Rostedt wrote:
> On Tue, 14 Apr 2020 16:05:55 +0100
> Qais Yousef <[email protected]> wrote:
>
> > +int cpumask_any_but(const struct cpumask *srcp, unsigned int cpu)
> > +{
> > + unsigned int i;
> > +
> > + cpumask_check(cpu);
> > +
> > + for_each_cpu(i, srcp) {
> > + i = cpumask_any(srcp);
>
> Hmm, if the current CPU is the last CPU in the mask, and cpumask_any()
> happens to return it, what happens?

cpumask_any() will wrap.

>
> > + if (i != cpu)
> > + return i;
>
> We loop again, and wouldn't i being the last CPU in the mask cause this
> loop to exit, and return nr_cpu_ids?

No, because if we happen to start from the last cpu, on the next call, we'll
wrap again to the beginning.

But this implementation is crap indeed. No matter how unlikely, there's no
guarantee that for all the iters cpumask_any() will return a different cpu.
So if there are 3 cpus in the mask, cpumask_any() could potentially always
return 'cpu' if the planets aligned correctly.

I'll open code it instead to guarantee robustness.

Thanks!

--
Qais Yousef