Message-ID: <AANLkTineUH2Z2CYVEz001S7srZHmF73qZJfQU9WOjVnM@mail.gmail.com>
Date:	Fri, 13 Aug 2010 10:52:37 -0700
From:	Venkatesh Pallipadi <venki@...gle.com>
To:	Andreas Herrmann <andreas.herrmann3@....com>
Cc:	Ingo Molnar <mingo@...e.hu>, "H. Peter Anvin" <hpa@...or.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] x86: Wrong /proc/cpuinfo core id on AMD fam_f model_9

On Fri, Aug 13, 2010 at 12:04 AM, Andreas Herrmann
<andreas.herrmann3@....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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists