The description of the macro in `include/linux/cpumask.h` says the
variable 'cpu' can be unsigned int.
However in the for_each_cpu(), for_each_cpu_wrap() and
for_each_cpu_and(), its value is assigned to -1.
That doesn't make sense. Moreover in the cpumask_next(),
cpumask_next_zero(), cpumask_next_wrap() and cpumask_next_and(),
'cpu' will be implicitly type conversed to int if the type is
unsigned int.
It is universally accepted that the implicit type conversion is
terrible.
Also, having the good programming custom will set an example for
others.
Thus, it might be better to fix the macro description of 'cpu' that
remove the '(optionally unsigned)' and change the definition of 'cpu'
in `drivers/md/raid5.c` from unsigned long to long.
Fixes: c743f0a ("sched/fair, cpumask: Export for_each_cpu_wrap()")
Fixes: 8bd93a2 ("rcu: Accelerate grace period if last non-dynticked CPU")
Fixes: 984f2f3 ("cpumask: introduce new API, without changing anything, v3")
Fixes: 738a273 ("md/raid5: fix allocation of 'scribble' array.")
Signed-off-by: Jiasheng Jiang <[email protected]>
---
drivers/md/raid5.c | 2 +-
include/linux/cpumask.h | 8 ++++----
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 7d4ff8a..32ef82b 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -2425,7 +2425,7 @@ static int scribble_alloc(struct raid5_percpu *percpu,
static int resize_chunks(struct r5conf *conf, int new_disks, int new_sectors)
{
- unsigned long cpu;
+ long cpu;
int err = 0;
/*
diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index bfc4690..ceaed99 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -232,7 +232,7 @@ int cpumask_any_distribute(const struct cpumask *srcp);
/**
* for_each_cpu - iterate over every cpu in a mask
- * @cpu: the (optionally unsigned) integer iterator
+ * @cpu: the integer iterator
* @mask: the cpumask pointer
*
* After the loop, cpu is >= nr_cpu_ids.
@@ -244,7 +244,7 @@ int cpumask_any_distribute(const struct cpumask *srcp);
/**
* for_each_cpu_not - iterate over every cpu in a complemented mask
- * @cpu: the (optionally unsigned) integer iterator
+ * @cpu: the integer iterator
* @mask: the cpumask pointer
*
* After the loop, cpu is >= nr_cpu_ids.
@@ -258,7 +258,7 @@ extern int cpumask_next_wrap(int n, const struct cpumask *mask, int start, bool
/**
* for_each_cpu_wrap - iterate over every cpu in a mask, starting at a specified location
- * @cpu: the (optionally unsigned) integer iterator
+ * @cpu: the integer iterator
* @mask: the cpumask poiter
* @start: the start location
*
@@ -273,7 +273,7 @@ extern int cpumask_next_wrap(int n, const struct cpumask *mask, int start, bool
/**
* for_each_cpu_and - iterate over every cpu in both masks
- * @cpu: the (optionally unsigned) integer iterator
+ * @cpu: the integer iterator
* @mask1: the first cpumask pointer
* @mask2: the second cpumask pointer
*
--
2.7.4
> On Oct 26, 2021, at 2:26 AM, Jiasheng Jiang <[email protected]> wrote:
>
> The description of the macro in `include/linux/cpumask.h` says the
> variable 'cpu' can be unsigned int.
> However in the for_each_cpu(), for_each_cpu_wrap() and
> for_each_cpu_and(), its value is assigned to -1.
> That doesn't make sense. Moreover in the cpumask_next(),
> cpumask_next_zero(), cpumask_next_wrap() and cpumask_next_and(),
> 'cpu' will be implicitly type conversed to int if the type is
> unsigned int.
> It is universally accepted that the implicit type conversion is
> terrible.
> Also, having the good programming custom will set an example for
> others.
> Thus, it might be better to fix the macro description of 'cpu' that
> remove the '(optionally unsigned)' and change the definition of 'cpu'
> in `drivers/md/raid5.c` from unsigned long to long.
Implicit casts are dangerous in certain cases. I am not sure the
case you addressed is such.
Sometimes the generated code is more efficient when casting is
avoided, especially when both the size and sign are changed.
However, in practice, the performance impact is negligent.
If you want to address this issue, it would be best, I think,
to add some assertion and actually deal with all the existing
issues (e.g., see below). Anyhow, unless you find a real
functional bug, I would drop the “fixes” tag.
To find additional issues, you can try to use something like:
diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index 5d4d07a9e1ed..cda89e6e601e 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -230,6 +230,12 @@ int cpumask_any_and_distribute(const struct cpumask *src1p,
const struct cpumask *src2p);
int cpumask_any_distribute(const struct cpumask *srcp);
+#include <linux/build_bug.h>
+
+static __always_inline void build_bug_on(bool c) {
+ BUILD_BUG_ON(c);
+}
+
/**
* for_each_cpu - iterate over every cpu in a mask
* @cpu: the (optionally unsigned) integer iterator
@@ -237,10 +243,10 @@ int cpumask_any_distribute(const struct cpumask *srcp);
*
* After the loop, cpu is >= nr_cpu_ids.
*/
-#define for_each_cpu(cpu, mask) \
- for ((cpu) = -1; \
- (cpu) = cpumask_next((cpu), (mask)), \
- (cpu) < nr_cpu_ids;)
+#define for_each_cpu(cpu, mask) \
+ for ((cpu) = -1, build_bug_on(!__same_type((cpu), int) && !__same_type((cpu), unsigned int); \
+ (cpu) = cpumask_next((cpu), (mask)), \
+ (cpu) < nr_cpu_ids; )
/**
* for_each_cpu_not - iterate over every cpu in a complemented mask