* [TRIVIAL PATCH] Use valid node number when unmapping CPUs
@ 2003-12-22 19:37 Matthew Dobson
2003-12-23 0:41 ` Nick Piggin
0 siblings, 1 reply; 6+ messages in thread
From: Matthew Dobson @ 2003-12-22 19:37 UTC (permalink / raw)
To: linux-kernel; +Cc: mbligh, Andrew Morton, Trivial Patch Monkey
[-- Attachment #1: Type: text/plain, Size: 487 bytes --]
The cpu_2_node array for i386 is initialized to 0 for each CPU,
effectively mapping all CPUs to node 0 unless changed. When we unmap
CPUs, however, we stick a -1 in the array, mapping the CPU to an invalid
node. This really isn't helpful. We should map the CPU to node 0, to
make sure that callers of cpu_to_node() and friends aren't returned a
bogus node number. This trivial patch changes the unmapping code to
place a 0 in the node mapping for removed CPUs.
Cheers!
-Matt
[-- Attachment #2: use_node0_on_unmap.patch --]
[-- Type: text/plain, Size: 572 bytes --]
diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.0-vanilla/arch/i386/kernel/smpboot.c linux-2.6.0-patched/arch/i386/kernel/smpboot.c
--- linux-2.6.0-vanilla/arch/i386/kernel/smpboot.c Wed Dec 17 18:58:49 2003
+++ linux-2.6.0-patched/arch/i386/kernel/smpboot.c Thu Dec 18 14:36:06 2003
@@ -520,7 +520,7 @@ static inline void unmap_cpu_to_node(int
printk("Unmapping cpu %d from all nodes\n", cpu);
for (node = 0; node < MAX_NUMNODES; node ++)
cpu_clear(cpu, node_2_cpu_mask[node]);
- cpu_2_node[cpu] = -1;
+ cpu_2_node[cpu] = 0;
}
#else /* !CONFIG_NUMA */
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [TRIVIAL PATCH] Use valid node number when unmapping CPUs
2003-12-22 19:37 [TRIVIAL PATCH] Use valid node number when unmapping CPUs Matthew Dobson
@ 2003-12-23 0:41 ` Nick Piggin
2004-01-05 22:52 ` Matthew Dobson
2004-03-29 15:45 ` Matthew Dobson
0 siblings, 2 replies; 6+ messages in thread
From: Nick Piggin @ 2003-12-23 0:41 UTC (permalink / raw)
To: colpatch; +Cc: linux-kernel, mbligh, Andrew Morton, Trivial Patch Monkey
Matthew Dobson wrote:
> The cpu_2_node array for i386 is initialized to 0 for each CPU,
> effectively mapping all CPUs to node 0 unless changed. When we unmap
> CPUs, however, we stick a -1 in the array, mapping the CPU to an
> invalid node. This really isn't helpful. We should map the CPU to
> node 0, to make sure that callers of cpu_to_node() and friends aren't
> returned a bogus node number. This trivial patch changes the
> unmapping code to place a 0 in the node mapping for removed CPUs.
>
> Cheers!
I'd prefer it got initialised to -1 for each cpu, and either set to -1
or not touched during unmap.
0 is more bogus than the alternatives, isn't it? At least for the subset
of CPUs not on node 0. Callers should be fixed.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [TRIVIAL PATCH] Use valid node number when unmapping CPUs
2003-12-23 0:41 ` Nick Piggin
@ 2004-01-05 22:52 ` Matthew Dobson
2004-01-06 0:39 ` Nick Piggin
2004-03-29 15:46 ` Nick Piggin
2004-03-29 15:45 ` Matthew Dobson
1 sibling, 2 replies; 6+ messages in thread
From: Matthew Dobson @ 2004-01-05 22:52 UTC (permalink / raw)
To: Nick Piggin; +Cc: linux-kernel, mbligh, Andrew Morton, Trivial Patch Monkey
Nick Piggin wrote:
>
>
> Matthew Dobson wrote:
>
>> The cpu_2_node array for i386 is initialized to 0 for each CPU,
>> effectively mapping all CPUs to node 0 unless changed. When we unmap
>> CPUs, however, we stick a -1 in the array, mapping the CPU to an
>> invalid node. This really isn't helpful. We should map the CPU to
>> node 0, to make sure that callers of cpu_to_node() and friends aren't
>> returned a bogus node number. This trivial patch changes the
>> unmapping code to place a 0 in the node mapping for removed CPUs.
>>
>> Cheers!
>
>
>
> I'd prefer it got initialised to -1 for each cpu, and either set to -1
> or not touched during unmap.
>
>
> 0 is more bogus than the alternatives, isn't it? At least for the subset
> of CPUs not on node 0. Callers should be fixed.
Not really... These macros are usually used for things like scheduling,
memory placement and other decisions. Right now the value doesn't have
to be error-checked, because it is assumed to always return a valid
node. For these types of uses, it's far better to schedule/allocate
something on the wrong node (ie: node 0) than on an invalid node (ie:
node -1). Having a possible negative value for this will break things
when used as an array index (as it often is), and will force us to put
tests to ensure it is a valid value before using it, and introduce
possible races in the future (ie: imagine testing if CPU 17's node
mapping is non-negative, simultaneously unmapping the CPU, then using
the macro again to make a node decision. You may get a negative value
back, thus causing you to index way off the end of your array... BOOM).
If we stick with the convention that we always have a valid (even if
not *correct*) value in these arrays, the worst we should get is poor
performance, not breakage.
Cheers!
-Matt
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [TRIVIAL PATCH] Use valid node number when unmapping CPUs
2004-01-05 22:52 ` Matthew Dobson
@ 2004-01-06 0:39 ` Nick Piggin
2004-03-29 15:46 ` Nick Piggin
1 sibling, 0 replies; 6+ messages in thread
From: Nick Piggin @ 2004-01-06 0:39 UTC (permalink / raw)
To: colpatch; +Cc: linux-kernel, mbligh, Andrew Morton, Trivial Patch Monkey
Matthew Dobson wrote:
> Nick Piggin wrote:
>
>>
>>
>> Matthew Dobson wrote:
>>
>>> The cpu_2_node array for i386 is initialized to 0 for each CPU,
>>> effectively mapping all CPUs to node 0 unless changed. When we
>>> unmap CPUs, however, we stick a -1 in the array, mapping the CPU to
>>> an invalid node. This really isn't helpful. We should map the CPU
>>> to node 0, to make sure that callers of cpu_to_node() and friends
>>> aren't returned a bogus node number. This trivial patch changes the
>>> unmapping code to place a 0 in the node mapping for removed CPUs.
>>>
>>> Cheers!
>>
>>
>>
>>
>> I'd prefer it got initialised to -1 for each cpu, and either set to -1
>> or not touched during unmap.
>
> >
>
>>
>> 0 is more bogus than the alternatives, isn't it? At least for the subset
>> of CPUs not on node 0. Callers should be fixed.
>
>
> Not really... These macros are usually used for things like
> scheduling, memory placement and other decisions. Right now the value
> doesn't have to be error-checked, because it is assumed to always
> return a valid node. For these types of uses, it's far better to
> schedule/allocate something on the wrong node (ie: node 0) than on an
> invalid node (ie: node -1). Having a possible negative value for this
> will break things when used as an array index (as it often is), and
> will force us to put tests to ensure it is a valid value before using
> it, and introduce possible races in the future (ie: imagine testing if
> CPU 17's node mapping is non-negative, simultaneously unmapping the
> CPU, then using the macro again to make a node decision. You may get
> a negative value back, thus causing you to index way off the end of
> your array... BOOM). If we stick with the convention that we always
> have a valid (even if not *correct*) value in these arrays, the worst
> we should get is poor performance, not breakage.
OK, then keep the correct node number. No need to change it to node 0.
Having the value not change at all (1) gives you the correct information
at all times, and (2) eliminates the remaining race possibilities.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [TRIVIAL PATCH] Use valid node number when unmapping CPUs
2004-01-05 22:52 ` Matthew Dobson
2004-01-06 0:39 ` Nick Piggin
@ 2004-03-29 15:46 ` Nick Piggin
1 sibling, 0 replies; 6+ messages in thread
From: Nick Piggin @ 2004-03-29 15:46 UTC (permalink / raw)
To: Administrator; +Cc: linux-kernel, mbligh, Andrew Morton, Trivial Patch Monkey
Matthew Dobson wrote:
> Nick Piggin wrote:
>
>>
>>
>> Matthew Dobson wrote:
>>
>>> The cpu_2_node array for i386 is initialized to 0 for each CPU,
>>> effectively mapping all CPUs to node 0 unless changed. When we
>>> unmap CPUs, however, we stick a -1 in the array, mapping the CPU to
>>> an invalid node. This really isn't helpful. We should map the CPU
>>> to node 0, to make sure that callers of cpu_to_node() and friends
>>> aren't returned a bogus node number. This trivial patch changes the
>>> unmapping code to place a 0 in the node mapping for removed CPUs.
>>>
>>> Cheers!
>>
>>
>>
>>
>> I'd prefer it got initialised to -1 for each cpu, and either set to -1
>> or not touched during unmap.
>
> >
>
>>
>> 0 is more bogus than the alternatives, isn't it? At least for the subset
>> of CPUs not on node 0. Callers should be fixed.
>
>
> Not really... These macros are usually used for things like
> scheduling, memory placement and other decisions. Right now the value
> doesn't have to be error-checked, because it is assumed to always
> return a valid node. For these types of uses, it's far better to
> schedule/allocate something on the wrong node (ie: node 0) than on an
> invalid node (ie: node -1). Having a possible negative value for this
> will break things when used as an array index (as it often is), and
> will force us to put tests to ensure it is a valid value before using
> it, and introduce possible races in the future (ie: imagine testing if
> CPU 17's node mapping is non-negative, simultaneously unmapping the
> CPU, then using the macro again to make a node decision. You may get
> a negative value back, thus causing you to index way off the end of
> your array... BOOM). If we stick with the convention that we always
> have a valid (even if not *correct*) value in these arrays, the worst
> we should get is poor performance, not breakage.
OK, then keep the correct node number. No need to change it to node 0.
Having the value not change at all (1) gives you the correct information
at all times, and (2) eliminates the remaining race possibilities.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [TRIVIAL PATCH] Use valid node number when unmapping CPUs
2003-12-23 0:41 ` Nick Piggin
2004-01-05 22:52 ` Matthew Dobson
@ 2004-03-29 15:45 ` Matthew Dobson
1 sibling, 0 replies; 6+ messages in thread
From: Matthew Dobson @ 2004-03-29 15:45 UTC (permalink / raw)
To: Administrator; +Cc: linux-kernel, mbligh, Andrew Morton, Trivial Patch Monkey
Nick Piggin wrote:
>
>
> Matthew Dobson wrote:
>
>> The cpu_2_node array for i386 is initialized to 0 for each CPU,
>> effectively mapping all CPUs to node 0 unless changed. When we unmap
>> CPUs, however, we stick a -1 in the array, mapping the CPU to an
>> invalid node. This really isn't helpful. We should map the CPU to
>> node 0, to make sure that callers of cpu_to_node() and friends aren't
>> returned a bogus node number. This trivial patch changes the
>> unmapping code to place a 0 in the node mapping for removed CPUs.
>>
>> Cheers!
>
>
>
> I'd prefer it got initialised to -1 for each cpu, and either set to -1
> or not touched during unmap.
>
>
> 0 is more bogus than the alternatives, isn't it? At least for the subset
> of CPUs not on node 0. Callers should be fixed.
Not really... These macros are usually used for things like scheduling,
memory placement and other decisions. Right now the value doesn't have
to be error-checked, because it is assumed to always return a valid
node. For these types of uses, it's far better to schedule/allocate
something on the wrong node (ie: node 0) than on an invalid node (ie:
node -1). Having a possible negative value for this will break things
when used as an array index (as it often is), and will force us to put
tests to ensure it is a valid value before using it, and introduce
possible races in the future (ie: imagine testing if CPU 17's node
mapping is non-negative, simultaneously unmapping the CPU, then using
the macro again to make a node decision. You may get a negative value
back, thus causing you to index way off the end of your array... BOOM).
If we stick with the convention that we always have a valid (even if
not *correct*) value in these arrays, the worst we should get is poor
performance, not breakage.
Cheers!
-Matt
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2004-03-29 15:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2003-12-22 19:37 [TRIVIAL PATCH] Use valid node number when unmapping CPUs Matthew Dobson
2003-12-23 0:41 ` Nick Piggin
2004-01-05 22:52 ` Matthew Dobson
2004-01-06 0:39 ` Nick Piggin
2004-03-29 15:46 ` Nick Piggin
2004-03-29 15:45 ` Matthew Dobson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox