[PATCH 1/1] x86: Add check for node passed to node_to_cpumask

[PATCH 1/1] x86: Add check for node passed to node_to_cpumask

[Mike Travis]

Subject: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask

  * When CONFIG_DEBUG_PER_CPU_MAPS is set, the node passed to
    node_to_cpumask and node_to_cpumask_ptr should be validated.

Based on "Thu Jun 26 09:23:55 PDT 2008" tip/master... ;-)

Signed-off-by: Mike Travis <travis@sgi.com>
---
 arch/x86/kernel/setup.c    |   26 +++++++++++++++++++++++---
 include/asm-x86/topology.h |    7 ++++++-
 2 files changed, 29 insertions(+), 4 deletions(-)

--- linux-2.6.tip.orig/arch/x86/kernel/setup.c
+++ linux-2.6.tip/arch/x86/kernel/setup.c
@@ -377,6 +377,10 @@ int early_cpu_to_node(int cpu)
 	return per_cpu(x86_cpu_to_node_map, cpu);
 }
 
+
+/* empty cpumask */
+static const cpumask_t cpu_mask_none;
+
 /*
  * Returns a pointer to the bitmask of CPUs on Node 'node'.
  */
@@ -389,13 +393,23 @@ cpumask_t *_node_to_cpumask_ptr(int node
 		dump_stack();
 		return &cpu_online_map;
 	}
-	BUG_ON(node >= nr_node_ids);
-	return &node_to_cpumask_map[node];
+	if (node >= nr_node_ids) {
+		printk(KERN_WARNING
+			"_node_to_cpumask_ptr(%d): node > nr_node_ids(%d)\n",
+			node, nr_node_ids);
+		dump_stack();
+		return &cpu_mask_none;
+	}
+	return (cpumask_t *)&node_to_cpumask_map[node];
 }
 EXPORT_SYMBOL(_node_to_cpumask_ptr);
 
 /*
  * Returns a bitmask of CPUs on Node 'node'.
+ *
+ * Side note: this function creates the returned cpumask on the stack
+ * so with a high NR_CPUS count, excessive stack space is used.  The
+ * node_to_cpumask_ptr function should be used whenever possible.
  */
 cpumask_t node_to_cpumask(int node)
 {
@@ -405,7 +419,13 @@ cpumask_t node_to_cpumask(int node)
 		dump_stack();
 		return cpu_online_map;
 	}
-	BUG_ON(node >= nr_node_ids);
+	if (node >= nr_node_ids) {
+		printk(KERN_WARNING
+			"node_to_cpumask(%d): node > nr_node_ids(%d)\n",
+			node, nr_node_ids);
+		dump_stack();
+		return cpu_mask_none;
+	}
 	return node_to_cpumask_map[node];
 }
 EXPORT_SYMBOL(node_to_cpumask);
--- linux-2.6.tip.orig/include/asm-x86/topology.h
+++ linux-2.6.tip/include/asm-x86/topology.h
@@ -57,7 +57,12 @@ static inline int cpu_to_node(int cpu)
 }
 #define early_cpu_to_node(cpu)	cpu_to_node(cpu)
 
-/* Returns a bitmask of CPUs on Node 'node'. */
+/* Returns a bitmask of CPUs on Node 'node'.
+ *
+ * Side note: this function creates the returned cpumask on the stack
+ * so with a high NR_CPUS count, excessive stack space is used.  The
+ * node_to_cpumask_ptr function should be used whenever possible.
+ */
 static inline cpumask_t node_to_cpumask(int node)
 {
 	return node_to_cpumask_map[node];

Re: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V2

[Mike Travis]

Subject: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V2

V2: Slightly different version to remove a compiler warning.

  * When CONFIG_DEBUG_PER_CPU_MAPS is set, the node passed to
      node_to_cpumask and node_to_cpumask_ptr should be validated.

Based on "Thu Jun 26 09:23:55 PDT 2008" tip/master...  ;-) 

Signed-off-by: Mike Travis <travis@sgi.com>
---
 arch/x86/kernel/setup.c    |   26 +++++++++++++++++++++++---
 include/asm-x86/topology.h |    7 ++++++-
 2 files changed, 29 insertions(+), 4 deletions(-)

--- linux-2.6.tip.orig/arch/x86/kernel/setup.c
+++ linux-2.6.tip/arch/x86/kernel/setup.c
@@ -377,6 +377,10 @@ int early_cpu_to_node(int cpu)
 	return per_cpu(x86_cpu_to_node_map, cpu);
 }
 
+
+/* empty cpumask */
+static const cpumask_t cpu_mask_none;
+
 /*
  * Returns a pointer to the bitmask of CPUs on Node 'node'.
  */
@@ -389,13 +393,23 @@ cpumask_t *_node_to_cpumask_ptr(int node
 		dump_stack();
 		return &cpu_online_map;
 	}
-	BUG_ON(node >= nr_node_ids);
-	return &node_to_cpumask_map[node];
+	if (node >= nr_node_ids) {
+		printk(KERN_WARNING
+			"_node_to_cpumask_ptr(%d): node > nr_node_ids(%d)\n",
+			node, nr_node_ids);
+		dump_stack();
+		return (cpumask_t *)&cpu_mask_none;
+	}
+	return (cpumask_t *)&node_to_cpumask_map[node];
 }
 EXPORT_SYMBOL(_node_to_cpumask_ptr);
 
 /*
  * Returns a bitmask of CPUs on Node 'node'.
+ *
+ * Side note: this function creates the returned cpumask on the stack
+ * so with a high NR_CPUS count, excessive stack space is used.  The
+ * node_to_cpumask_ptr function should be used whenever possible.
  */
 cpumask_t node_to_cpumask(int node)
 {
@@ -405,7 +419,13 @@ cpumask_t node_to_cpumask(int node)
 		dump_stack();
 		return cpu_online_map;
 	}
-	BUG_ON(node >= nr_node_ids);
+	if (node >= nr_node_ids) {
+		printk(KERN_WARNING
+			"node_to_cpumask(%d): node > nr_node_ids(%d)\n",
+			node, nr_node_ids);
+		dump_stack();
+		return cpu_mask_none;
+	}
 	return node_to_cpumask_map[node];
 }
 EXPORT_SYMBOL(node_to_cpumask);
--- linux-2.6.tip.orig/include/asm-x86/topology.h
+++ linux-2.6.tip/include/asm-x86/topology.h
@@ -57,7 +57,12 @@ static inline int cpu_to_node(int cpu)
 }
 #define early_cpu_to_node(cpu)	cpu_to_node(cpu)
 
