[PATCH] x86: Wrong /proc/cpuinfo core id on AMD fam_f model_9

[PATCH] x86: Wrong /proc/cpuinfo core id on AMD fam_f model_9

[Venkatesh Pallipadi]

Commit 4a376ec3a2599c02207cd4cbd5dbf73783548463 changes cpuinfo cpu_core_id
from an unique id in a physical package to core id within a node. And it
does not change phys_proc_id or booted_cores to reflect this new topology.

This breaks the user view of topology in /proc/cpuinfo.

With commit 4a376ec /proc/cpuinfo output (for one package) looks something like
processor: 0..11
physical id: 0..0
core id: 0..5 0..5
siblings: 12
cpu cores: 12

That is, there are processors with same "physical id" and same "core id" (which
are not SMT siblings). As I understand, if /proc/cpuinfo says there are 12
"cpu cores" per package, there should be 12 unique core ids in that package.
And same "physical id" and "core id" is supposed to indicate SMT siblings.

The change below reverts the cpu_core_id part of that commit and
/proc/cpuinfo has
processor: 0..11
physical id: 0..0
core id: 0..11
siblings: 12
cpu cores: 12


Also, if the intention of the original change was to export two node info,
then changing just the core id is not enough. User has no way to determine
which of these 6 cores out of 12 belong to same node by looking at
/proc/cpuinfo output. Both "physical id" and "cpu cores" has to change
to reflect that or one more node id needs to be added (I didn't say that :))

Signed-off-by: Venkatesh Pallipadi <venki@google.com>
---
 arch/x86/kernel/cpu/amd.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index e485825..80d3d80 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -281,9 +281,6 @@ static void __cpuinit amd_fixup_dcm(struct cpuinfo_x86 *c)
 
 	/* store NodeID, use llc_shared_map to store sibling info */
 	per_cpu(cpu_llc_id, cpu) = value & 7;
-
-	/* fixup core id to be in range from 0 to (cores_per_node - 1) */
-	c->cpu_core_id = c->cpu_core_id % cores_per_node;
 }
 #endif
 
-- 
1.7.1

Re: [PATCH] x86: Wrong /proc/cpuinfo core id on AMD fam_f model_9

[Andreas Herrmann]

On Thu, Aug 12, 2010 at 05:28:49PM -0400, Venkatesh Pallipadi wrote:
> Commit 4a376ec3a2599c02207cd4cbd5dbf73783548463 changes cpuinfo cpu_core_id
> from an unique id in a physical package to core id within a node. And it
> does not change phys_proc_id or booted_cores to reflect this new topology.

It shouldn't change phys_proc_id because that is supposed to be the
socket information. (both for AMD and Intel)

> This breaks the user view of topology in /proc/cpuinfo.
> 
> With commit 4a376ec /proc/cpuinfo output (for one package) looks something like
> processor: 0..11
> physical id: 0..0
> core id: 0..5 0..5
> siblings: 12
> cpu cores: 12
> 
> That is, there are processors with same "physical id" and same "core id" (which
> are not SMT siblings). As I understand, if /proc/cpuinfo says there are 12
> "cpu cores" per package, there should be 12 unique core ids in that package.
> And same "physical id" and "core id" is supposed to indicate SMT siblings.

I don't agree. That's rather an Intel centric view. You should use
thread_sibling information to identify this.  See the topology
sub-directory for each CPU.

> The change below reverts the cpu_core_id part of that commit and

Please don't do that. Potentially you are breaking user space. You
rather need to know the core id (0..5) within a node instead of the
CoreId (0..11) within the entire package. See AMD's BKDG for family
0x10 CPUs. As a rule of thumb you require the ID of a core within one
Node -- not within a package.

> /proc/cpuinfo has
> processor: 0..11
> physical id: 0..0
> core id: 0..11
> siblings: 12
> cpu cores: 12

> Also, if the intention of the original change was to export two node info,
> then changing just the core id is not enough. User has no way to determine
> which of these 6 cores out of 12 belong to same node by looking at
> /proc/cpuinfo output.

