Re: [PATCH 0/14] v2 multi-die/package topology support
On Tue, Apr 30, 2019 at 5:33 AM Borislav Petkov wrote: > So that die thing has only small relevance to some software, as you say: > > "These topology changes primarily impact parts of the kernel and some > applciations that care about package MSR scope." > > So if there's no real need to add it there, then it probably shouldn't > be added. The topology is already too complex - so much so, that tools > are even generating PDFs from it :) Agreed. Let's keep /proc/cpuinfo simple. If a case emerges where it makes sense to add more detail there, it is trivial do add later. thanks, Len Brown, Intel Open Source Technology Center
Re: [PATCH 0/14] v2 multi-die/package topology support
On Tue, Apr 30, 2019 at 02:50:58AM -0400, Len Brown wrote: > If one were to make a change here, I'd consider adding the (physical) die_id, > though it is already in sysfs topology as an attribute. From: Documentation/x86/topology.txt "The kernel does not care about the concept of physical sockets because a socket has no relevance to software. It's an electromechanical component. In the past a socket always contained a single package (see below), but with the advent of Multi Chip Modules (MCM) a socket can hold more than one package. So there might be still references to sockets in the code, but they are of historical nature and should be cleaned up." So that die thing has only small relevance to some software, as you say: "These topology changes primarily impact parts of the kernel and some applciations that care about package MSR scope." So if there's no real need to add it there, then it probably shouldn't be added. The topology is already too complex - so much so, that tools are even generating PDFs from it :) -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [PATCH 0/14] v2 multi-die/package topology support
On Tue, Feb 26, 2019 at 2:05 PM Peter Zijlstra wrote: > > On Tue, Feb 26, 2019 at 01:19:58AM -0500, Len Brown wrote: > > Documentation/cputopology.txt| 72 ++- > > Documentation/x86/topology.txt | 6 +- > > arch/x86/include/asm/processor.h | 5 +- > > arch/x86/include/asm/smp.h | 1 + > > arch/x86/include/asm/topology.h | 5 ++ > > arch/x86/kernel/cpu/topology.c | 85 > > +--- > > arch/x86/kernel/smpboot.c| 73 +++- > > arch/x86/xen/smp_pv.c| 1 + > > drivers/base/topology.c | 22 +++ > > drivers/hwmon/coretemp.c | 9 +-- > > drivers/powercap/intel_rapl.c| 75 +--- > > drivers/thermal/intel/x86_pkg_temp_thermal.c | 9 +-- > > include/linux/topology.h | 6 ++ > > 13 files changed, 276 insertions(+), 93 deletions(-) > > Should we not also have changes to > arch/x86/kernel/cpu/proc.c:show_cpuinfo_cores() ? Good question. I was thinking that /proc/cpuinfo was sort of the legacy API, and adding a field might break something. While adding an attribute to sysfs topology directory was the compatible/safe way to make additions. /proc/cpuinfo has these fields today: physical id : 0 this is the physical package id siblings : 8 this is the count of cpus in the same package core id : 3 this is cpu_core_id cpu cores : 4 this is booted_cores If one were to make a change here, I'd consider adding the (physical) die_id, though it is already in sysfs topology as an attribute. Not sure if it would then make sense to print the count of cpus in the die. Not sure what I'd name it, and this info is already in sysfs as a map and list. Len Brown, Intel Open Source Technology Center
Re: [PATCH 0/14] v2 multi-die/package topology support
Le 12/04/2019 à 21:52, Len Brown a écrit : I think I prefer 's/threads/cpus/g' on that. Threads makes me think SMT, and I don't think there's any guarantee the part in question will have SMT on. >>> I think 'threads' is a bit confusing as well. We seem to be using 'cpu' >>> everywhere for something we can schedule tasks on, including the sysfs >>> /sys/devices/system/cpu/ subdirs for each SMT thread on SMT systems. > I agree with Peter and Morten. > "cpu" is more clear and consistent than "thread" here. > I'll spin the series with that string changed. Agreed, I should have used that suffix from the beginning. Brice
Re: [PATCH 0/14] v2 multi-die/package topology support
> > > I think I prefer 's/threads/cpus/g' on that. Threads makes me think SMT, > > > and I don't think there's any guarantee the part in question will have > > > SMT on. > > > > I think 'threads' is a bit confusing as well. We seem to be using 'cpu' > > everywhere for something we can schedule tasks on, including the sysfs > > /sys/devices/system/cpu/ subdirs for each SMT thread on SMT systems. I agree with Peter and Morten. "cpu" is more clear and consistent than "thread" here. I'll spin the series with that string changed. > > Another thing that I find confusing is that with this series we a new > > die id/mask which is totally unrelated to the DIE level in the > > sched_domain hierarchy. We should rename DIE level to something that > > reflects what it really is. If we can agree on that ;-) > > > > NODE level? Cache topology and node interconnect topology impact performance, and so we what we look at, when we decided to run something on this CPU or that one. That logical topology lives within the physical package and die topology, but doesn't necessarily match it. For example, caches can be shared or split into pieces inside a package or die. Logical nodes may match die boundaries, or there may be multiple logical nodes within a single physical package or die. We have mechanisms for explicitly enumerating the caches, and for nodes. This patch series does not touch those mechanisms. The reason we need to know about physical packages and die is that there are other things associated with them. eg. power and temperature domains, and certain system registers follow these physical boundaries. Code that talks to those items needs to be able to understand these physical boundaries. I hope that helps. thanks, Len Brown, Intel Open Source Technology Center
Re: [PATCH 0/14] v2 multi-die/package topology support
On Tue, Feb 26, 2019 at 1:51 PM Peter Zijlstra wrote: > > On Tue, Feb 26, 2019 at 01:19:58AM -0500, Len Brown wrote: > > Restored sysfs core_siblings, core_siblings_list > > > > v1 proposed re-defining this existing attribute to > > be the threads in a die, rather than in a package. > > > > For compatibility, decided rather to keep this > > attribute unchanged, for now, even though > > its name makes little sense, and it makes > > no sense in a multi-die system. > > So why do things that make no sense? >>> 7) /sys/devices/system/cpu/cpuX/topology/core_siblings: >>> >>> internal kernel map of cpuX's hardware threads within the same >>> physical_package_id. This definition tells you what cpus are in what package. That is fine, it is useful, and it is in use. What doesn't make sense is that it is called "core_siblings". Who is to say that the map of CPUs inside a package has anything to do with "cores"? Sometimes it does, sometimes it doesn't... > What breaks? User space applications, such as lscpu and hwloc are using this attribute per its definition, to figure out what cpus are in what packages. If we change the definition to match the attribute's name, they break. If we change the attribute name to match the definition, they break. So the plan is to simply leave this attribute and its definition in place, deprecate it, and move to the new attribute names that don't have the word "siblings" in them -- which imply a known fixed topology. We can schedule this attribute to be deleted some day, but changing it and hoping you've updated all of user-space would be a unnecessary pain. Len Brown, Intel Open Source Technology Center
Re: [PATCH 0/14] v2 multi-die/package topology support
On Thu, 7 Mar 2019, Morten Rasmussen wrote: > On Tue, Feb 26, 2019 at 07:53:58PM +0100, Peter Zijlstra wrote: > > On Tue, Feb 26, 2019 at 01:19:58AM -0500, Len Brown wrote: > > > Added sysfs core_threads, core_threads_list > > > > > > Added this attribute to show which threads siblings in a core. > > > Exactly same as "thread_siblings", a name now deprecated. > > > This attribute name and definition is immune to future > > > topology changes. > > > > > > Suggested by Brice. > > > > I think I prefer 's/threads/cpus/g' on that. Threads makes me think SMT, > > and I don't think there's any guarantee the part in question will have > > SMT on. > > I think 'threads' is a bit confusing as well. We seem to be using 'cpu' > everywhere for something we can schedule tasks on, including the sysfs > /sys/devices/system/cpu/ subdirs for each SMT thread on SMT systems. > > Another thing that I find confusing is that with this series we a new > die id/mask which is totally unrelated to the DIE level in the > sched_domain hierarchy. We should rename DIE level to something that > reflects what it really is. If we can agree on that ;-) > > NODE level? Any conclusions here?
Re: [PATCH 0/14] v2 multi-die/package topology support
On Tue, 26 Feb 2019, Len Brown wrote: > This patch series does 4 things. > > 1. Parses the new CPUID.1F leaf to discover multi-die/package topology > > 2. Export multi-die topology inside the kernel > > 3. Update 3 places (coretemp, pkgtemp, rapl) that that need to know >the difference between die and package-scope MSR. > >(Note: Kan Liang has a patch series on top of this one to similarly >make the uncore perf code multi-die/package aware.) > > 4. Export multi-die topology to user-space via sysfs Aside of the few nitpicks (which apply to several patches) this looks very nice. Thanks, tglx
Re: [PATCH 0/14] v2 multi-die/package topology support
On Tue, Feb 26, 2019 at 07:53:58PM +0100, Peter Zijlstra wrote: > On Tue, Feb 26, 2019 at 01:19:58AM -0500, Len Brown wrote: > > Added sysfs package_threads, package_threads_list > > > > Added this attribute to show threads siblings in a package. > > Exactly same as "core_siblings above", a name now deprecated. > > This attribute name and definition is immune to future > > topology changes. > > > > Suggested by Brice. > > > > Added sysfs die_threads, die_threads_list > > > > Added this attribute to show which threads siblings in a die. > > V1 had proposed putting this info into "core_siblings", but we > > decided to leave that legacy attribute alone. > > This attribute name and definition is immune to future > > topology changes. > > > > On a single die-package system this attribute has same contents > > as "package_threads". > > > > Suggested by Brice. > > > > Added sysfs core_threads, core_threads_list > > > > Added this attribute to show which threads siblings in a core. > > Exactly same as "thread_siblings", a name now deprecated. > > This attribute name and definition is immune to future > > topology changes. > > > > Suggested by Brice. > > I think I prefer 's/threads/cpus/g' on that. Threads makes me think SMT, > and I don't think there's any guarantee the part in question will have > SMT on. I think 'threads' is a bit confusing as well. We seem to be using 'cpu' everywhere for something we can schedule tasks on, including the sysfs /sys/devices/system/cpu/ subdirs for each SMT thread on SMT systems. Another thing that I find confusing is that with this series we a new die id/mask which is totally unrelated to the DIE level in the sched_domain hierarchy. We should rename DIE level to something that reflects what it really is. If we can agree on that ;-) NODE level? Morten
Re: [PATCH 0/14] v2 multi-die/package topology support
On Tue, Feb 26, 2019 at 01:19:58AM -0500, Len Brown wrote: > Documentation/cputopology.txt| 72 ++- > Documentation/x86/topology.txt | 6 +- > arch/x86/include/asm/processor.h | 5 +- > arch/x86/include/asm/smp.h | 1 + > arch/x86/include/asm/topology.h | 5 ++ > arch/x86/kernel/cpu/topology.c | 85 > +--- > arch/x86/kernel/smpboot.c| 73 +++- > arch/x86/xen/smp_pv.c| 1 + > drivers/base/topology.c | 22 +++ > drivers/hwmon/coretemp.c | 9 +-- > drivers/powercap/intel_rapl.c| 75 +--- > drivers/thermal/intel/x86_pkg_temp_thermal.c | 9 +-- > include/linux/topology.h | 6 ++ > 13 files changed, 276 insertions(+), 93 deletions(-) Should we not also have changes to arch/x86/kernel/cpu/proc.c:show_cpuinfo_cores() ?
Re: [PATCH 0/14] v2 multi-die/package topology support
On Tue, Feb 26, 2019 at 01:19:58AM -0500, Len Brown wrote: > Added sysfs package_threads, package_threads_list > > Added this attribute to show threads siblings in a package. > Exactly same as "core_siblings above", a name now deprecated. > This attribute name and definition is immune to future > topology changes. > > Suggested by Brice. > > Added sysfs die_threads, die_threads_list > > Added this attribute to show which threads siblings in a die. > V1 had proposed putting this info into "core_siblings", but we > decided to leave that legacy attribute alone. > This attribute name and definition is immune to future > topology changes. > > On a single die-package system this attribute has same contents > as "package_threads". > > Suggested by Brice. > > Added sysfs core_threads, core_threads_list > > Added this attribute to show which threads siblings in a core. > Exactly same as "thread_siblings", a name now deprecated. > This attribute name and definition is immune to future > topology changes. > > Suggested by Brice. I think I prefer 's/threads/cpus/g' on that. Threads makes me think SMT, and I don't think there's any guarantee the part in question will have SMT on.
Re: [PATCH 0/14] v2 multi-die/package topology support
On Tue, Feb 26, 2019 at 01:19:58AM -0500, Len Brown wrote: > Restored sysfs core_siblings, core_siblings_list > > v1 proposed re-defining this existing attribute to > be the threads in a die, rather than in a package. > > For compatibility, decided rather to keep this > attribute unchanged, for now, even though > its name makes little sense, and it makes > no sense in a multi-die system. So why do things that make no sense? What breaks?
