+ fix-x86_64-splat.patch added to -mm tree

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

---
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(-)

arch/x86/kernel/setup_percpu.c +4 -0

@@ -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();

Quoted 33 lines · 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 <[email protected]>

---
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(-)

arch/x86/kernel/setup_percpu.c +4 -0

@@ -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();

arch/x86/kernel/setup_percpu.c cpumask_t *_node_to_cpumask_ptr(int node · line 370

Quoted 10 lines

arch/x86/kernel/setup_percpu.c +6 -2

               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

Quoted 6 lines

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.

Quoted 6 lines · Mike Travis

Quoted 6 lines

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.

...

Quoted 5 lines

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

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

---
V2: Slightly different version to remove a compiler warning.
V3: Redone to reflect moving setup.c -> setup_percpu.c

Quoted 9 lines · 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... ;-)

Quoted 4 lines

Signed-off-by Mike Travis <[email protected]>

---
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?

Quoted 12 lines · Ingo Molnar

Quoted 9 lines · 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... ;-)

Quoted 8 lines

Quoted 4 lines

Signed-off-by Mike Travis <[email protected]>

---
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 :-)

Quoted 24 lines · Vegard Nossum

Quoted 11 lines · Ingo Molnar

Quoted 9 lines · 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... ;-)

Quoted 7 lines

Quoted 4 lines

Signed-off-by Mike Travis <[email protected]>

---
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.

Quoted 2 lines

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

Quoted 3 lines

(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 <[email protected]>

---
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.

Hi,

Quoted 9 lines · Mike Travis

Quoted 3 lines

(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.

Quoted 8 lines

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

---
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?

Quoted 9 lines

Quoted 2 lines

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?

Quoted 20 lines · Mike Travis

Quoted 8 lines

Quoted 2 lines

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);
       }

Quoted 7 lines

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

Quoted 29 lines · Vegard Nossum

Quoted 20 lines · Mike Travis

Quoted 8 lines

Quoted 2 lines

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);
       }

Quoted 7 lines

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?

Quoted 7 lines

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:

include/asm-generic/topology.h +1 -1

@@ -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.)

Quoted 52 lines · Mike Travis

Quoted 28 lines · Vegard Nossum

Quoted 20 lines · Mike Travis

Quoted 8 lines

Quoted 2 lines

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);
       }

Quoted 7 lines

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?

Quoted 7 lines

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:

include/asm-generic/topology.h +1 -1

@@ -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)

Quoted 1 line

(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)

include/asm-generic/topology.h +2 -1

index a6aea79..56957f2 100644
@@ -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

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

---

Quoted 10 lines · 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"

Quoted 2 lines

Signed-off-by Mike Travis <[email protected]>

---

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

Quoted 11 lines · Ingo Molnar

Quoted 9 lines · 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... ;-)

Quoted 7 lines

Quoted 4 lines

Signed-off-by Mike Travis <[email protected]>

---
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