Re: [RFC 3/3] kernel.h: use new typechecking macros in min()/max() and friends

Re: [RFC 3/3] kernel.h: use new typechecking macros in min()/max() and friends

[Peter Zijlstra]

On Sat, 2012-04-14 at 18:14 -0400, Sasha Levin wrote:
>  #define min(x, y) ({                           \
> -       typeof(x) _min1 = (x);                  \
> -       typeof(y) _min2 = (y);                  \
> -       (void) (&_min1 == &_min2);              \
> -       _min1 < _min2 ? _min1 : _min2; })
> +       typecmp2((x), (y));                             \
> +       (x) < (y) ? (x) : (y); }) 

Problem with this is that it evaluates x and y multiple times.

Re: [RFC 0/3] Extend type checking macros

[Peter Zijlstra]

On Sat, 2012-04-14 at 18:14 -0400, Sasha Levin wrote:
> the forgotten include/linux/typecheck.h 

AH! that's where it was hidden, I knew we had more of that stuff.

Re: [RFC 1/3] typecheck: extend typecheck.h with more useful typechecking macros

[Linus Torvalds]

On Sat, Apr 14, 2012 at 3:14 PM, Sasha Levin <levinsasha928@gmail.com> wrote:
> +#define typecheck2(type,x,y)           \
> +({     typecheck(type, x);             \
> +       typecheck(type, y);             \
> +       1;                              \
> +})

This seems silly.

Since typecheck() is already a nice expression returning 1, there's no
reason to do *another* level of statement expressions. So it would be
just

  #define typecheck2(type,x,y) \
    (typecheck(type, x), typecheck(type, y))

instead.

Which makes me suspect you don't really need that whole typecheck2 etc
macro set at all.

                 Linus

Re: [RFC 2/3] sched: add type checks to for_each_cpu_mask()

[Linus Torvalds]

On Sat, Apr 14, 2012 at 3:14 PM, Sasha Levin <levinsasha928@gmail.com> wrote:
> -#define for_each_cpu_mask(cpu, mask)   \
> +#define for_each_cpu_mask(cpu, mask)           \
> +       typecheck(struct cpumask *, (mask));    \
>        for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask)

Stuff like this is *WRONG*.

You now totally broke

  if (xyzzy)
    for_each_cpu_mask() {
    }

because you turned the loop into two separate statements.

If this is needed, it needs to be done right. You could have done it by doing

  #define for_each_cpu_mask(cpu, mask)           \
      for (typecheck(struct cpumask *, (mask)), (cpu) = 0; (cpu) < 1; (cpu)++)

or similar (I think you can drop the (void)(mask) because the variable
is now used by the typecheck macro).

                 Linus

Re: [RFC 3/3] kernel.h: use new typechecking macros in min()/max() and friends

[Linus Torvalds]

On Sat, Apr 14, 2012 at 3:14 PM, Sasha Levin <levinsasha928@gmail.com> wrote:
> Convert to using new typechecking macros in kernel.h. This prevents code
> duplication and makes the modified macros (easily) readable again.
>
>  #define min(x, y) ({                           \
> -       typeof(x) _min1 = (x);                  \
> -       typeof(y) _min2 = (y);                  \
> -       (void) (&_min1 == &_min2);              \
> -       _min1 < _min2 ? _min1 : _min2; })
> +       typecmp2((x), (y));                             \
> +       (x) < (y) ? (x) : (y); })

They may be readable, but they are total shit.

You now evaluate 'x' and 'y' multiple times, so if they are function
calls or contain other side effects (like "a++" etc), you get the
wrong result.

So no. Hell no.

                  Linus

Re: [RFC 0/3] Extend type checking macros

[Srivatsa S. Bhat]

On 04/15/2012 03:44 AM, Sasha Levin wrote:

