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
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.
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
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
>
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 <[email protected]>
---
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) \