Strange code in include/linux/cpumask.h

Strange code in include/linux/cpumask.h

[Nikanth Karthikesan]

Hi Rusty

I do not understand this code.

/**
 * to_cpumask - convert an NR_CPUS bitmap to a struct cpumask *
 * @bitmap: the bitmap
 *
 * There are a few places where cpumask_var_t isn't appropriate and
 * static cpumasks must be used (eg. very early boot), yet we don't
 * expose the definition of 'struct cpumask'.
 *
 * This does the conversion, and can be used as a constant initializer.
 */
#define to_cpumask(bitmap)                                              \
        ((struct cpumask *)(1 ? (bitmap)                                \
                            : (void *)sizeof(__check_is_bitmap(bitmap))))

static inline int __check_is_bitmap(const unsigned long *bitmap)
{
        return 1;
}

The conditional operator would always evaluates to true and return bitmap. So 
all it seems to does is
#define to_cpumansk(bitmap) (struct cpumask *)(bitmap)

Even If it would return (void *)sizeof(__check_is_bitmap(bitmap)), wouldn't it 
be always
(struct cpumask *) (sizeof(1))!?

Is this some magic for type safety?

Thanks
Nikanth

Re: Strange code in include/linux/cpumask.h

[Rusty Russell]

On Wednesday 25 March 2009 15:21:13 Nikanth Karthikesan wrote:
> Hi Rusty
> 
> I do not understand this code.

Hi Nikanth,

   That's OK, it's a little tricky.

> The conditional operator would always evaluates to true and return bitmap. So 
> all it seems to does is
> #define to_cpumansk(bitmap) (struct cpumask *)(bitmap)

Yes, except that this insists that bitmap be an unsigned long * or you'll get
a warning.  Otherwise the macro could be used on anything.  And it needs to be
a macro to use it as a static initializer.

As the NR_CPUS bit arrays vanish, this macro will be used less and less; but
some of them will probably take a while.

Thanks for the question,
Rusty.

Re: Strange code in include/linux/cpumask.h

[Nikanth Karthikesan]

On Wednesday 25 March 2009 10:41:11 Rusty Russell wrote:
> On Wednesday 25 March 2009 15:21:13 Nikanth Karthikesan wrote:
> > Hi Rusty
> >
> > I do not understand this code.
>
> Hi Nikanth,
>
>    That's OK, it's a little tricky.
>
> > The conditional operator would always evaluates to true and return
> > bitmap. So all it seems to does is
> > #define to_cpumansk(bitmap) (struct cpumask *)(bitmap)
>
> Yes, except that this insists that bitmap be an unsigned long * or you'll
> get a warning.  Otherwise the macro could be used on anything.  And it
> needs to be a macro to use it as a static initializer.
>

Ah, got it. Thanks a lot for the explanation. May be a comment could be added 
to the source.

So if only type-checking is required, something like this should work.

diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index 9f31538..7857f8f 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -282,11 +282,11 @@ static inline void __cpus_shift_left(cpumask_t *dstp,
  */
 #define to_cpumask(bitmap)						\
 	((struct cpumask *)(1 ? (bitmap)				\
-			    : (void *)sizeof(__check_is_bitmap(bitmap))))
+			    : __check_is_bitmap(bitmap)))
 
-static inline int __check_is_bitmap(const unsigned long *bitmap)
+static inline void * __check_is_bitmap(const unsigned long *bitmap)
 {
-	return 1;
+	return 0;
 }
 
 /*

And for static initialization + type checking, will this work?

diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index 9f31538..d86b8d0 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -282,11 +282,11 @@ static inline void __cpus_shift_left(cpumask_t *dstp,
  */
 #define to_cpumask(bitmap)						\
 	((struct cpumask *)(1 ? (bitmap)				\
-			    : (void *)sizeof(__check_is_bitmap(bitmap))))
+			    :( __check_is_bitmap(bitmap),(void *)0)))
 
-static inline int __check_is_bitmap(const unsigned long *bitmap)
+static inline void * __check_is_bitmap(const unsigned long *bitmap)
 {
-	return 1;
+	return 0;
 }
 
 /*

>
> As the NR_CPUS bit arrays vanish, this macro will be used less and less;
> but some of them will probably take a while.
>

Also looks like, this is not being used as a static initializer anywhere. 
 i.e., Using my type-checking only version didn't trigger any error/warnings! 
Being a deprecated interface, no new users are expected? Or is gcc smart, not 
complaining when used as static initializer, as it would always evaluate to 
(struct cpumask *)(bitmap)? Can the sizeof constification be removed?

Thanks
Nikanth

Re: Strange code in include/linux/cpumask.h

[Rusty Russell]

On Wednesday 25 March 2009 17:14:30 Nikanth Karthikesan wrote:
> On Wednesday 25 March 2009 10:41:11 Rusty Russell wrote:
> > Yes, except that this insists that bitmap be an unsigned long * or you'll
> > get a warning.  Otherwise the macro could be used on anything.  And it
> > needs to be a macro to use it as a static initializer.
> 
> Ah, got it. Thanks a lot for the explanation. May be a comment could be added 
> to the source.

Well, it's not that unusual a trick in the kernel, but an explanation might
help.

> Also looks like, this is not being used as a static initializer anywhere. 
>  i.e., Using my type-checking only version didn't trigger any error/warnings! 

It will.  cpu_all_mask uses to_cpumask, and it's replacing CPU_MASK_ALL.

> Being a deprecated interface, no new users are expected? Or is gcc smart, not 
> complaining when used as static initializer, as it would always evaluate to 
> (struct cpumask *)(bitmap)?

Yes, it seems to be.  AFAICT though, using a function in an initializer is not
valid C.

> Can the sizeof constification be removed?

Possibly, but it makes me nervous to rely on it.

Thanks,
Rusty.


> 
> Thanks
> Nikanth
> 

Re: Strange code in include/linux/cpumask.h

[Nikanth Karthikesan]

On Thursday 26 March 2009 04:14:46 Rusty Russell wrote:
> On Wednesday 25 March 2009 17:14:30 Nikanth Karthikesan wrote:
> > On Wednesday 25 March 2009 10:41:11 Rusty Russell wrote:
> > > Yes, except that this insists that bitmap be an unsigned long * or
> > > you'll get a warning.  Otherwise the macro could be used on anything. 
> > > And it needs to be a macro to use it as a static initializer.
> >
> > Ah, got it. Thanks a lot for the explanation. May be a comment could be
> > added to the source.
>
> Well, it's not that unusual a trick in the kernel, but an explanation might
> help.
>

I _assumed_ that kernel developers are good programmers who do not need type
enforcements/warnings. And tricky code for non-performance optimizations wont
be allowed. Yes, this is not very tricky once you are aware that such tricks
are done in the kernel just for type-safety. Anyway lets document it.

Thanks
Nikanth


Add a comment explaining the purpose of the tricky code in the macro
to_cpumask(). The __check_is_bitmap() is used to enforce type-safety.

Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>

---

diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index 9f31538..32b802b 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -279,6 +279,7 @@ static inline void __cpus_shift_left(cpumask_t *dstp,
  * expose the definition of 'struct cpumask'.
  *
  * This does the conversion, and can be used as a constant initializer.
+ * __check_is_bitmap() enforces bitmap to be an 'unsigned long *'.
  */
 #define to_cpumask(bitmap)						\
 	((struct cpumask *)(1 ? (bitmap)				\