> Commit e3831ed ("sched: Fix incorrect usage of for_each_cpu_mask() in
> select_fallback_rq()") fixes a very non obvious bug in select_fallback_rq()
> which was caused by passing 'struct cpumask' instead of 'struct cpumask *'
> to a macro in include/linux/cpumask.h
> 


Good heavens! I just found out that *each* *and* *every* *one* of the
existing 12 users of for_each_cpu_mask() are wrong!! Unbelievable!

> This bug was quite a pain to debug since it doesn't raise any warnings or
> erros during compilation, and the assumption of the kernel hackers who try
> to fix a bug is that if the compiler didn't complain, they passed the right
> types to functions.
> 
> This series of patches adds some more type checking macros to the forgotten
> include/linux/typecheck.h, it modified for_each_cpu_mask() to use those
> macros to trigger a warning when needed (this is a nice demonstration of how
> the bug mentioned before would have been visible with these checks), and
> modifies min()/max() and friend to use these macros as well to show their
> value in reducing duplicate code and improving readability.
> 
> Sasha Levin (3):
>   typecheck: extend typecheck.h with more useful typechecking macros
>   sched: add type checks to for_each_cpu_mask()


I think it would be better to correct and move the existing 12 users of
for_each_cpu_mask() to for_each_cpu() and simply get rid of the obsolete
for_each_cpu_mask() macro. After all, why do we need 2 macros that do the
exact same thing?

>   kernel.h: use new typechecking macros in min()/max() and friends
> 
>  include/linux/cpumask.h   |    4 ++-
>  include/linux/kernel.h    |   47 ++++++++++++++------------------------------
>  include/linux/typecheck.h |   42 ++++++++++++++++++++++++++++++++-------
>  3 files changed, 52 insertions(+), 41 deletions(-)
> 


Regards,
Srivatsa S. Bhat

[RFC 0/3] Extend type checking macros

[Sasha Levin]

Commit e3831ed ("sched: Fix incorrect usage of for_each_cpu_mask() in
select_fallback_rq()") fixes a very non obvious bug in select_fallback_rq()
which was caused by passing 'struct cpumask' instead of 'struct cpumask *'
to a macro in include/linux/cpumask.h

This bug was quite a pain to debug since it doesn't raise any warnings or
erros during compilation, and the assumption of the kernel hackers who try
to fix a bug is that if the compiler didn't complain, they passed the right
types to functions.

This series of patches adds some more type checking macros to the forgotten
include/linux/typecheck.h, it modified for_each_cpu_mask() to use those
macros to trigger a warning when needed (this is a nice demonstration of how
the bug mentioned before would have been visible with these checks), and
modifies min()/max() and friend to use these macros as well to show their
value in reducing duplicate code and improving readability.

Sasha Levin (3):
  typecheck: extend typecheck.h with more useful typechecking macros
  sched: add type checks to for_each_cpu_mask()
  kernel.h: use new typechecking macros in min()/max() and friends

 include/linux/cpumask.h   |    4 ++-
 include/linux/kernel.h    |   47 ++++++++++++++------------------------------
 include/linux/typecheck.h |   42 ++++++++++++++++++++++++++++++++-------
 3 files changed, 52 insertions(+), 41 deletions(-)

-- 
1.7.8.5

[RFC 1/3] typecheck: extend typecheck.h with more useful typechecking macros

[Sasha Levin]

Add macros to compare multiple parameters to a given type, and macros to
compare multiple parameters between themselves.

Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 include/linux/typecheck.h |   42 ++++++++++++++++++++++++++++++++++--------
 1 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/include/linux/typecheck.h b/include/linux/typecheck.h
index eb5b74a..a495dcb 100644
--- a/include/linux/typecheck.h
+++ b/include/linux/typecheck.h
@@ -5,20 +5,46 @@
  * Check at compile time that something is of a particular type.
  * Always evaluates to 1 so you may use it easily in comparisons.
  */
-#define typecheck(type,x) \
-({	type __dummy; \
-	typeof(x) __dummy2; \
-	(void)(&__dummy == &__dummy2); \
-	1; \
+#define typecheck(type,x)		\
+({	type __dummy;			\
+	typeof(x) __dummy2;		\
+	(void)(&__dummy == &__dummy2);	\
+	1;				\
+})
+
+#define typecheck2(type,x,y)		\
+({	typecheck(type, x);		\
+	typecheck(type, y);		\
+	1;				\
+})
+
+#define typecheck3(type,x,y,z)		\
+({	typecheck2(type,x,y);		\
+	typecheck(type,z);		\
+	1;				\
+})
+
+#define typecmp2(x,y)			\
+({	typeof(x) __x = (x);		\
+	typeof(y) __y = (y);		\
+	typecheck(typeof(x),x);		\
+	typecheck(typeof(y),y);		\
+	(void)(&__x==&__y);		\
+	1;				\
+})
+
+#define typecmp3(x,y,z)			\
+({	typecmp2((x),(y));			\
+	typecmp2((x),(z));			\
 })
 
 /*
  * Check at compile time that 'function' is a certain type, or is a pointer
  * to that type (needs to use typedef for the function type.)
  */
-#define typecheck_fn(type,function) \
-({	typeof(type) __tmp = function; \
-	(void)__tmp; \
+#define typecheck_fn(type,function)	\
+({	typeof(type) __tmp = function;	\
+	(void)__tmp;			\
 })
 
 #endif		/* TYPECHECK_H_INCLUDED */
-- 
1.7.8.5

[RFC 2/3] sched: add type checks to for_each_cpu_mask()

[Sasha Levin]

Add type checks to assert that 'mask' is 'struct cpumask *'. This check
would have detected the bug fixed in e3831ed ("sched: Fix incorrect usage
of for_each_cpu_mask() in select_fallback_rq()"):

kernel/sched/core.c: In function 'select_fallback_rq':
kernel/sched/core.c:1273:2: warning: comparison of distinct pointer types lacks a cast
kernel/sched/core.c:1284:3: warning: comparison of distinct pointer types lacks a cast

Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 include/linux/cpumask.h |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index a2c819d..58fd022 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -799,7 +799,8 @@ static inline const struct cpumask *get_cpu_mask(unsigned int cpu)
 #define first_cpu(src)		({ (void)(src); 0; })
 #define next_cpu(n, src)	({ (void)(src); 1; })
 #define any_online_cpu(mask)	0
-#define for_each_cpu_mask(cpu, mask)	\
+#define for_each_cpu_mask(cpu, mask)		\
+	typecheck(struct cpumask *, (mask));	\
 	for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask)
 #else /* NR_CPUS > 1 */
 int __first_cpu(const cpumask_t *srcp);
@@ -809,6 +810,7 @@ int __next_cpu(int n, const cpumask_t *srcp);
 #define next_cpu(n, src)	__next_cpu((n), &(src))
 #define any_online_cpu(mask) cpumask_any_and(&mask, cpu_online_mask)
 #define for_each_cpu_mask(cpu, mask)			\
+	typecheck(struct cpumask *, (mask));		\
 	for ((cpu) = -1;				\
 		(cpu) = next_cpu((cpu), (mask)),	\
 		(cpu) < NR_CPUS; )
-- 
1.7.8.5

[RFC 3/3] kernel.h: use new typechecking macros in min()/max() and friends

[Sasha Levin]

Convert to using new typechecking macros in kernel.h. This prevents code
duplication and makes the modified macros (easily) readable again.

Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 include/linux/kernel.h |   47 +++++++++++++++--------------------------------
 1 files changed, 15 insertions(+), 32 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 645231c..ec509cc 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -555,34 +555,22 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
  * "unnecessary" pointer comparison.
  */
 #define min(x, y) ({				\
-	typeof(x) _min1 = (x);			\
-	typeof(y) _min2 = (y);			\
-	(void) (&_min1 == &_min2);		\
-	_min1 < _min2 ? _min1 : _min2; })
+	typecmp2((x), (y));				\
+	(x) < (y) ? (x) : (y); })
 
 #define max(x, y) ({				\
-	typeof(x) _max1 = (x);			\
-	typeof(y) _max2 = (y);			\
-	(void) (&_max1 == &_max2);		\
-	_max1 > _max2 ? _max1 : _max2; })
+	typecmp2((x), (y));				\
+	(x) > (y) ? (x) : (y); }) 
 
 #define min3(x, y, z) ({			\