-/* Returns a bitmask of CPUs on Node 'node'. */
+/* Returns a bitmask of CPUs on Node 'node'.
+ *
+ * Side note: this function creates the returned cpumask on the stack
+ * so with a high NR_CPUS count, excessive stack space is used.  The
+ * node_to_cpumask_ptr function should be used whenever possible.
+ */
 static inline cpumask_t node_to_cpumask(int node)
 {
 	return node_to_cpumask_map[node];

Re: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3

[Mike Travis]

Subject: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3

  * When CONFIG_DEBUG_PER_CPU_MAPS is set, the node passed to
    node_to_cpumask and node_to_cpumask_ptr should be validated.
    If invalid, then a dump_stack is performed and a zero cpumask
    is returned.

Based on "Fri Jun 27 10:06:06 PDT 2008" tip/master...  ;-) 

Signed-off-by: Mike Travis <travis@sgi.com>
---
V2: Slightly different version to remove a compiler warning.
V3: Redone to reflect moving setup.c -> setup_percpu.c
---
 arch/x86/kernel/setup_percpu.c |   26 +++++++++++++++++++++++---
 include/asm-x86/topology.h     |    7 ++++++-
 2 files changed, 29 insertions(+), 4 deletions(-)

--- linux-2.6.tip.orig/arch/x86/kernel/setup_percpu.c
+++ linux-2.6.tip/arch/x86/kernel/setup_percpu.c
@@ -346,6 +346,10 @@ int early_cpu_to_node(int cpu)
 	return per_cpu(x86_cpu_to_node_map, cpu);
 }
 
+
+/* empty cpumask */
+static const cpumask_t cpu_mask_none;
+
 /*
  * Returns a pointer to the bitmask of CPUs on Node 'node'.
  */
@@ -358,13 +362,23 @@ cpumask_t *_node_to_cpumask_ptr(int node
 		dump_stack();
 		return &cpu_online_map;
 	}
-	BUG_ON(node >= nr_node_ids);
-	return &node_to_cpumask_map[node];
+	if (node >= nr_node_ids) {
+		printk(KERN_WARNING
+			"_node_to_cpumask_ptr(%d): node > nr_node_ids(%d)\n",
+			node, nr_node_ids);
+		dump_stack();
+		return (cpumask_t *)&cpu_mask_none;
+	}
+	return (cpumask_t *)&node_to_cpumask_map[node];
 }
 EXPORT_SYMBOL(_node_to_cpumask_ptr);
 
 /*
  * Returns a bitmask of CPUs on Node 'node'.
+ *
+ * Side note: this function creates the returned cpumask on the stack
+ * so with a high NR_CPUS count, excessive stack space is used.  The
+ * node_to_cpumask_ptr function should be used whenever possible.
  */
 cpumask_t node_to_cpumask(int node)
 {
@@ -374,7 +388,13 @@ cpumask_t node_to_cpumask(int node)
 		dump_stack();
 		return cpu_online_map;
 	}
-	BUG_ON(node >= nr_node_ids);
+	if (node >= nr_node_ids) {
+		printk(KERN_WARNING
+			"node_to_cpumask(%d): node > nr_node_ids(%d)\n",
+			node, nr_node_ids);
+		dump_stack();
+		return cpu_mask_none;
+	}
 	return node_to_cpumask_map[node];
 }
 EXPORT_SYMBOL(node_to_cpumask);
--- linux-2.6.tip.orig/include/asm-x86/topology.h
+++ linux-2.6.tip/include/asm-x86/topology.h
@@ -57,7 +57,12 @@ static inline int cpu_to_node(int cpu)
 }
 #define early_cpu_to_node(cpu)	cpu_to_node(cpu)
 
-/* Returns a bitmask of CPUs on Node 'node'. */
+/* Returns a bitmask of CPUs on Node 'node'.
+ *
+ * Side note: this function creates the returned cpumask on the stack
+ * so with a high NR_CPUS count, excessive stack space is used.  The
+ * node_to_cpumask_ptr function should be used whenever possible.
+ */
 static inline cpumask_t node_to_cpumask(int node)
 {
 	return node_to_cpumask_map[node];

Re: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3

[Vegard Nossum]

On Fri, Jun 27, 2008 at 7:10 PM, Mike Travis <travis@sgi.com> wrote:
> Subject: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3
>
>  * When CONFIG_DEBUG_PER_CPU_MAPS is set, the node passed to
>    node_to_cpumask and node_to_cpumask_ptr should be validated.
>    If invalid, then a dump_stack is performed and a zero cpumask
>    is returned.
>
> Based on "Fri Jun 27 10:06:06 PDT 2008" tip/master...  ;-)
>
> Signed-off-by: Mike Travis <travis@sgi.com>
> ---
> V2: Slightly different version to remove a compiler warning.
> V3: Redone to reflect moving setup.c -> setup_percpu.c
> ---
>  arch/x86/kernel/setup_percpu.c |   26 +++++++++++++++++++++++---
>  include/asm-x86/topology.h     |    7 ++++++-
>  2 files changed, 29 insertions(+), 4 deletions(-)
>
> --- linux-2.6.tip.orig/arch/x86/kernel/setup_percpu.c
> +++ linux-2.6.tip/arch/x86/kernel/setup_percpu.c
> @@ -346,6 +346,10 @@ int early_cpu_to_node(int cpu)
>        return per_cpu(x86_cpu_to_node_map, cpu);
>  }
>
> +
> +/* empty cpumask */
> +static const cpumask_t cpu_mask_none;
> +
>  /*
>  * Returns a pointer to the bitmask of CPUs on Node 'node'.
>  */
> @@ -358,13 +362,23 @@ cpumask_t *_node_to_cpumask_ptr(int node
>                dump_stack();
>                return &cpu_online_map;
>        }
> -       BUG_ON(node >= nr_node_ids);
> -       return &node_to_cpumask_map[node];
> +       if (node >= nr_node_ids) {
> +               printk(KERN_WARNING
> +                       "_node_to_cpumask_ptr(%d): node > nr_node_ids(%d)\n",
> +                       node, nr_node_ids);
> +               dump_stack();
> +               return (cpumask_t *)&cpu_mask_none;

Hm, I am wondering about this.

There exists an option DEBUG_RODATA ("Write protect kernel read-only
data structures"). I'm guessing this RO protections means that we'll
get page faults (and thus BUG/panic) if this "none" mask is ever
attempted to be changed.

So if CONFIG_DEBUG_RODATA=y, I believe we'll see first this warning,
then a panic (after all). Would it be better to make cpu_mask_none
non-const, in spirit of trying to continue as far as possible? I don't
really know if it matters, though. It seems that fedora kernels at
least ship with a default of DEBUG_RODATA=y.

What you could also do is to make it non-const and then zero it each
time it is requested. Again, this is an error situation in either
case, so maybe it's not worth fussing about.


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3

[Mike Travis]

Vegard Nossum wrote:
> On Fri, Jun 27, 2008 at 7:10 PM, Mike Travis <travis@sgi.com> wrote:
>> Subject: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3
>>
>>  * When CONFIG_DEBUG_PER_CPU_MAPS is set, the node passed to
>>    node_to_cpumask and node_to_cpumask_ptr should be validated.
>>    If invalid, then a dump_stack is performed and a zero cpumask
>>    is returned.
>>
>> Based on "Fri Jun 27 10:06:06 PDT 2008" tip/master...  ;-)
>>
>> Signed-off-by: Mike Travis <travis@sgi.com>
>> ---
>> V2: Slightly different version to remove a compiler warning.
>> V3: Redone to reflect moving setup.c -> setup_percpu.c
>> ---
>>  arch/x86/kernel/setup_percpu.c |   26 +++++++++++++++++++++++---
>>  include/asm-x86/topology.h     |    7 ++++++-
>>  2 files changed, 29 insertions(+), 4 deletions(-)
>>
>> --- linux-2.6.tip.orig/arch/x86/kernel/setup_percpu.c
>> +++ linux-2.6.tip/arch/x86/kernel/setup_percpu.c
>> @@ -346,6 +346,10 @@ int early_cpu_to_node(int cpu)
>>        return per_cpu(x86_cpu_to_node_map, cpu);
>>  }
>>
>> +
>> +/* empty cpumask */
>> +static const cpumask_t cpu_mask_none;
>> +
>>  /*
>>  * Returns a pointer to the bitmask of CPUs on Node 'node'.
>>  */
>> @@ -358,13 +362,23 @@ cpumask_t *_node_to_cpumask_ptr(int node
>>                dump_stack();
>>                return &cpu_online_map;
>>        }
>> -       BUG_ON(node >= nr_node_ids);
>> -       return &node_to_cpumask_map[node];
>> +       if (node >= nr_node_ids) {
>> +               printk(KERN_WARNING
>> +                       "_node_to_cpumask_ptr(%d): node > nr_node_ids(%d)\n",
>> +                       node, nr_node_ids);
>> +               dump_stack();
>> +               return (cpumask_t *)&cpu_mask_none;
> 
> Hm, I am wondering about this.
> 
> There exists an option DEBUG_RODATA ("Write protect kernel read-only
> data structures"). I'm guessing this RO protections means that we'll
> get page faults (and thus BUG/panic) if this "none" mask is ever
> attempted to be changed.
> 
> So if CONFIG_DEBUG_RODATA=y, I believe we'll see first this warning,
> then a panic (after all). Would it be better to make cpu_mask_none
> non-const, in spirit of trying to continue as far as possible? I don't
> really know if it matters, though. It seems that fedora kernels at
> least ship with a default of DEBUG_RODATA=y.
> 
> What you could also do is to make it non-const and then zero it each
> time it is requested. Again, this is an error situation in either
> case, so maybe it's not worth fussing about.
> 
> 
> Vegard
> 

Ingo asked me to add the "const".  But it would be a mistake to use
node_to_cpumask_ptr to modify the map... numa_set/clear_node is the
proper way to modify the maps.  Hmm, maybe I should add "const" to
the real node_to_cpumask_ptr function, at least then the compiler
would help spot illegal usages.

Thanks,
Mike

Re: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3

[Vegard Nossum]

On Fri, Jun 27, 2008 at 8:03 PM, Mike Travis <travis@sgi.com> wrote:
>> So if CONFIG_DEBUG_RODATA=y, I believe we'll see first this warning,
>> then a panic (after all). Would it be better to make cpu_mask_none
>> non-const, in spirit of trying to continue as far as possible? I don't
>> really know if it matters, though. It seems that fedora kernels at
>> least ship with a default of DEBUG_RODATA=y.
>>

...

> Ingo asked me to add the "const".  But it would be a mistake to use
> node_to_cpumask_ptr to modify the map... numa_set/clear_node is the
> proper way to modify the maps.  Hmm, maybe I should add "const" to
> the real node_to_cpumask_ptr function, at least then the compiler
> would help spot illegal usages.

That sounds like a brilliant idea, as long as none of the callers
expect it to be non-const.

I changed it to return const to see, and built allmodconfig + a few
randconfigs without error (well, warning). There are not that many
users of this function anyway.

BTW, what's up with the topology.h/setup_percpu.c mismatch, when
topology.c exists as well?


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3

[Mike Travis]

Vegard Nossum wrote:
> On Fri, Jun 27, 2008 at 8:03 PM, Mike Travis <travis@sgi.com> wrote:
>>> So if CONFIG_DEBUG_RODATA=y, I believe we'll see first this warning,
>>> then a panic (after all). Would it be better to make cpu_mask_none
>>> non-const, in spirit of trying to continue as far as possible? I don't
>>> really know if it matters, though. It seems that fedora kernels at
>>> least ship with a default of DEBUG_RODATA=y.
>>>
> 
> ...
> 
>> Ingo asked me to add the "const".  But it would be a mistake to use
>> node_to_cpumask_ptr to modify the map... numa_set/clear_node is the
>> proper way to modify the maps.  Hmm, maybe I should add "const" to
>> the real node_to_cpumask_ptr function, at least then the compiler
>> would help spot illegal usages.
> 
> That sounds like a brilliant idea, as long as none of the callers
> expect it to be non-const.

Being that I modified all the callers to use this new function, I did
verify that each caller expected it to be read only... ;-)
> 
> I changed it to return const to see, and built allmodconfig + a few
> randconfigs without error (well, warning). There are not that many
> users of this function anyway.

Did any violate the read-only nature of the call?
> 
> BTW, what's up with the topology.h/setup_percpu.c mismatch, when
> topology.c exists as well?

Topology.h supplies the generic usage which is to define a local cpumask
variable, fill it, and then return a pointer to it.  This is for arch's
that do not maintain a separate node_to_cpumask_map[] but allows a
compatible call interface.
> 
> 
> Vegard
> 

Thanks!
Mike

Re: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3

[Ingo Molnar]

* Mike Travis <travis@sgi.com> wrote:

> Subject: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3
> 
>   * When CONFIG_DEBUG_PER_CPU_MAPS is set, the node passed to
>     node_to_cpumask and node_to_cpumask_ptr should be validated.
>     If invalid, then a dump_stack is performed and a zero cpumask
>     is returned.
> 
> Based on "Fri Jun 27 10:06:06 PDT 2008" tip/master...  ;-) 
> 
> Signed-off-by: Mike Travis <travis@sgi.com>
> ---
> V2: Slightly different version to remove a compiler warning.
> V3: Redone to reflect moving setup.c -> setup_percpu.c