Yes, you can't get this info from /proc/cpuinfo output. But
/proc/cpuinfo is completely useless to gather entire toplogy
information anyway.  You should extract most stuff from the topology
subdirectory for CPUs.

> Both "physical id" and "cpu cores" has to change
> to reflect that or one more node id needs to be added (I didn't say that :))

No, physical id has _not_ to change.

The other option -- exporting NodeId -- I tried last year but this
was rejected. (see http://marc.info/?l=linux-kernel&m=124964980507887)
Did not have time to work on implementing other proposed solutions though ... Sigh

Thus, so far, the node sibling information is only reflected in the L3
cache sibling mask for Magny-Cours-CPUs. Ugly, but working.

> Signed-off-by: Venkatesh Pallipadi <venki@google.com>

NOT-Acked-by: Andreas Herrmann <andreas.herrmann3@amd.com>


Andreas

-- 
Operating | Advanced Micro Devices GmbH
  System  | Einsteinring 24, 85609 Dornach b. München, Germany
 Research | Geschäftsführer: Alberto Bozzo, Andrew Bowd
  Center  | Sitz: Dornach, Gemeinde Aschheim, Landkreis München
  (OSRC)  | Registergericht München, HRB Nr. 43632

Re: [PATCH] x86: Wrong /proc/cpuinfo core id on AMD fam_f model_9

[Venkatesh Pallipadi]

On Fri, Aug 13, 2010 at 12:04 AM, Andreas Herrmann
<andreas.herrmann3@amd.com> wrote:
> On Thu, Aug 12, 2010 at 05:28:49PM -0400, Venkatesh Pallipadi wrote:
>> Commit 4a376ec3a2599c02207cd4cbd5dbf73783548463 changes cpuinfo cpu_core_id
>> from an unique id in a physical package to core id within a node. And it
>> does not change phys_proc_id or booted_cores to reflect this new topology.
>
> It shouldn't change phys_proc_id because that is supposed to be the
> socket information. (both for AMD and Intel)
>
>> This breaks the user view of topology in /proc/cpuinfo.
>>
>> With commit 4a376ec /proc/cpuinfo output (for one package) looks something like
>> processor: 0..11
>> physical id: 0..0
>> core id: 0..5 0..5
>> siblings: 12
>> cpu cores: 12
>>
>> That is, there are processors with same "physical id" and same "core id" (which
>> are not SMT siblings). As I understand, if /proc/cpuinfo says there are 12
>> "cpu cores" per package, there should be 12 unique core ids in that package.
>> And same "physical id" and "core id" is supposed to indicate SMT siblings.
>
> I don't agree. That's rather an Intel centric view. You should use
> thread_sibling information to identify this.  See the topology
> sub-directory for each CPU.

The reason for this patch was a breakage in the app that tries to
build CPU maps for packages and cores (Nothing to do with Intel or
AMD). The app is using info under /sys/devices/system//node/ and
/sys/device/system/cpu/cpu0/cache to get the NUMA node and cache
sharing info etc. And looking at cpuinfo for core package info. And is
getting confused with NUMA overloaded package and core info in
/proc/cpunfo. I can of course change this specific app to workaround
this. But, I think changing core id to reflect numa-ness without a
numa id to go with it is not right.

Basically, with this NUMA overloading of core id, we are loosing the
unique id of cores that was in "core id" before. Without this
/proc/cpuinfo output makes sense with "cpu cores" being 12 and 12
unique ids for cores under one physical id. With this change, "cpu
cores" under a package is still 12. But, trying to id those 12 cores,
one ends up with 0..5 0..5. We are loosing the MSB on these IDs only
to give some implicit and incomplete information ( Number of nodes =
"cpu cores" /  number of unique core ids, but we wont tell you here
how to identify those nodes. Look at /sys..node for that).

Isn't it better to leave /proc/cpuinfo out of this so that older apps
continue to work and newer apps can use all the /sys fs topology
information to get the complete info?

Taking this a bit further, this different cores sharing same core id
within a package is going to be messy in general. Consider totally
hypothetical CPU package with 2 nodes and 2 SMT siblings in each node.
If we follow this current scheme of things of - stuffing node info
without node id - in proc/cpuinfo, it will look something like
processor: 0
physical id: 0
siblings: 4
core id: 0
cpu cores: 4

processor: 1
physical id: 0
siblings: 4
core id: 0
cpu cores: 4

processor: 2
physical id: 0
siblings: 4
core id: 0
cpu cores: 4

processor: 3
physical id: 0
siblings: 4
core id: 0
cpu cores: 4

Which would be very much useless. We should either have "node id" info
or not change the "core id" meaning to include node info.

>> The change below reverts the cpu_core_id part of that commit and
>
> Please don't do that. Potentially you are breaking user space. You
> rather need to know the core id (0..5) within a node instead of the
> CoreId (0..11) within the entire package. See AMD's BKDG for family
> 0x10 CPUs. As a rule of thumb you require the ID of a core within one
> Node -- not within a package.

The problem is the core_id change is breaking the userspace decoding
of package/core/thread info from /proc/cpuinfo. Yes. I may need to
know core id's in a node. But, /proc/cpuinfo is not giving be that
info of "ID of core withing one Node". There is no Node info there.
Its still giving me Core ID withing a physical package with no way of
knowing which cores are within a node.

>
>> /proc/cpuinfo has
>> processor: 0..11
>> physical id: 0..0
>> core id: 0..11
>> siblings: 12
>> cpu cores: 12
>
>> Also, if the intention of the original change was to export two node info,
>> then changing just the core id is not enough. User has no way to determine
>> which of these 6 cores out of 12 belong to same node by looking at
>> /proc/cpuinfo output.
>
> Yes, you can't get this info from /proc/cpuinfo output. But
> /proc/cpuinfo is completely useless to gather entire toplogy
> information anyway.  You should extract most stuff from the topology
> subdirectory for CPUs.

So, if you think cpuinfo is useless for NUMA topology, then why do you
think core id should be change to reflect NUMA info?

>
>> Both "physical id" and "cpu cores" has to change
>> to reflect that or one more node id needs to be added (I didn't say that :))
>
> No, physical id has _not_ to change.

I agree with physical id should not change, so that user will get the
package info correctly. I mentioned that as one option to make
/proc/cpuinfo self-contained with this NUMA overloading.

> The other option -- exporting NodeId -- I tried last year but this
> was rejected. (see http://marc.info/?l=linux-kernel&m=124964980507887)
> Did not have time to work on implementing other proposed solutions though ... Sigh
>
> Thus, so far, the node sibling information is only reflected in the L3
> cache sibling mask for Magny-Cours-CPUs. Ugly, but working.

Just to summarize, I am of the opinion that if we can't describe
something in /proc/cpuinfo fully, we should not stuff partial info and
break existing assumptions and add new assumptions. We have new APIS
is /sys anyway to describe full topology.

Thanks,
Venki

Re: [PATCH] x86: Wrong /proc/cpuinfo core id on AMD fam_f model_9

[Venkatesh Pallipadi]

On Fri, Aug 13, 2010 at 10:52 AM, Venkatesh Pallipadi <venki@google.com> wrote:
> On Fri, Aug 13, 2010 at 12:04 AM, Andreas Herrmann
> <andreas.herrmann3@amd.com> wrote:
>> On Thu, Aug 12, 2010 at 05:28:49PM -0400, Venkatesh Pallipadi wrote:
>>> Commit 4a376ec3a2599c02207cd4cbd5dbf73783548463 changes cpuinfo cpu_core_id
>>> from an unique id in a physical package to core id within a node. And it
>>> does not change phys_proc_id or booted_cores to reflect this new topology.
>>
>> It shouldn't change phys_proc_id because that is supposed to be the
>> socket information. (both for AMD and Intel)
>>
>>> This breaks the user view of topology in /proc/cpuinfo.
>>>
>>> With commit 4a376ec /proc/cpuinfo output (for one package) looks something like
>>> processor: 0..11
>>> physical id: 0..0
>>> core id: 0..5 0..5
>>> siblings: 12
>>> cpu cores: 12
>>>
>>> That is, there are processors with same "physical id" and same "core id" (which
>>> are not SMT siblings). As I understand, if /proc/cpuinfo says there are 12
>>> "cpu cores" per package, there should be 12 unique core ids in that package.
>>> And same "physical id" and "core id" is supposed to indicate SMT siblings.
>>
>> I don't agree. That's rather an Intel centric view. You should use
>> thread_sibling information to identify this.  See the topology
>> sub-directory for each CPU.
>
> The reason for this patch was a breakage in the app that tries to
> build CPU maps for packages and cores (Nothing to do with Intel or
> AMD). The app is using info under /sys/devices/system//node/ and
> /sys/device/system/cpu/cpu0/cache to get the NUMA node and cache
> sharing info etc. And looking at cpuinfo for core package info. And is
> getting confused with NUMA overloaded package and core info in
> /proc/cpunfo. I can of course change this specific app to workaround
> this. But, I think changing core id to reflect numa-ness without a
> numa id to go with it is not right.
>
> Basically, with this NUMA overloading of core id, we are loosing the
> unique id of cores that was in "core id" before. Without this
> /proc/cpuinfo output makes sense with "cpu cores" being 12 and 12
> unique ids for cores under one physical id. With this change, "cpu
> cores" under a package is still 12. But, trying to id those 12 cores,
> one ends up with 0..5 0..5. We are loosing the MSB on these IDs only
> to give some implicit and incomplete information ( Number of nodes =
> "cpu cores" /  number of unique core ids, but we wont tell you here
> how to identify those nodes. Look at /sys..node for that).
>
> Isn't it better to leave /proc/cpuinfo out of this so that older apps
> continue to work and newer apps can use all the /sys fs topology
> information to get the complete info?
>
> Taking this a bit further, this different cores sharing same core id
> within a package is going to be messy in general. Consider totally
> hypothetical CPU package with 2 nodes and 2 SMT siblings in each node.
> If we follow this current scheme of things of - stuffing node info
> without node id - in proc/cpuinfo, it will look something like

Correction. s/cpu cores: 4/cpu cores: 2/ for this case. But, the
problem with core id will still be there.

> processor: 0
> physical id: 0
> siblings: 4
> core id: 0
> cpu cores: 4
>
> processor: 1
> physical id: 0
> siblings: 4
> core id: 0
> cpu cores: 4
>
> processor: 2
> physical id: 0
> siblings: 4
> core id: 0
> cpu cores: 4
>
> processor: 3
> physical id: 0
> siblings: 4
> core id: 0
> cpu cores: 4
>
> Which would be very much useless. We should either have "node id" info
> or not change the "core id" meaning to include node info.
>
>>> The change below reverts the cpu_core_id part of that commit and
>>
>> Please don't do that. Potentially you are breaking user space. You
>> rather need to know the core id (0..5) within a node instead of the
>> CoreId (0..11) within the entire package. See AMD's BKDG for family
>> 0x10 CPUs. As a rule of thumb you require the ID of a core within one
>> Node -- not within a package.
>
> The problem is the core_id change is breaking the userspace decoding
> of package/core/thread info from /proc/cpuinfo. Yes. I may need to
> know core id's in a node. But, /proc/cpuinfo is not giving be that
> info of "ID of core withing one Node". There is no Node info there.
> Its still giving me Core ID withing a physical package with no way of
> knowing which cores are within a node.
>
>>
>>> /proc/cpuinfo has
>>> processor: 0..11
>>> physical id: 0..0
>>> core id: 0..11
>>> siblings: 12
>>> cpu cores: 12
>>
>>> Also, if the intention of the original change was to export two node info,
>>> then changing just the core id is not enough. User has no way to determine
>>> which of these 6 cores out of 12 belong to same node by looking at
>>> /proc/cpuinfo output.
>>
>> Yes, you can't get this info from /proc/cpuinfo output. But
>> /proc/cpuinfo is completely useless to gather entire toplogy
>> information anyway.  You should extract most stuff from the topology
>> subdirectory for CPUs.
>
> So, if you think cpuinfo is useless for NUMA topology, then why do you
> think core id should be change to reflect NUMA info?
>
>>
>>> Both "physical id" and "cpu cores" has to change
>>> to reflect that or one more node id needs to be added (I didn't say that :))
>>
>> No, physical id has _not_ to change.
>
> I agree with physical id should not change, so that user will get the
> package info correctly. I mentioned that as one option to make
> /proc/cpuinfo self-contained with this NUMA overloading.
>
>> The other option -- exporting NodeId -- I tried last year but this
>> was rejected. (see http://marc.info/?l=linux-kernel&m=124964980507887)
>> Did not have time to work on implementing other proposed solutions though ... Sigh
>>
>> Thus, so far, the node sibling information is only reflected in the L3
>> cache sibling mask for Magny-Cours-CPUs. Ugly, but working.
>
> Just to summarize, I am of the opinion that if we can't describe
> something in /proc/cpuinfo fully, we should not stuff partial info and
> break existing assumptions and add new assumptions. We have new APIS
> is /sys anyway to describe full topology.
>
> Thanks,
> Venki
>

Re: [PATCH] x86: Wrong /proc/cpuinfo core id on AMD fam_f model_9

[Andreas Herrmann]

On Fri, Aug 13, 2010 at 01:52:37PM -0400, Venkatesh Pallipadi wrote:
> On Fri, Aug 13, 2010 at 12:04 AM, Andreas Herrmann
> <andreas.herrmann3@amd.com> wrote:

  [...]

> The reason for this patch was a breakage in the app that tries to
> build CPU maps for packages and cores (Nothing to do with Intel or
> AMD). The app is using info under /sys/devices/system//node/ and
> /sys/device/system/cpu/cpu0/cache to get the NUMA node and cache
> sharing info etc. And looking at cpuinfo for core package info.

Hmm, and why not using .../cpu/cpuX/topology/core_siblings for the
package info? It just sounds strange to me to use /proc/cpuinfo for
CPU topology information.

> And is getting confused with NUMA overloaded package and core info
> in /proc/cpunfo. I can of course change this specific app to
> workaround this. But, I think changing core id to reflect numa-ness
> without a numa id to go with it is not right.

Using Node ID from SRAT table would not be right.
You could have just one NUMA node 0 (spanning all packages) but the
CPUs are still associated to a node within the physical package.
So, it is the ID of the northbridge device to which this CPU belongs
which should be exported.

> Basically, with this NUMA overloading of core id, we are loosing the
> unique id of cores that was in "core id" before.

Yes, I see. Just couldn't imagine that some apps are relying on that info
if there are much more convenient ways of getting it.
Why parsing /proc/cpuinfo for SMT sibling information if we have a
CPU list and mask for this in /sys.

> Without this /proc/cpuinfo output makes sense with "cpu cores" being
> 12 and 12 unique ids for cores under one physical id. With this
> change, "cpu cores" under a package is still 12. But, trying to id
> those 12 cores, one ends up with 0..5 0..5. We are loosing the MSB
> on these IDs only to give some implicit and incomplete information (
> Number of nodes = "cpu cores" / number of unique core ids, but we
> wont tell you here how to identify those nodes. Look at /sys..node
> for that).

Exporting the node_id for the northbridge devices to which the core
is attached would solve this problem.

> Isn't it better to leave /proc/cpuinfo out of this so that older apps
> continue to work and newer apps can use all the /sys fs topology
> information to get the complete info?

BTW, which app is this?
I've quickly checked some tools and couldn't find heavy usage of
/proc/cpuinfo.

> Taking this a bit further, this different cores sharing same core id
> within a package is going to be messy in general. Consider totally
> hypothetical CPU package with 2 nodes and 2 SMT siblings in each node.
> If we follow this current scheme of things of - stuffing node info
> without node id - in proc/cpuinfo, it will look something like
> processor: 0
> physical id: 0
> siblings: 4
> core id: 0
> cpu cores: 4
> 
> processor: 1
> physical id: 0
> siblings: 4
> core id: 0
> cpu cores: 4
>
> processor: 2
> physical id: 0
> siblings: 4
> core id: 0
> cpu cores: 4
> 
> processor: 3
> physical id: 0
> siblings: 4
> core id: 0
> cpu cores: 4

Yep, in theory this could reflect physcial packages having
- 1 core having 4 SMT siblings
- 2 nodes with 2 cores each and 2 SMT siblings
- 4 nodes, 1 core

/proc/cpuinfo is insufficient to reflect full topology information.
Changing core_id you would still get similar entries for physical
packages

- having 2 internal nodes, 1 core each with 2 SMT siblings
- having 2 cores with 2 SMT siblings each

> Which would be very much useless. We should either have "node id" info
> or not change the "core id" meaning to include node info.

Then I'd vote to provide the CPU northbridge node id information.

> >> The change below reverts the cpu_core_id part of that commit and
> >
> > Please don't do that. Potentially you are breaking user space. You
> > rather need to know the core id (0..5) within a node instead of the
> > CoreId (0..11) within the entire package. See AMD's BKDG for family
> > 0x10 CPUs. As a rule of thumb you require the ID of a core within one
> > Node -- not within a package.
> 
> The problem is the core_id change is breaking the userspace decoding
> of package/core/thread info from /proc/cpuinfo.

> Yes. I may need to know core id's in a node. But, /proc/cpuinfo is
> not giving be that info of "ID of core withing one Node". There is
> no Node info there.  Its still giving me Core ID withing a physical
> package with no way of knowing which cores are within a node.

  [...]

> So, if you think cpuinfo is useless for NUMA topology, then why do you
> think core id should be change to reflect NUMA info?

Ehm, sorry I did not decide to not provide the information in
/proc/cpuinfo. IIRC exporting node id in /proc/cpuinfo was part
of the initial topology adaption patch sets that were rejected.

In-kernel we still have the entire info -- but it is just partially
exported (or in too many different places in sysfs) to user space.

  [...]

> Just to summarize, I am of the opinion that if we can't describe
> something in /proc/cpuinfo fully, we should not stuff partial info and
> break existing assumptions

Well, some assumptions might prove false over time.

> and add new assumptions. We have new APIS is /sys anyway to describe
> full topology.

I still recommend to use sysfs info for topology detection.
But I also agree that there is some inconsistency in /proc/cpuinfo

It's the maintainers' call to decide how to fix this inconsistency.
I am fine with both solutions.
But if core_id is not "normalized" anymore I need to do some more review
and testing to ensure that nothing breaks.



Regards,

Andreas


-- 
Operating | Advanced Micro Devices GmbH
  System  | Einsteinring 24, 85609 Dornach b. München, Germany
 Research | Geschäftsführer: Alberto Bozzo, Andrew Bowd
  Center  | Sitz: Dornach, Gemeinde Aschheim, Landkreis München
  (OSRC)  | Registergericht München, HRB Nr. 43632

Re: [PATCH] x86: Wrong /proc/cpuinfo core id on AMD fam_f model_9

[Venkatesh Pallipadi]

On Wed, Aug 18, 2010 at 12:17 PM, Andreas Herrmann
<andreas.herrmann3@amd.com> wrote:
> On Fri, Aug 13, 2010 at 01:52:37PM -0400, Venkatesh Pallipadi wrote:
>> On Fri, Aug 13, 2010 at 12:04 AM, Andreas Herrmann
>> <andreas.herrmann3@amd.com> wrote:
>
>  [...]
>
>> The reason for this patch was a breakage in the app that tries to
>> build CPU maps for packages and cores (Nothing to do with Intel or
>> AMD). The app is using info under /sys/devices/system//node/ and
>> /sys/device/system/cpu/cpu0/cache to get the NUMA node and cache
>> sharing info etc. And looking at cpuinfo for core package info.
>
> Hmm, and why not using .../cpu/cpuX/topology/core_siblings for the
> package info? It just sounds strange to me to use /proc/cpuinfo for
> CPU topology information.

I think its because, the app is supposed to work across different kernels
(with and without /sys topology) and also using /proc/cpuinfo was working OK
until we saw the problem with this particular platform.

>
>> And is getting confused with NUMA overloaded package and core info
>> in /proc/cpunfo. I can of course change this specific app to
>> workaround this. But, I think changing core id to reflect numa-ness
>> without a numa id to go with it is not right.
>
> Using Node ID from SRAT table would not be right.
> You could have just one NUMA node 0 (spanning all packages) but the
> CPUs are still associated to a node within the physical package.
> So, it is the ID of the northbridge device to which this CPU belongs
> which should be exported.

Agree. I said Node ID in generic sense. In this case it will be northbridge dev.

>> Basically, with this NUMA overloading of core id, we are loosing the
>> unique id of cores that was in "core id" before.
>
> Yes, I see. Just couldn't imagine that some apps are relying on that info
> if there are much more convenient ways of getting it.
> Why parsing /proc/cpuinfo for SMT sibling information if we have a
> CPU list and mask for this in /sys.

We can term such apps as "legacy apps" :-)

>> Without this /proc/cpuinfo output makes sense with "cpu cores" being
>> 12 and 12 unique ids for cores under one physical id. With this
>> change, "cpu cores" under a package is still 12. But, trying to id
>> those 12 cores, one ends up with 0..5 0..5. We are loosing the MSB
>> on these IDs only to give some implicit and incomplete information (
>> Number of nodes = "cpu cores" / number of unique core ids, but we
>> wont tell you here how to identify those nodes. Look at /sys..node
>> for that).
>
> Exporting the node_id for the northbridge devices to which the core
> is attached would solve this problem.
>
>> Isn't it better to leave /proc/cpuinfo out of this so that older apps
>> continue to work and newer apps can use all the /sys fs topology
>> information to get the complete info?
>
> BTW, which app is this?
> I've quickly checked some tools and couldn't find heavy usage of
> /proc/cpuinfo.

This is an internal app.

There are few programs reporting CPU topology like hwloc, x86info.
Not sure whether they are affected by this.

I did find few "knowledge-base" articles that explains /proc/cpuinfo
as "same core id" => same core
[like -
http://planet.admon.org/howto/about-cpu-the-logical-and-physical-cores/
http://linuxhunt.blogspot.com/2010/03/understanding-proccpuinfo.html
]

>
>> Taking this a bit further, this different cores sharing same core id
>> within a package is going to be messy in general. Consider totally
>> hypothetical CPU package with 2 nodes and 2 SMT siblings in each node.
>> If we follow this current scheme of things of - stuffing node info
>> without node id - in proc/cpuinfo, it will look something like
>> processor: 0
>> physical id: 0
>> siblings: 4
>> core id: 0
>> cpu cores: 4
>>
>> processor: 1
>> physical id: 0
>> siblings: 4
>> core id: 0
>> cpu cores: 4
>>
>> processor: 2
>> physical id: 0
>> siblings: 4
>> core id: 0
>> cpu cores: 4
>>
>> processor: 3
>> physical id: 0
>> siblings: 4
>> core id: 0
>> cpu cores: 4
>
> Yep, in theory this could reflect physcial packages having
> - 1 core having 4 SMT siblings
> - 2 nodes with 2 cores each and 2 SMT siblings
> - 4 nodes, 1 core
>
> /proc/cpuinfo is insufficient to reflect full topology information.
> Changing core_id you would still get similar entries for physical
> packages
>
> - having 2 internal nodes, 1 core each with 2 SMT siblings
> - having 2 cores with 2 SMT siblings each

Agree. But, identifying this "internal node" topology is a new
requirement and just overloading core id isn't going to be enough.


>> Which would be very much useless. We should either have "node id" info
>> or not change the "core id" meaning to include node info.
>
> Then I'd vote to provide the CPU northbridge node id information.

<snip>
>
>> and add new assumptions. We have new APIS is /sys anyway to describe
>> full topology.
>
> I still recommend to use sysfs info for topology detection.
> But I also agree that there is some inconsistency in /proc/cpuinfo
>
> It's the maintainers' call to decide how to fix this inconsistency.
> I am fine with both solutions.
> But if core_id is not "normalized" anymore I need to do some more review
> and testing to ensure that nothing breaks.
>

hpa/thomas/ingo: Please let us know how you prefer to deal with this...

Thanks,
Venki