-	typeof(x) _min1 = (x);			\
-	typeof(y) _min2 = (y);			\
-	typeof(z) _min3 = (z);			\
-	(void) (&_min1 == &_min2);		\
-	(void) (&_min1 == &_min3);		\
-	_min1 < _min2 ? (_min1 < _min3 ? _min1 : _min3) : \
-		(_min2 < _min3 ? _min2 : _min3); })
+	typecmp3((x), (y), (z));			\
+	(x) < (y) ? ((x) < (z) ? (x) : (z)) :	\
+	((y) < (z) ? (y) : (z)); })
 
 #define max3(x, y, z) ({			\
-	typeof(x) _max1 = (x);			\
-	typeof(y) _max2 = (y);			\
-	typeof(z) _max3 = (z);			\
-	(void) (&_max1 == &_max2);		\
-	(void) (&_max1 == &_max3);		\
-	_max1 > _max2 ? (_max1 > _max3 ? _max1 : _max3) : \
-		(_max2 > _max3 ? _max2 : _max3); })
+	typecmp3((x), (y), (z));                      \
+	(x) > (y) ? ((x) > (z) ? (x) : (z)) :   \
+		((y) > (z) ? (y) : (z)); })
 
 /**
  * min_not_zero - return the minimum that is _not_ zero, unless both are zero
@@ -590,9 +578,8 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
  * @y: value2
  */
 #define min_not_zero(x, y) ({			\
-	typeof(x) __x = (x);			\
-	typeof(y) __y = (y);			\
-	__x == 0 ? __y : ((__y == 0) ? __x : min(__x, __y)); })
+	typecmp2((x), (y));			\
+	(x) == 0 ? (y) : ((y) == 0 ? (x) : min((x), (y))); })
 
 /**
  * clamp - return a value clamped to a given range with strict typechecking
@@ -604,13 +591,9 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
  * same type as val.  See the unnecessary pointer comparisons.
  */
 #define clamp(val, min, max) ({			\