applied to tip/x86/unify-setup - thanks Mike.

Vegard, can i add your Acked-by too?

	Ingo

Re: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3

[Vegard Nossum]

On Thu, Jul 3, 2008 at 10:44 AM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Mike Travis <travis@sgi.com> wrote:
>
>> Subject: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3
>>
>>   * When CONFIG_DEBUG_PER_CPU_MAPS is set, the node passed to
>>     node_to_cpumask and node_to_cpumask_ptr should be validated.
>>     If invalid, then a dump_stack is performed and a zero cpumask
>>     is returned.
>>
>> Based on "Fri Jun 27 10:06:06 PDT 2008" tip/master...  ;-)
>>
>> Signed-off-by: Mike Travis <travis@sgi.com>
>> ---
>> V2: Slightly different version to remove a compiler warning.
>> V3: Redone to reflect moving setup.c -> setup_percpu.c
>
> applied to tip/x86/unify-setup - thanks Mike.
>
> Vegard, can i add your Acked-by too?

To be honest, I'd prefer that the function returns a const pointer.
Mike and I have both reviewed all callers independently and concluded
that there is no problem in doing this, and that, in fact, this is the
correct way to deal with it.

So if Mike submits a V4 with this const return type, or another patch
on top of this one, I'll ack it :-)


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3

[Ingo Molnar]

* Vegard Nossum <vegard.nossum@gmail.com> wrote:

> On Thu, Jul 3, 2008 at 10:44 AM, Ingo Molnar <mingo@elte.hu> wrote:
> >
> > * Mike Travis <travis@sgi.com> wrote:
> >
> >> Subject: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3
> >>
> >>   * When CONFIG_DEBUG_PER_CPU_MAPS is set, the node passed to
> >>     node_to_cpumask and node_to_cpumask_ptr should be validated.
> >>     If invalid, then a dump_stack is performed and a zero cpumask
> >>     is returned.
> >>
> >> Based on "Fri Jun 27 10:06:06 PDT 2008" tip/master...  ;-)
> >>
> >> Signed-off-by: Mike Travis <travis@sgi.com>
> >> ---
> >> V2: Slightly different version to remove a compiler warning.
> >> V3: Redone to reflect moving setup.c -> setup_percpu.c
> >
> > applied to tip/x86/unify-setup - thanks Mike.
> >
> > Vegard, can i add your Acked-by too?
> 
> To be honest, I'd prefer that the function returns a const pointer. 
> Mike and I have both reviewed all callers independently and concluded 
> that there is no problem in doing this, and that, in fact, this is the 
> correct way to deal with it.
> 
> So if Mike submits a V4 with this const return type, or another patch 
> on top of this one, I'll ack it :-)

ok, i'll wait for v4 :)

(v3 is applied already so Mike please send a delta to v3.)

	Ingo

[PATCH 1/1] x86: Change _node_to_cpumask_ptr to return const ptr

[Mike Travis]

Ingo Molnar wrote:
> * Vegard Nossum <vegard.nossum@gmail.com> wrote:
> 
>> On Thu, Jul 3, 2008 at 10:44 AM, Ingo Molnar <mingo@elte.hu> wrote:
>>> * Mike Travis <travis@sgi.com> wrote:
>>>
>>>> Subject: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3
>>>>
>>>>   * When CONFIG_DEBUG_PER_CPU_MAPS is set, the node passed to
>>>>     node_to_cpumask and node_to_cpumask_ptr should be validated.
>>>>     If invalid, then a dump_stack is performed and a zero cpumask
>>>>     is returned.
>>>>
>>>> Based on "Fri Jun 27 10:06:06 PDT 2008" tip/master...  ;-)
>>>>
>>>> Signed-off-by: Mike Travis <travis@sgi.com>
>>>> ---
>>>> V2: Slightly different version to remove a compiler warning.
>>>> V3: Redone to reflect moving setup.c -> setup_percpu.c
>>> applied to tip/x86/unify-setup - thanks Mike.
>>>
>>> Vegard, can i add your Acked-by too?
>> To be honest, I'd prefer that the function returns a const pointer. 
>> Mike and I have both reviewed all callers independently and concluded 
>> that there is no problem in doing this, and that, in fact, this is the 
>> correct way to deal with it.
>>
>> So if Mike submits a V4 with this const return type, or another patch 
>> on top of this one, I'll ack it :-)
> 
> ok, i'll wait for v4 :)
> 
> (v3 is applied already so Mike please send a delta to v3.)
> 
> 	Ingo

Subject: [PATCH 1/1] x86: Change _node_to_cpumask_ptr to return const ptr

  * Strengthen the return type for the _node_to_cpumask_ptr to be
    a const pointer.  This adds compiler checking to insure that
    node_to_cpumask_map[] is not changed inadvertently.

Applies to tip/master with the following patch applied:

	"[PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3"

Signed-off-by: Mike Travis <travis@sgi.com>
---
Note: I did not change node_to_cpumask_ptr() in include/asm-generic/topology.h
      as node_to_cpumask_ptr_next() does change the cpumask value.
---
 arch/x86/kernel/setup_percpu.c |    8 ++++----
 include/asm-x86/topology.h     |   10 +++++-----
 2 files changed, 9 insertions(+), 9 deletions(-)

--- linux-2.6.tip.orig/arch/x86/kernel/setup_percpu.c
+++ linux-2.6.tip/arch/x86/kernel/setup_percpu.c
@@ -353,23 +353,23 @@ static const cpumask_t cpu_mask_none;
 /*
  * Returns a pointer to the bitmask of CPUs on Node 'node'.
  */