-	typeof(val) __val = (val);		\
-	typeof(min) __min = (min);		\
-	typeof(max) __max = (max);		\
-	(void) (&__val == &__min);		\
-	(void) (&__val == &__max);		\
-	__val = __val < __min ? __min: __val;	\
-	__val > __max ? __max: __val; })
+	typecmp3((val), (min), (max));		\
+	((val) > (max) ? (max) : (val)) < (min) ? \
+	(min) : ((val) > (max) ? (max) : (val)); })
 
 /*
  * ..and if you can't take the strict
-- 
1.7.8.5

Re: [RFC 0/3] Extend type checking macros

[Srivatsa S. Bhat]

On 04/15/2012 02:48 AM, Srivatsa S. Bhat wrote:

> On 04/15/2012 03:44 AM, Sasha Levin wrote:
> 
>> Commit e3831ed ("sched: Fix incorrect usage of for_each_cpu_mask() in
>> select_fallback_rq()") fixes a very non obvious bug in select_fallback_rq()
>> which was caused by passing 'struct cpumask' instead of 'struct cpumask *'
>> to a macro in include/linux/cpumask.h
>>
> 
> 
> Good heavens! I just found out that *each* *and* *every* *one* of the
> existing 12 users of for_each_cpu_mask() are wrong!! Unbelievable!
> 


Never mind.. I think I was blind! Gah, having 2 helpers to do the same
thing is confusing, no doubt!

So, looking again, for_each_cpu_mask() does expect the second value to
be struct cpumask, and not struct cpumask *. Which means, we saw the
warning (http://thread.gmane.org/gmane.linux.kernel/1274579/) for an
entirely different reason than what the changelog of commit e3831ed says!

Looking further, the problem is fairly simple:
for_each_cpu() and friends (eg: cpumask_next) cap at nr_cpu_ids.
for_each_cpu_mask() and friends (eg: __next_cpu) cap at NR_CPUS.

And cpumask_test_cpu() that was used in select_fallback_rq() essentially
boils down to cpumask_check(), which uses nr_cpumask_bits as the cap,
which can be either nr_cpu_ids or NR_CPUS depending on whether we have
CONFIG_CPUMASK_OFFSTACK set or unset.

IOW, we got the warning because we mixed up NR_CPUS and nr_cpu_ids.
And commit e3831ed fixed it because it properly matched the functions
used, so that they cap at, and check for the same value; and not because
of a mismatch between the type of arguments expected vs sent to
for_each_cpu_mask(), as its changelog states.
(No wonder Peter's code compiled without problems...)

One more thing: the old interface decides between nr_cpu_ids vs NR_CPUS
depending on whether NR_CPUS > 64, whereas the new interface decides
based on the config option - CPUMASK_OFFSTACK.
All in all, lots of confusion and incompatibility.

So I think we need to standardize the values - use nr_cpumask_bits
everywhere (since it can become nr_cpu_ids or NR_CPUS depending on our
config options), and be done with it. Then we wouldn't have such
mismatches and bugs.

Don't know how many more places have got these mixed up.. All the more
reason to get rid of for_each_cpu_mask(), if you ask me..

Regards,
Srivatsa S. Bhat

Re: [RFC 2/3] sched: add type checks to for_each_cpu_mask()

[Andrew Morton]

On Sat, 14 Apr 2012 18:14:44 -0400
Sasha Levin <levinsasha928@gmail.com> wrote:

> Add type checks to assert that 'mask' is 'struct cpumask *'. This check
> would have detected the bug fixed in e3831ed ("sched: Fix incorrect usage
> of for_each_cpu_mask() in select_fallback_rq()"):
> 
> kernel/sched/core.c: In function 'select_fallback_rq':
> kernel/sched/core.c:1273:2: warning: comparison of distinct pointer types lacks a cast
> kernel/sched/core.c:1284:3: warning: comparison of distinct pointer types lacks a cast
> 
> ...
>
> @@ -809,6 +810,7 @@ int __next_cpu(int n, const cpumask_t *srcp);
>  #define next_cpu(n, src)	__next_cpu((n), &(src))
>  #define any_online_cpu(mask) cpumask_any_and(&mask, cpu_online_mask)
>  #define for_each_cpu_mask(cpu, mask)			\
> +	typecheck(struct cpumask *, (mask));		\
>  	for ((cpu) = -1;				\
>  		(cpu) = next_cpu((cpu), (mask)),	\
>  		(cpu) < NR_CPUS; )

and int __next_cpu(int n, const cpumask_t *srcp);

I'm mystified.  Why didn't the next_cpu() call generate a warning when
passed the wrong type?