-cpumask_t *_node_to_cpumask_ptr(int node)
+const cpumask_t *_node_to_cpumask_ptr(int node)
 {
 	if (node_to_cpumask_map == NULL) {
 		printk(KERN_WARNING
 			"_node_to_cpumask_ptr(%d): no node_to_cpumask_map!\n",
 			node);
 		dump_stack();
-		return &cpu_online_map;
+		return (const cpumask_t *)&cpu_online_map;
 	}
 	if (node >= nr_node_ids) {
 		printk(KERN_WARNING
 			"_node_to_cpumask_ptr(%d): node > nr_node_ids(%d)\n",
 			node, nr_node_ids);
 		dump_stack();
-		return (cpumask_t *)&cpu_mask_none;
+		return &cpu_mask_none;
 	}
-	return (cpumask_t *)&node_to_cpumask_map[node];
+	return &node_to_cpumask_map[node];
 }
 EXPORT_SYMBOL(_node_to_cpumask_ptr);
 
--- linux-2.6.tip.orig/include/asm-x86/topology.h
+++ linux-2.6.tip/include/asm-x86/topology.h
@@ -82,7 +82,7 @@ DECLARE_EARLY_PER_CPU(int, x86_cpu_to_no
 #ifdef CONFIG_DEBUG_PER_CPU_MAPS
 extern int cpu_to_node(int cpu);
 extern int early_cpu_to_node(int cpu);
-extern cpumask_t *_node_to_cpumask_ptr(int node);
+extern const cpumask_t *_node_to_cpumask_ptr(int node);
 extern cpumask_t node_to_cpumask(int node);
 
 #else	/* !CONFIG_DEBUG_PER_CPU_MAPS */
@@ -103,7 +103,7 @@ static inline int early_cpu_to_node(int 
 }
 
 /* Returns a pointer to the cpumask of CPUs on Node 'node'. */
-static inline cpumask_t *_node_to_cpumask_ptr(int node)
+static inline const cpumask_t *_node_to_cpumask_ptr(int node)
 {
 	return &node_to_cpumask_map[node];
 }
@@ -118,7 +118,7 @@ static inline cpumask_t node_to_cpumask(
 
 /* Replace default node_to_cpumask_ptr with optimized version */
 #define node_to_cpumask_ptr(v, node)		\
-		cpumask_t *v = _node_to_cpumask_ptr(node)
+		const cpumask_t *v = _node_to_cpumask_ptr(node)
 
 #define node_to_cpumask_ptr_next(v, node)	\
 			   v = _node_to_cpumask_ptr(node)
@@ -186,7 +186,7 @@ extern int __node_distance(int, int);
 #define	cpu_to_node(cpu)	0
 #define	early_cpu_to_node(cpu)	0
 
-static inline cpumask_t *_node_to_cpumask_ptr(int node)
+static inline const cpumask_t *_node_to_cpumask_ptr(int node)
 {
 	return &cpu_online_map;
 }
@@ -201,7 +201,7 @@ static inline int node_to_first_cpu(int 
 
 /* Replace default node_to_cpumask_ptr with optimized version */
 #define node_to_cpumask_ptr(v, node)		\
-		cpumask_t *v = _node_to_cpumask_ptr(node)
+		const cpumask_t *v = _node_to_cpumask_ptr(node)
 
 #define node_to_cpumask_ptr_next(v, node)	\
 			   v = _node_to_cpumask_ptr(node)

Re: [PATCH 1/1] x86: Change _node_to_cpumask_ptr to return const ptr

[Vegard Nossum]

Hi,

On Tue, Jul 8, 2008 at 7:06 PM, Mike Travis <travis@sgi.com> wrote:
>> (v3 is applied already so Mike please send a delta to v3.)
>>
>>       Ingo
>
> Subject: [PATCH 1/1] x86: Change _node_to_cpumask_ptr to return const ptr
>
>  * Strengthen the return type for the _node_to_cpumask_ptr to be
>    a const pointer.  This adds compiler checking to insure that
>    node_to_cpumask_map[] is not changed inadvertently.
>
> Applies to tip/master with the following patch applied:
>
>        "[PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3"
>
> Signed-off-by: Mike Travis <travis@sgi.com>
> ---
> Note: I did not change node_to_cpumask_ptr() in include/asm-generic/topology.h
>      as node_to_cpumask_ptr_next() does change the cpumask value.

Hmmm. Does it really?

#define node_to_cpumask_ptr_next(v, node)                               \
                          _##v = node_to_cpumask(node)

This doesn't seem to modify it?

Also, isn't it unfortunate to have the same function return
const/non-const depending on your arch/config?


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: [PATCH 1/1] x86: Change _node_to_cpumask_ptr to return const ptr

[Mike Travis]

Vegard Nossum wrote:
> Hi,
> 
> On Tue, Jul 8, 2008 at 7:06 PM, Mike Travis <travis@sgi.com> wrote:
>>> (v3 is applied already so Mike please send a delta to v3.)
>>>
>>>       Ingo
>> Subject: [PATCH 1/1] x86: Change _node_to_cpumask_ptr to return const ptr
>>
>>  * Strengthen the return type for the _node_to_cpumask_ptr to be
>>    a const pointer.  This adds compiler checking to insure that
>>    node_to_cpumask_map[] is not changed inadvertently.
>>
>> Applies to tip/master with the following patch applied:
>>
>>        "[PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3"
>>
>> Signed-off-by: Mike Travis <travis@sgi.com>
>> ---
>> Note: I did not change node_to_cpumask_ptr() in include/asm-generic/topology.h
>>      as node_to_cpumask_ptr_next() does change the cpumask value.
> 
> Hmmm. Does it really?
> 
> #define node_to_cpumask_ptr_next(v, node)                               \
>                           _##v = node_to_cpumask(node)
> 
> This doesn't seem to modify it?

Well I thought about it.  The pointer (*v) does not change
but the underlying cpumask variable is updated with the
cpumask for the (supposedly) new node number.  You can see
that in this code snippet from kernel/sched.c:

        for (i = 1; i < SD_NODES_PER_DOMAIN; i++) {
                int next_node = find_next_best_node(node, &used_nodes);

                node_to_cpumask_ptr_next(nodemask, next_node);
                cpus_or(*span, *span, *nodemask);
        }

In the optimized (x86_64) case, the pointer is simply modified
to point to the new node_to_cpumask_map[node] entry.  It remains
a pointer to a const value.

But the non-optimized version replaces the const cpumask value
with the new cpumask value.  Isn't this breaking the const
attribute?

> 
> Also, isn't it unfortunate to have the same function return
> const/non-const depending on your arch/config?

But isn't that exactly what it does?  (And in reality, the real
protection happens when there is a node_to_cpumask_map[] present.)

But whichever seems more correct is fine with me... ;-)

Thanks,
Mike

Re: [PATCH 1/1] x86: Change _node_to_cpumask_ptr to return const ptr

[Vegard Nossum]

On Tue, Jul 8, 2008 at 8:05 PM, Mike Travis <travis@sgi.com> wrote:
>>> Note: I did not change node_to_cpumask_ptr() in include/asm-generic/topology.h
>>>      as node_to_cpumask_ptr_next() does change the cpumask value.
>>
>> Hmmm. Does it really?
>>
>> #define node_to_cpumask_ptr_next(v, node)                               \
>>                           _##v = node_to_cpumask(node)
>>
>> This doesn't seem to modify it?
>
> Well I thought about it.  The pointer (*v) does not change
> but the underlying cpumask variable is updated with the
> cpumask for the (supposedly) new node number.  You can see
> that in this code snippet from kernel/sched.c:
>
>        for (i = 1; i < SD_NODES_PER_DOMAIN; i++) {
>                int next_node = find_next_best_node(node, &used_nodes);
>
>                node_to_cpumask_ptr_next(nodemask, next_node);
>                cpus_or(*span, *span, *nodemask);
>        }
>
> In the optimized (x86_64) case, the pointer is simply modified
> to point to the new node_to_cpumask_map[node] entry.  It remains
> a pointer to a const value.
>
> But the non-optimized version replaces the const cpumask value
> with the new cpumask value.  Isn't this breaking the const
> attribute?

No, I think the pointer really should be const. This doesn't guarantee
that the value doesn't change behind our backs, it only prevents us
from modifying it ourselves.


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: [PATCH 1/1] x86: Change _node_to_cpumask_ptr to return const ptr

[Mike Travis]

Vegard Nossum wrote:
> On Tue, Jul 8, 2008 at 8:05 PM, Mike Travis <travis@sgi.com> wrote:
>>>> Note: I did not change node_to_cpumask_ptr() in include/asm-generic/topology.h
>>>>      as node_to_cpumask_ptr_next() does change the cpumask value.
>>> Hmmm. Does it really?
>>>
>>> #define node_to_cpumask_ptr_next(v, node)                               \
>>>                           _##v = node_to_cpumask(node)
>>>
>>> This doesn't seem to modify it?
>> Well I thought about it.  The pointer (*v) does not change
>> but the underlying cpumask variable is updated with the
>> cpumask for the (supposedly) new node number.  You can see
>> that in this code snippet from kernel/sched.c:
>>
>>        for (i = 1; i < SD_NODES_PER_DOMAIN; i++) {
>>                int next_node = find_next_best_node(node, &used_nodes);
>>
>>                node_to_cpumask_ptr_next(nodemask, next_node);
>>                cpus_or(*span, *span, *nodemask);
>>        }
>>
>> In the optimized (x86_64) case, the pointer is simply modified
>> to point to the new node_to_cpumask_map[node] entry.  It remains
>> a pointer to a const value.
>>
>> But the non-optimized version replaces the const cpumask value
>> with the new cpumask value.  Isn't this breaking the const
>> attribute?
> 
> No, I think the pointer really should be const. This doesn't guarantee
> that the value doesn't change behind our backs, it only prevents us
> from modifying it ourselves.
> 
> 
> Vegard
> 

Is this what you had in mind:

 
--- linux-2.6.tip.orig/include/asm-generic/topology.h
+++ linux-2.6.tip/include/asm-generic/topology.h
@@ -60,7 +60,7 @@
 #ifndef node_to_cpumask_ptr
 
 #define	node_to_cpumask_ptr(v, node) 					\
-		cpumask_t _##v = node_to_cpumask(node), *v = &_##v
+		const cpumask_t _##v = node_to_cpumask(node), *v = &_##v
 
 #define node_to_cpumask_ptr_next(v, node)				\
 			  _##v = node_to_cpumask(node)


(It's taking a while as now I need to do some cross-compile testing.)

Thanks,
Mike

Re: [PATCH 1/1] x86: Change _node_to_cpumask_ptr to return const ptr

[Vegard Nossum]

On Tue, Jul 8, 2008 at 10:51 PM, Mike Travis <travis@sgi.com> wrote:
> Vegard Nossum wrote:
>> On Tue, Jul 8, 2008 at 8:05 PM, Mike Travis <travis@sgi.com> wrote:
>>>>> Note: I did not change node_to_cpumask_ptr() in include/asm-generic/topology.h
>>>>>      as node_to_cpumask_ptr_next() does change the cpumask value.
>>>> Hmmm. Does it really?
>>>>
>>>> #define node_to_cpumask_ptr_next(v, node)                               \
>>>>                           _##v = node_to_cpumask(node)
>>>>
>>>> This doesn't seem to modify it?
>>> Well I thought about it.  The pointer (*v) does not change
>>> but the underlying cpumask variable is updated with the
>>> cpumask for the (supposedly) new node number.  You can see
>>> that in this code snippet from kernel/sched.c:
>>>
>>>        for (i = 1; i < SD_NODES_PER_DOMAIN; i++) {
>>>                int next_node = find_next_best_node(node, &used_nodes);
>>>
>>>                node_to_cpumask_ptr_next(nodemask, next_node);
>>>                cpus_or(*span, *span, *nodemask);
>>>        }
>>>
>>> In the optimized (x86_64) case, the pointer is simply modified
>>> to point to the new node_to_cpumask_map[node] entry.  It remains
>>> a pointer to a const value.
>>>
>>> But the non-optimized version replaces the const cpumask value
>>> with the new cpumask value.  Isn't this breaking the const
>>> attribute?
>>
>> No, I think the pointer really should be const. This doesn't guarantee
>> that the value doesn't change behind our backs, it only prevents us
>> from modifying it ourselves.
>>
>>
>> Vegard
>>
>
> Is this what you had in mind:
>
>
> --- linux-2.6.tip.orig/include/asm-generic/topology.h
> +++ linux-2.6.tip/include/asm-generic/topology.h
> @@ -60,7 +60,7 @@
>  #ifndef node_to_cpumask_ptr
>
>  #define        node_to_cpumask_ptr(v, node)                                    \
> -               cpumask_t _##v = node_to_cpumask(node), *v = &_##v
> +               const cpumask_t _##v = node_to_cpumask(node), *v = &_##v
>
>  #define node_to_cpumask_ptr_next(v, node)                              \
>                          _##v = node_to_cpumask(node)
>
>
> (It's taking a while as now I need to do some cross-compile testing.)

Actually, no.

We don't want the _##v to be const, do we? What do you think about
this? (Watch out for whitespace munges)

diff --git a/include/asm-generic/topology.h b/include/asm-generic/topology.h
index a6aea79..56957f2 100644
--- a/include/asm-generic/topology.h
+++ b/include/asm-generic/topology.h
@@ -60,7 +60,8 @@
 #ifndef node_to_cpumask_ptr

 #define        node_to_cpumask_ptr(v, node)
-               cpumask_t _##v = node_to_cpumask(node), *v = &_##v
+               cpumask_t _##v = node_to_cpumask(node);                 \
+               const cpumask_t *v = &_##v;

 #define node_to_cpumask_ptr_next(v, node)                              \
                          _##v = node_to_cpumask(node)


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: [PATCH 1/1] x86: Change _node_to_cpumask_ptr to return const ptr

[Mike Travis]

Vegard Nossum wrote:
> On Tue, Jul 8, 2008 at 10:51 PM, Mike Travis <travis@sgi.com> wrote:
>> Vegard Nossum wrote:
>>> On Tue, Jul 8, 2008 at 8:05 PM, Mike Travis <travis@sgi.com> wrote:
>>>>>> Note: I did not change node_to_cpumask_ptr() in include/asm-generic/topology.h
>>>>>>      as node_to_cpumask_ptr_next() does change the cpumask value.
>>>>> Hmmm. Does it really?
>>>>>
>>>>> #define node_to_cpumask_ptr_next(v, node)                               \
>>>>>                           _##v = node_to_cpumask(node)
>>>>>
>>>>> This doesn't seem to modify it?
>>>> Well I thought about it.  The pointer (*v) does not change
>>>> but the underlying cpumask variable is updated with the
>>>> cpumask for the (supposedly) new node number.  You can see
>>>> that in this code snippet from kernel/sched.c:
>>>>
>>>>        for (i = 1; i < SD_NODES_PER_DOMAIN; i++) {
>>>>                int next_node = find_next_best_node(node, &used_nodes);
>>>>
>>>>                node_to_cpumask_ptr_next(nodemask, next_node);
>>>>                cpus_or(*span, *span, *nodemask);
>>>>        }
>>>>
>>>> In the optimized (x86_64) case, the pointer is simply modified
>>>> to point to the new node_to_cpumask_map[node] entry.  It remains
>>>> a pointer to a const value.
>>>>
>>>> But the non-optimized version replaces the const cpumask value
>>>> with the new cpumask value.  Isn't this breaking the const
>>>> attribute?
>>> No, I think the pointer really should be const. This doesn't guarantee
>>> that the value doesn't change behind our backs, it only prevents us
>>> from modifying it ourselves.
>>>
>>>
>>> Vegard
>>>
>> Is this what you had in mind:
>>
>>
>> --- linux-2.6.tip.orig/include/asm-generic/topology.h
>> +++ linux-2.6.tip/include/asm-generic/topology.h
>> @@ -60,7 +60,7 @@
>>  #ifndef node_to_cpumask_ptr
>>
>>  #define        node_to_cpumask_ptr(v, node)                                    \
>> -               cpumask_t _##v = node_to_cpumask(node), *v = &_##v
>> +               const cpumask_t _##v = node_to_cpumask(node), *v = &_##v
>>
>>  #define node_to_cpumask_ptr_next(v, node)                              \
>>                          _##v = node_to_cpumask(node)
>>
>>
>> (It's taking a while as now I need to do some cross-compile testing.)
> 
> Actually, no.
> 
> We don't want the _##v to be const, do we? What do you think about
> this? (Watch out for whitespace munges)
> 
> diff --git a/include/asm-generic/topology.h b/include/asm-generic/topology.h
> index a6aea79..56957f2 100644
> --- a/include/asm-generic/topology.h
> +++ b/include/asm-generic/topology.h
> @@ -60,7 +60,8 @@
>  #ifndef node_to_cpumask_ptr
> 
>  #define        node_to_cpumask_ptr(v, node)
> -               cpumask_t _##v = node_to_cpumask(node), *v = &_##v
> +               cpumask_t _##v = node_to_cpumask(node);                 \
> +               const cpumask_t *v = &_##v;
> 
>  #define node_to_cpumask_ptr_next(v, node)                              \
>                           _##v = node_to_cpumask(node)
> 
> 
> Vegard
> 

Thanks.  That was my alternative though I was hoping to confirm that
the compiler detected the overwrite by node_to_cpumask_ptr_next().
Unfortunately every non-x86 cross-compile that I have for a machine
that has NUMA is failing in some other way.

I'll resubmit with that change.

Mike

Re: [PATCH 1/1] x86: Change _node_to_cpumask_ptr to return const ptr

[Mike Travis]

Subject: [PATCH 1/1] x86: Change _node_to_cpumask_ptr to return const ptr

  * Strengthen the return type for the _node_to_cpumask_ptr to be
    a const pointer.  This adds compiler checking to insure that
    node_to_cpumask_map[] is not changed inadvertently.

Applies to tip/master with the following patch applied:

	"[PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3"

Signed-off-by: Mike Travis <travis@sgi.com>
---
 arch/x86/kernel/setup_percpu.c |    8 ++++----
 include/asm-generic/topology.h |    3 ++-
 include/asm-x86/topology.h     |   10 +++++-----
 3 files changed, 11 insertions(+), 10 deletions(-)

--- linux-2.6.tip.orig/arch/x86/kernel/setup_percpu.c
+++ linux-2.6.tip/arch/x86/kernel/setup_percpu.c
@@ -353,23 +353,23 @@ static const cpumask_t cpu_mask_none;
 /*
  * Returns a pointer to the bitmask of CPUs on Node 'node'.
  */
-cpumask_t *_node_to_cpumask_ptr(int node)
+const cpumask_t *_node_to_cpumask_ptr(int node)
 {
 	if (node_to_cpumask_map == NULL) {
 		printk(KERN_WARNING
 			"_node_to_cpumask_ptr(%d): no node_to_cpumask_map!\n",
 			node);
 		dump_stack();
-		return &cpu_online_map;
+		return (const cpumask_t *)&cpu_online_map;
 	}
 	if (node >= nr_node_ids) {
 		printk(KERN_WARNING
 			"_node_to_cpumask_ptr(%d): node > nr_node_ids(%d)\n",
 			node, nr_node_ids);
 		dump_stack();
-		return (cpumask_t *)&cpu_mask_none;
+		return &cpu_mask_none;
 	}
-	return (cpumask_t *)&node_to_cpumask_map[node];
+	return &node_to_cpumask_map[node];
 }
 EXPORT_SYMBOL(_node_to_cpumask_ptr);
 
--- linux-2.6.tip.orig/include/asm-generic/topology.h
+++ linux-2.6.tip/include/asm-generic/topology.h
@@ -60,7 +60,8 @@
 #ifndef node_to_cpumask_ptr
 
 #define	node_to_cpumask_ptr(v, node) 					\
-		cpumask_t _##v = node_to_cpumask(node), *v = &_##v
+		cpumask_t _##v = node_to_cpumask(node);			\
+		const cpumask_t *v = &_##v
 
 #define node_to_cpumask_ptr_next(v, node)				\
 			  _##v = node_to_cpumask(node)
--- linux-2.6.tip.orig/include/asm-x86/topology.h
+++ linux-2.6.tip/include/asm-x86/topology.h
@@ -82,7 +82,7 @@ DECLARE_EARLY_PER_CPU(int, x86_cpu_to_no
 #ifdef CONFIG_DEBUG_PER_CPU_MAPS
 extern int cpu_to_node(int cpu);
 extern int early_cpu_to_node(int cpu);
-extern cpumask_t *_node_to_cpumask_ptr(int node);
+extern const cpumask_t *_node_to_cpumask_ptr(int node);
 extern cpumask_t node_to_cpumask(int node);
 
 #else	/* !CONFIG_DEBUG_PER_CPU_MAPS */
@@ -103,7 +103,7 @@ static inline int early_cpu_to_node(int 
 }
 
 /* Returns a pointer to the cpumask of CPUs on Node 'node'. */
-static inline cpumask_t *_node_to_cpumask_ptr(int node)
+static inline const cpumask_t *_node_to_cpumask_ptr(int node)
 {
 	return &node_to_cpumask_map[node];
 }
@@ -118,7 +118,7 @@ static inline cpumask_t node_to_cpumask(
 
 /* Replace default node_to_cpumask_ptr with optimized version */
 #define node_to_cpumask_ptr(v, node)		\
-		cpumask_t *v = _node_to_cpumask_ptr(node)
+		const cpumask_t *v = _node_to_cpumask_ptr(node)
 
 #define node_to_cpumask_ptr_next(v, node)	\
 			   v = _node_to_cpumask_ptr(node)
@@ -186,7 +186,7 @@ extern int __node_distance(int, int);
 #define	cpu_to_node(cpu)	0
 #define	early_cpu_to_node(cpu)	0
 
-static inline cpumask_t *_node_to_cpumask_ptr(int node)
+static inline const cpumask_t *_node_to_cpumask_ptr(int node)
 {
 	return &cpu_online_map;
 }
@@ -201,7 +201,7 @@ static inline int node_to_first_cpu(int 
 
 /* Replace default node_to_cpumask_ptr with optimized version */
 #define node_to_cpumask_ptr(v, node)		\
-		cpumask_t *v = _node_to_cpumask_ptr(node)
+		const cpumask_t *v = _node_to_cpumask_ptr(node)
 
 #define node_to_cpumask_ptr_next(v, node)	\
 			   v = _node_to_cpumask_ptr(node)

Re: [PATCH 1/1] x86: Change _node_to_cpumask_ptr to return const ptr

[Vegard Nossum]

On Tue, Jul 8, 2008 at 11:35 PM, Mike Travis <travis@sgi.com> wrote:
> Subject: [PATCH 1/1] x86: Change _node_to_cpumask_ptr to return const ptr
>
>  * Strengthen the return type for the _node_to_cpumask_ptr to be
>    a const pointer.  This adds compiler checking to insure that
>    node_to_cpumask_map[] is not changed inadvertently.
>
> Applies to tip/master with the following patch applied:
>
>        "[PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3"
>
> Signed-off-by: Mike Travis <travis@sgi.com>
> ---

I can successfully cross-compile the asm-generic changes with a NUMA=y
config on ia64 without error or warning. So I guess, at long last, you
may put, FWIW:

    Acked-by: Vegard Nossum <vegard.nossum@gmail.com>

Sorry for causing much trouble...


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: [PATCH 1/1] x86: Change _node_to_cpumask_ptr to return const ptr

[Ingo Molnar]

* Vegard Nossum <vegard.nossum@gmail.com> wrote:

> On Tue, Jul 8, 2008 at 11:35 PM, Mike Travis <travis@sgi.com> wrote:
> > Subject: [PATCH 1/1] x86: Change _node_to_cpumask_ptr to return const ptr
> >
> >  * Strengthen the return type for the _node_to_cpumask_ptr to be
> >    a const pointer.  This adds compiler checking to insure that
> >    node_to_cpumask_map[] is not changed inadvertently.
> >
> > Applies to tip/master with the following patch applied:
> >
> >        "[PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3"
> >
> > Signed-off-by: Mike Travis <travis@sgi.com>
> > ---
> 
> I can successfully cross-compile the asm-generic changes with a NUMA=y 
> config on ia64 without error or warning. So I guess, at long last, you 
> may put, FWIW:
> 
>     Acked-by: Vegard Nossum <vegard.nossum@gmail.com>

thanks - applied to tip/x86/core.

	Ingo

Re: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3

[Mike Travis]

Vegard Nossum wrote:
> On Thu, Jul 3, 2008 at 10:44 AM, Ingo Molnar <mingo@elte.hu> wrote:
>> * Mike Travis <travis@sgi.com> wrote:
>>
>>> Subject: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3
>>>
>>>   * When CONFIG_DEBUG_PER_CPU_MAPS is set, the node passed to
>>>     node_to_cpumask and node_to_cpumask_ptr should be validated.
>>>     If invalid, then a dump_stack is performed and a zero cpumask
>>>     is returned.
>>>
>>> Based on "Fri Jun 27 10:06:06 PDT 2008" tip/master...  ;-)
>>>
>>> Signed-off-by: Mike Travis <travis@sgi.com>
>>> ---
>>> V2: Slightly different version to remove a compiler warning.
>>> V3: Redone to reflect moving setup.c -> setup_percpu.c
>> applied to tip/x86/unify-setup - thanks Mike.
>>
>> Vegard, can i add your Acked-by too?
> 
> To be honest, I'd prefer that the function returns a const pointer.
> Mike and I have both reviewed all callers independently and concluded
> that there is no problem in doing this, and that, in fact, this is the
> correct way to deal with it.
> 
> So if Mike submits a V4 with this const return type, or another patch
> on top of this one, I'll ack it :-)
> 
> 
> Vegard
> 

Sure, I can do this...

Thanks,
Mike