* [PATCH] arm64: acpi: reenumerate topology ids @ 2018-06-28 14:51 Andrew Jones 2018-06-28 16:30 ` Sudeep Holla 0 siblings, 1 reply; 20+ messages in thread From: Andrew Jones @ 2018-06-28 14:51 UTC (permalink / raw) To: linux-arm-kernel, linux-kernel Cc: jeremy.linton, ard.biesheuvel, sudeep.holla, shunyong.yang, yu.zheng, catalin.marinas, will.deacon When booting with devicetree, and the devicetree has the cpu-map node, the topology IDs that are visible from sysfs are generated with counters. ACPI, on the other hand, uses ACPI table pointer offsets, which, while guaranteed to be unique, look a bit weird. Instead, we can generate DT identical topology IDs for ACPI by just using counters for the leaf nodes and by remapping the non-leaf table pointer offsets to counters. Cc: Jeremy Linton <jeremy.linton@arm.com> Cc: Sudeep Holla <sudeep.holla@arm.com> Signed-off-by: Andrew Jones <drjones@redhat.com> --- v1: Reworked this since the RFC in order to make the algorithm more obvious. It wasn't clear in the RFC that the ACPI nodes could be in any order, although they could have been. I've tested that this works with nodes in arbitrary order by hacking the QEMU PPTT table generator[*]. Note, while this produces equivalent topology IDs to what the DT cpu-map node produces for all sane configs, if PEs are threads (have MPIDR.MT set), but the cpu-map does not specify threads, then, while the DT parsing code will happily call the threads "cores", ACPI will see that the PPTT leaf nodes are for threads and produce different topology IDs. I see this difference as a bug with the DT parsing which can be addressed separately. [*] https://github.com/rhdrjones/qemu/commits/virt-cpu-topology arch/arm64/kernel/topology.c | 70 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 55 insertions(+), 15 deletions(-) diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c index f845a8617812..7ef457401b24 100644 --- a/arch/arm64/kernel/topology.c +++ b/arch/arm64/kernel/topology.c @@ -316,6 +316,10 @@ static void __init reset_cpu_topology(void) } #ifdef CONFIG_ACPI + +#define acpi_topology_mktag(x) (-((x) + 1)) +#define acpi_topology_istag(x) ((x) < 0) + /* * Propagate the topology information of the processor_topology_node tree to the * cpu_topology array. @@ -323,27 +327,31 @@ static void __init reset_cpu_topology(void) static int __init parse_acpi_topology(void) { bool is_threaded; - int cpu, topology_id; + int package_id = 0; + int cpu, ret; is_threaded = read_cpuid_mpidr() & MPIDR_MT_BITMASK; + /* + * Loop through all PEs twice. In the first loop store parent + * tags into the IDs. In the second loop we reset the IDs as + * 0..N-1 per parent tag. + */ + for_each_possible_cpu(cpu) { int i, cache_id; - topology_id = find_acpi_cpu_topology(cpu, 0); - if (topology_id < 0) - return topology_id; - - if (is_threaded) { - cpu_topology[cpu].thread_id = topology_id; - topology_id = find_acpi_cpu_topology(cpu, 1); - cpu_topology[cpu].core_id = topology_id; - } else { - cpu_topology[cpu].thread_id = -1; - cpu_topology[cpu].core_id = topology_id; - } - topology_id = find_acpi_cpu_topology_package(cpu); - cpu_topology[cpu].package_id = topology_id; + ret = find_acpi_cpu_topology(cpu, 0); + if (ret < 0) + return ret; + + if (is_threaded) + ret = find_acpi_cpu_topology(cpu, 1); + else + cpu_topology[cpu].thread_id = -1; + cpu_topology[cpu].core_id = acpi_topology_mktag(ret); + ret = find_acpi_cpu_topology_package(cpu); + cpu_topology[cpu].package_id = acpi_topology_mktag(ret); i = acpi_find_last_cache_level(cpu); @@ -358,6 +366,38 @@ static int __init parse_acpi_topology(void) } } + for_each_possible_cpu(cpu) { + int package_tag = cpu_topology[cpu].package_id; + int core_id = 0, cpu2; + + if (!acpi_topology_istag(package_tag)) + continue; + + for_each_possible_cpu(cpu2) { + if (cpu_topology[cpu2].package_id != package_tag) + continue; + + if (is_threaded) { + int core_tag = cpu_topology[cpu2].core_id; + int thread_id = 0, cpu3; + + for_each_possible_cpu(cpu3) { + if (cpu_topology[cpu3].core_id != core_tag) + continue; + + cpu_topology[cpu3].thread_id = thread_id++; + cpu_topology[cpu3].core_id = core_id; + cpu_topology[cpu3].package_id = package_id; + } + ++core_id; + } else { + cpu_topology[cpu2].core_id = core_id++; + cpu_topology[cpu2].package_id = package_id; + } + } + ++package_id; + } + return 0; } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] arm64: acpi: reenumerate topology ids 2018-06-28 14:51 [PATCH] arm64: acpi: reenumerate topology ids Andrew Jones @ 2018-06-28 16:30 ` Sudeep Holla 2018-06-28 17:12 ` Jeremy Linton 2018-06-28 17:32 ` Andrew Jones 0 siblings, 2 replies; 20+ messages in thread From: Sudeep Holla @ 2018-06-28 16:30 UTC (permalink / raw) To: Andrew Jones, linux-arm-kernel, linux-kernel, jeremy.linton Cc: Sudeep Holla, ard.biesheuvel, shunyong.yang, yu.zheng, catalin.marinas, will.deacon On 28/06/18 15:51, Andrew Jones wrote: > When booting with devicetree, and the devicetree has the cpu-map > node, the topology IDs that are visible from sysfs are generated > with counters. ACPI, on the other hand, uses ACPI table pointer > offsets, which, while guaranteed to be unique, look a bit weird. > Instead, we can generate DT identical topology IDs for ACPI by > just using counters for the leaf nodes and by remapping the > non-leaf table pointer offsets to counters. > > Cc: Jeremy Linton <jeremy.linton@arm.com> > Cc: Sudeep Holla <sudeep.holla@arm.com> > Signed-off-by: Andrew Jones <drjones@redhat.com> > --- > > v1: > Reworked this since the RFC in order to make the algorithm more > obvious. It wasn't clear in the RFC that the ACPI nodes could be > in any order, although they could have been. I've tested that > this works with nodes in arbitrary order by hacking the QEMU > PPTT table generator[*]. > > Note, while this produces equivalent topology IDs to what the > DT cpu-map node produces for all sane configs, if PEs are > threads (have MPIDR.MT set), but the cpu-map does not specify > threads, then, while the DT parsing code will happily call the > threads "cores", ACPI will see that the PPTT leaf nodes are for > threads and produce different topology IDs. I see this difference > as a bug with the DT parsing which can be addressed separately. > > [*] https://github.com/rhdrjones/qemu/commits/virt-cpu-topology > > > arch/arm64/kernel/topology.c | 70 ++++++++++++++++++++++++++++++++++---------- > 1 file changed, 55 insertions(+), 15 deletions(-) > > diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c > index f845a8617812..7ef457401b24 100644 > --- a/arch/arm64/kernel/topology.c > +++ b/arch/arm64/kernel/topology.c > @@ -316,6 +316,10 @@ static void __init reset_cpu_topology(void) > } > > #ifdef CONFIG_ACPI > + > +#define acpi_topology_mktag(x) (-((x) + 1)) > +#define acpi_topology_istag(x) ((x) < 0) > + > /* > * Propagate the topology information of the processor_topology_node tree to the > * cpu_topology array. > @@ -323,27 +327,31 @@ static void __init reset_cpu_topology(void) > static int __init parse_acpi_topology(void) > { > bool is_threaded; > - int cpu, topology_id; > + int package_id = 0; > + int cpu, ret; > > is_threaded = read_cpuid_mpidr() & MPIDR_MT_BITMASK; > > + /* > + * Loop through all PEs twice. In the first loop store parent > + * tags into the IDs. In the second loop we reset the IDs as > + * 0..N-1 per parent tag. > + */ > + > for_each_possible_cpu(cpu) { > int i, cache_id; > > - topology_id = find_acpi_cpu_topology(cpu, 0); > - if (topology_id < 0) > - return topology_id; > - > - if (is_threaded) { > - cpu_topology[cpu].thread_id = topology_id; > - topology_id = find_acpi_cpu_topology(cpu, 1); > - cpu_topology[cpu].core_id = topology_id; > - } else { > - cpu_topology[cpu].thread_id = -1; > - cpu_topology[cpu].core_id = topology_id; > - } > - topology_id = find_acpi_cpu_topology_package(cpu); > - cpu_topology[cpu].package_id = topology_id; > + ret = find_acpi_cpu_topology(cpu, 0); > + if (ret < 0) > + return ret; > + > + if (is_threaded) > + ret = find_acpi_cpu_topology(cpu, 1); > + else > + cpu_topology[cpu].thread_id = -1; > + cpu_topology[cpu].core_id = acpi_topology_mktag(ret); > + ret = find_acpi_cpu_topology_package(cpu); > + cpu_topology[cpu].package_id = acpi_topology_mktag(ret); I am not sure if we can ever guarantee that DT and ACPI will get the same ids whatever counter we use as it depends on the order presented in the firmware(DT or ACPI). So I am not for generating ids for core and threads in that way. So I would like to keep it simple and just have this counters for package ids as demonstrated in Shunyong's patch. Also looking @ topology_get_acpi_cpu_tag again now, we should have valid flag check instead of level = 0. Jeremy ? -- Regards, Sudeep diff --git i/drivers/acpi/pptt.c w/drivers/acpi/pptt.c index e5ea1974d1e3..795c43053570 100644 --- i/drivers/acpi/pptt.c +++ w/drivers/acpi/pptt.c @@ -481,8 +481,12 @@ static int topology_get_acpi_cpu_tag(struct acpi_table_header *table, if (cpu_node) { cpu_node = acpi_find_processor_package_id(table, cpu_node, level, flag); - /* Only the first level has a guaranteed id */ - if (level == 0) + /* + * As per specification if the processor structure represents + * an actual processor ACPI processor ID must be valid, which + * should be always true for level 0 + */ + if (cpu_node-flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID) return cpu_node->acpi_processor_id; return ACPI_PTR_DIFF(cpu_node, table); } ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] arm64: acpi: reenumerate topology ids 2018-06-28 16:30 ` Sudeep Holla @ 2018-06-28 17:12 ` Jeremy Linton 2018-06-29 10:53 ` Sudeep Holla 2018-06-28 17:32 ` Andrew Jones 1 sibling, 1 reply; 20+ messages in thread From: Jeremy Linton @ 2018-06-28 17:12 UTC (permalink / raw) To: Sudeep Holla, Andrew Jones, linux-arm-kernel, linux-kernel Cc: ard.biesheuvel, shunyong.yang, yu.zheng, catalin.marinas, will.deacon Hi, On 06/28/2018 11:30 AM, Sudeep Holla wrote: > > > On 28/06/18 15:51, Andrew Jones wrote: >> When booting with devicetree, and the devicetree has the cpu-map >> node, the topology IDs that are visible from sysfs are generated >> with counters. ACPI, on the other hand, uses ACPI table pointer >> offsets, which, while guaranteed to be unique, look a bit weird. >> Instead, we can generate DT identical topology IDs for ACPI by >> just using counters for the leaf nodes and by remapping the >> non-leaf table pointer offsets to counters. >> >> Cc: Jeremy Linton <jeremy.linton@arm.com> >> Cc: Sudeep Holla <sudeep.holla@arm.com> >> Signed-off-by: Andrew Jones <drjones@redhat.com> >> --- >> >> v1: >> Reworked this since the RFC in order to make the algorithm more >> obvious. It wasn't clear in the RFC that the ACPI nodes could be >> in any order, although they could have been. I've tested that >> this works with nodes in arbitrary order by hacking the QEMU >> PPTT table generator[*]. >> >> Note, while this produces equivalent topology IDs to what the >> DT cpu-map node produces for all sane configs, if PEs are >> threads (have MPIDR.MT set), but the cpu-map does not specify >> threads, then, while the DT parsing code will happily call the >> threads "cores", ACPI will see that the PPTT leaf nodes are for >> threads and produce different topology IDs. I see this difference >> as a bug with the DT parsing which can be addressed separately. >> >> [*] https://github.com/rhdrjones/qemu/commits/virt-cpu-topology >> >> >> arch/arm64/kernel/topology.c | 70 ++++++++++++++++++++++++++++++++++---------- >> 1 file changed, 55 insertions(+), 15 deletions(-) >> >> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c >> index f845a8617812..7ef457401b24 100644 >> --- a/arch/arm64/kernel/topology.c >> +++ b/arch/arm64/kernel/topology.c >> @@ -316,6 +316,10 @@ static void __init reset_cpu_topology(void) >> } >> >> #ifdef CONFIG_ACPI >> + >> +#define acpi_topology_mktag(x) (-((x) + 1)) >> +#define acpi_topology_istag(x) ((x) < 0) >> + >> /* >> * Propagate the topology information of the processor_topology_node tree to the >> * cpu_topology array. >> @@ -323,27 +327,31 @@ static void __init reset_cpu_topology(void) >> static int __init parse_acpi_topology(void) >> { >> bool is_threaded; >> - int cpu, topology_id; >> + int package_id = 0; >> + int cpu, ret; >> >> is_threaded = read_cpuid_mpidr() & MPIDR_MT_BITMASK; >> >> + /* >> + * Loop through all PEs twice. In the first loop store parent >> + * tags into the IDs. In the second loop we reset the IDs as >> + * 0..N-1 per parent tag. >> + */ >> + >> for_each_possible_cpu(cpu) { >> int i, cache_id; >> >> - topology_id = find_acpi_cpu_topology(cpu, 0); >> - if (topology_id < 0) >> - return topology_id; >> - >> - if (is_threaded) { >> - cpu_topology[cpu].thread_id = topology_id; >> - topology_id = find_acpi_cpu_topology(cpu, 1); >> - cpu_topology[cpu].core_id = topology_id; >> - } else { >> - cpu_topology[cpu].thread_id = -1; >> - cpu_topology[cpu].core_id = topology_id; >> - } >> - topology_id = find_acpi_cpu_topology_package(cpu); >> - cpu_topology[cpu].package_id = topology_id; >> + ret = find_acpi_cpu_topology(cpu, 0); >> + if (ret < 0) >> + return ret; >> + >> + if (is_threaded) >> + ret = find_acpi_cpu_topology(cpu, 1); >> + else >> + cpu_topology[cpu].thread_id = -1; >> + cpu_topology[cpu].core_id = acpi_topology_mktag(ret); >> + ret = find_acpi_cpu_topology_package(cpu); >> + cpu_topology[cpu].package_id = acpi_topology_mktag(ret); > > I am not sure if we can ever guarantee that DT and ACPI will get the > same ids whatever counter we use as it depends on the order presented in > the firmware(DT or ACPI). So I am not for generating ids for core and > threads in that way. > > So I would like to keep it simple and just have this counters for > package ids as demonstrated in Shunyong's patch. So, currently on a non threaded system, the core id's look nice because they are just the ACPI ids. Its the package id's that look strange, we could just fix the package ids, but on threaded machines the threads have the nice acpi ids, and the core ids are then funny numbers. So, I suspect that is driving this as much as the strange package ids. (and as a side, I actually like the PE has a acpi id behavior, but for threads its being lost with this patch...) Given i've seen odd package/core ids on x86s a few years ago, it never bothered me much as a lot of userspace tools are just using what is effectively the logical processor number anyway. Further, this table may be having some clarifications published for some of these fields. I'm not sure the final wording will help us, but it might. > > > Also looking @ topology_get_acpi_cpu_tag again now, we should have > valid flag check instead of level = 0. Jeremy ? ? I'm not sure I understand, but your saying for the leaf nodes we should be checking the valid flag rather than whether the caller requested level 0? I don't think that is right. The original PPTT spec is unclear about proper use of the valid flag. So, while this part of the spec may be clarified in the near future, (AFAIK) there are already tables in the wild which fail to set valid on the leaf nodes! So I think using the level check is the safest at the moment. Depending on what happens with the next rev of the ACPI spec (or whenever) some of this whole discussion might be bypassed simply by using whatever id is marked valid on the node, as you suggest, but until then... ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] arm64: acpi: reenumerate topology ids 2018-06-28 17:12 ` Jeremy Linton @ 2018-06-29 10:53 ` Sudeep Holla 2018-06-29 11:42 ` Andrew Jones 0 siblings, 1 reply; 20+ messages in thread From: Sudeep Holla @ 2018-06-29 10:53 UTC (permalink / raw) To: Jeremy Linton Cc: Andrew Jones, linux-arm-kernel, linux-kernel, ard.biesheuvel, shunyong.yang, yu.zheng, catalin.marinas, will.deacon On Thu, Jun 28, 2018 at 12:12:00PM -0500, Jeremy Linton wrote: > Hi, > > On 06/28/2018 11:30 AM, Sudeep Holla wrote: [...] > >I am not sure if we can ever guarantee that DT and ACPI will get the > >same ids whatever counter we use as it depends on the order presented in > >the firmware(DT or ACPI). So I am not for generating ids for core and > >threads in that way. > > > >So I would like to keep it simple and just have this counters for > >package ids as demonstrated in Shunyong's patch. > > So, currently on a non threaded system, the core id's look nice because they > are just the ACPI ids. Its the package id's that look strange, we could just > fix the package ids, but on threaded machines the threads have the nice acpi > ids, and the core ids are then funny numbers. So, I suspect that is driving > this as much as the strange package ids. > Yes, I know that and that's what made be look at topology_get_acpi_cpu_tag For me, if the PPTT has valid ID, we should use that. Just becuase DT lacks it and uses counter doesn't mean ACPI also needs to follow that. I am sure some vendor will put valid UID and expect that to be in the sysfs. > (and as a side, I actually like the PE has a acpi id behavior, but for > threads its being lost with this patch...) > > Given i've seen odd package/core ids on x86s a few years ago, it never > bothered me much as a lot of userspace tools are just using what is > effectively the logical processor number anyway. > Indeed. > Further, this table may be having some clarifications published for some of > these fields. I'm not sure the final wording will help us, but it might. > I prefer that. We can then just use the IDs if valid, else offset if PPTT ignores those fields. What's the point in having all these in specification and vendors ignoring them altogether. Since this is new feature, we can always enforce and say we don't care if firmware doesn't, sorry. > > > > > >Also looking @ topology_get_acpi_cpu_tag again now, we should have > >valid flag check instead of level = 0. Jeremy ? > > ? I'm not sure I understand, but your saying for the leaf nodes we should be > checking the valid flag rather than whether the caller requested level 0? > > I don't think that is right. The original PPTT spec is unclear about proper > use of the valid flag. So, while this part of the spec may be clarified in > the near future, (AFAIK) there are already tables in the wild which fail to > set valid on the leaf nodes! So I think using the level check is the safest > at the moment. > If it's unclear, we can fix it. I really don't like to support something based on the counters as the logic is arbitrary. > Depending on what happens with the next rev of the ACPI spec (or whenever) > some of this whole discussion might be bypassed simply by using whatever id > is marked valid on the node, as you suggest, but until then... I will check if that can be done or enforced. I am sure if we add some counter based logic, we will break something in future. So I would rather stay with UID/offset though it's not mandatory in the specification. -- Regards, Sudeep ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] arm64: acpi: reenumerate topology ids 2018-06-29 10:53 ` Sudeep Holla @ 2018-06-29 11:42 ` Andrew Jones 2018-06-29 11:55 ` Andrew Jones 2018-06-29 13:38 ` Sudeep Holla 0 siblings, 2 replies; 20+ messages in thread From: Andrew Jones @ 2018-06-29 11:42 UTC (permalink / raw) To: Sudeep Holla Cc: Jeremy Linton, linux-arm-kernel, linux-kernel, ard.biesheuvel, shunyong.yang, yu.zheng, catalin.marinas, will.deacon On Fri, Jun 29, 2018 at 11:53:34AM +0100, Sudeep Holla wrote: > On Thu, Jun 28, 2018 at 12:12:00PM -0500, Jeremy Linton wrote: > > Hi, > > > > On 06/28/2018 11:30 AM, Sudeep Holla wrote: > > [...] > > > >I am not sure if we can ever guarantee that DT and ACPI will get the > > >same ids whatever counter we use as it depends on the order presented in > > >the firmware(DT or ACPI). So I am not for generating ids for core and > > >threads in that way. > > > > > >So I would like to keep it simple and just have this counters for > > >package ids as demonstrated in Shunyong's patch. > > > > So, currently on a non threaded system, the core id's look nice because they > > are just the ACPI ids. Its the package id's that look strange, we could just > > fix the package ids, but on threaded machines the threads have the nice acpi > > ids, and the core ids are then funny numbers. So, I suspect that is driving > > this as much as the strange package ids. > > > > Yes, I know that and that's what made be look at topology_get_acpi_cpu_tag > For me, if the PPTT has valid ID, we should use that. Just becuase DT lacks > it and uses counter doesn't mean ACPI also needs to follow that. AFAIK, a valid ACPI UID doesn't need to be something derivable directly from the hardware, so it's just as arbitrary as the CPU phandle that is in the DT cpu-map, i.e. DT *does* have an analogous leaf node integer. > > I am sure some vendor will put valid UID and expect that to be in the > sysfs. I can't think of any reason that would be useful, especially when the UID is for a thread, which isn't even displayed by sysfs. > > > (and as a side, I actually like the PE has a acpi id behavior, but for > > threads its being lost with this patch...) > > > > Given i've seen odd package/core ids on x86s a few years ago, it never So this inspired me to grep some x86 topology code. I found arch/x86/kernel/smpboot.c:topology_update_package_map(), which uses a counter to set the logical package id and Documentation/x86/topology.txt states """ - cpuinfo_x86.logical_id: The logical ID of the package. As we do not trust BIOSes to enumerate the packages in a consistent way, we introduced the concept of logical package ID so we can sanely calculate the number of maximum possible packages in the system and have the packages enumerated linearly. """ Which I see as x86 precedent for the consistency argument I made in my other reply. Thanks, drew ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] arm64: acpi: reenumerate topology ids 2018-06-29 11:42 ` Andrew Jones @ 2018-06-29 11:55 ` Andrew Jones 2018-06-29 13:48 ` Sudeep Holla 2018-06-29 13:38 ` Sudeep Holla 1 sibling, 1 reply; 20+ messages in thread From: Andrew Jones @ 2018-06-29 11:55 UTC (permalink / raw) To: Sudeep Holla Cc: Jeremy Linton, linux-arm-kernel, linux-kernel, ard.biesheuvel, shunyong.yang, yu.zheng, catalin.marinas, will.deacon On Fri, Jun 29, 2018 at 01:42:27PM +0200, Andrew Jones wrote: > On Fri, Jun 29, 2018 at 11:53:34AM +0100, Sudeep Holla wrote: > > On Thu, Jun 28, 2018 at 12:12:00PM -0500, Jeremy Linton wrote: > > > Hi, > > > > > > On 06/28/2018 11:30 AM, Sudeep Holla wrote: > > > > [...] > > > > > >I am not sure if we can ever guarantee that DT and ACPI will get the > > > >same ids whatever counter we use as it depends on the order presented in > > > >the firmware(DT or ACPI). So I am not for generating ids for core and > > > >threads in that way. > > > > > > > >So I would like to keep it simple and just have this counters for > > > >package ids as demonstrated in Shunyong's patch. > > > > > > So, currently on a non threaded system, the core id's look nice because they > > > are just the ACPI ids. Its the package id's that look strange, we could just > > > fix the package ids, but on threaded machines the threads have the nice acpi > > > ids, and the core ids are then funny numbers. So, I suspect that is driving > > > this as much as the strange package ids. > > > > > > > Yes, I know that and that's what made be look at topology_get_acpi_cpu_tag > > For me, if the PPTT has valid ID, we should use that. Just becuase DT lacks > > it and uses counter doesn't mean ACPI also needs to follow that. > > AFAIK, a valid ACPI UID doesn't need to be something derivable directly > from the hardware, so it's just as arbitrary as the CPU phandle that is > in the DT cpu-map, i.e. DT *does* have an analogous leaf node integer. > > > > > I am sure some vendor will put valid UID and expect that to be in the > > sysfs. > > I can't think of any reason that would be useful, especially when the > UID is for a thread, which isn't even displayed by sysfs. > > > > > > (and as a side, I actually like the PE has a acpi id behavior, but for > > > threads its being lost with this patch...) > > > > > > Given i've seen odd package/core ids on x86s a few years ago, it never > > So this inspired me to grep some x86 topology code. I found > arch/x86/kernel/smpboot.c:topology_update_package_map(), which uses > a counter to set the logical package id and Documentation/x86/topology.txt > states > > """ > - cpuinfo_x86.logical_id: > > The logical ID of the package. As we do not trust BIOSes to enumerate the > packages in a consistent way, we introduced the concept of logical package > ID so we can sanely calculate the number of maximum possible packages in > the system and have the packages enumerated linearly. > """ Eh, x86 does seem to display the physical, rather than logical (linear) IDs in sysfs though, arch/x86/include/asm/topology.h:#define topology_physical_package_id(cpu) (cpu_data(cpu).phys_proc_id) """ - cpuinfo_x86.phys_proc_id: The physical ID of the package. This information is retrieved via CPUID and deduced from the APIC IDs of the cores in the package. """ So, hmmm... But, I think we should either be looking for a hardware derived ID to use (like x86), or remap to counters. I don't believe the current scheme of using ACPI offsets can be better than counters, and it has consistency and human readability issues. Thanks, drew > > Which I see as x86 precedent for the consistency argument I made in my > other reply. > > Thanks, > drew ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] arm64: acpi: reenumerate topology ids 2018-06-29 11:55 ` Andrew Jones @ 2018-06-29 13:48 ` Sudeep Holla 0 siblings, 0 replies; 20+ messages in thread From: Sudeep Holla @ 2018-06-29 13:48 UTC (permalink / raw) To: Andrew Jones Cc: Jeremy Linton, linux-arm-kernel, linux-kernel, ard.biesheuvel, shunyong.yang, yu.zheng, catalin.marinas, will.deacon On Fri, Jun 29, 2018 at 01:55:39PM +0200, Andrew Jones wrote: > On Fri, Jun 29, 2018 at 01:42:27PM +0200, Andrew Jones wrote: > > On Fri, Jun 29, 2018 at 11:53:34AM +0100, Sudeep Holla wrote: > > > On Thu, Jun 28, 2018 at 12:12:00PM -0500, Jeremy Linton wrote: > > > > Hi, > > > > > > > > On 06/28/2018 11:30 AM, Sudeep Holla wrote: > > > > > > [...] > > > > > > > >I am not sure if we can ever guarantee that DT and ACPI will get the > > > > >same ids whatever counter we use as it depends on the order presented in > > > > >the firmware(DT or ACPI). So I am not for generating ids for core and > > > > >threads in that way. > > > > > > > > > >So I would like to keep it simple and just have this counters for > > > > >package ids as demonstrated in Shunyong's patch. > > > > > > > > So, currently on a non threaded system, the core id's look nice because they > > > > are just the ACPI ids. Its the package id's that look strange, we could just > > > > fix the package ids, but on threaded machines the threads have the nice acpi > > > > ids, and the core ids are then funny numbers. So, I suspect that is driving > > > > this as much as the strange package ids. > > > > > > > > > > Yes, I know that and that's what made be look at topology_get_acpi_cpu_tag > > > For me, if the PPTT has valid ID, we should use that. Just becuase DT lacks > > > it and uses counter doesn't mean ACPI also needs to follow that. > > > > AFAIK, a valid ACPI UID doesn't need to be something derivable directly > > from the hardware, so it's just as arbitrary as the CPU phandle that is > > in the DT cpu-map, i.e. DT *does* have an analogous leaf node integer. > > > > > > > > I am sure some vendor will put valid UID and expect that to be in the > > > sysfs. > > > > I can't think of any reason that would be useful, especially when the > > UID is for a thread, which isn't even displayed by sysfs. > > > > > > > > > (and as a side, I actually like the PE has a acpi id behavior, but for > > > > threads its being lost with this patch...) > > > > > > > > Given i've seen odd package/core ids on x86s a few years ago, it never > > > > So this inspired me to grep some x86 topology code. I found > > arch/x86/kernel/smpboot.c:topology_update_package_map(), which uses > > a counter to set the logical package id and Documentation/x86/topology.txt > > states > > > > """ > > - cpuinfo_x86.logical_id: > > > > The logical ID of the package. As we do not trust BIOSes to enumerate the > > packages in a consistent way, we introduced the concept of logical package > > ID so we can sanely calculate the number of maximum possible packages in > > the system and have the packages enumerated linearly. > > """ > > Eh, x86 does seem to display the physical, rather than logical (linear) > IDs in sysfs though, > > arch/x86/include/asm/topology.h:#define topology_physical_package_id(cpu) (cpu_data(cpu).phys_proc_id) > > """ > - cpuinfo_x86.phys_proc_id: > > The physical ID of the package. This information is retrieved via CPUID > and deduced from the APIC IDs of the cores in the package. > """ > > So, hmmm... > > But, I think we should either be looking for a hardware derived ID to use > (like x86), or remap to counters. I don't believe the current scheme of > using ACPI offsets can be better than counters, and it has consistency and > human readability issues. > UID was added for the same reason and we *have* to use it if present. If not, OS can have it's own policy and I am fine with offset. So if offset hurts eyes, better even the absence of UID in the PPTT. As we don't have architectural way to derive it, we *have* to rely on platform providing UID. If it doesn't, why should OS ? I really don't think counter is the solution as this is user ABI, better be consistent rather than human readable especially if platforms don't care to provide one. -- Regards, Sudeep ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] arm64: acpi: reenumerate topology ids 2018-06-29 11:42 ` Andrew Jones 2018-06-29 11:55 ` Andrew Jones @ 2018-06-29 13:38 ` Sudeep Holla 2018-06-29 16:03 ` Andrew Jones 1 sibling, 1 reply; 20+ messages in thread From: Sudeep Holla @ 2018-06-29 13:38 UTC (permalink / raw) To: Andrew Jones Cc: Jeremy Linton, linux-arm-kernel, linux-kernel, ard.biesheuvel, shunyong.yang, yu.zheng, catalin.marinas, will.deacon On Fri, Jun 29, 2018 at 01:42:27PM +0200, Andrew Jones wrote: > On Fri, Jun 29, 2018 at 11:53:34AM +0100, Sudeep Holla wrote: > > On Thu, Jun 28, 2018 at 12:12:00PM -0500, Jeremy Linton wrote: > > > Hi, > > > > > > On 06/28/2018 11:30 AM, Sudeep Holla wrote: > > > > [...] > > > > > >I am not sure if we can ever guarantee that DT and ACPI will get the > > > >same ids whatever counter we use as it depends on the order presented in > > > >the firmware(DT or ACPI). So I am not for generating ids for core and > > > >threads in that way. > > > > > > > >So I would like to keep it simple and just have this counters for > > > >package ids as demonstrated in Shunyong's patch. > > > > > > So, currently on a non threaded system, the core id's look nice because they > > > are just the ACPI ids. Its the package id's that look strange, we could just > > > fix the package ids, but on threaded machines the threads have the nice acpi > > > ids, and the core ids are then funny numbers. So, I suspect that is driving > > > this as much as the strange package ids. > > > > > > > Yes, I know that and that's what made be look at topology_get_acpi_cpu_tag > > For me, if the PPTT has valid ID, we should use that. Just becuase DT lacks > > it and uses counter doesn't mean ACPI also needs to follow that. > > AFAIK, a valid ACPI UID doesn't need to be something derivable directly > from the hardware, so it's just as arbitrary as the CPU phandle that is > in the DT cpu-map, i.e. DT *does* have an analogous leaf node integer. > Its platform specific, left to vendors to identify that. Are you referring to reg property in DT for leaf nodes ? If so, that's MPIDR and is present in ACPI MADT. > > > > I am sure some vendor will put valid UID and expect that to be in the > > sysfs. > > I can't think of any reason that would be useful, especially when the > UID is for a thread, which isn't even displayed by sysfs. > You are mixing MPIDR and UID here. > > > > > (and as a side, I actually like the PE has a acpi id behavior, but for > > > threads its being lost with this patch...) > > > > > > Given i've seen odd package/core ids on x86s a few years ago, it never > > So this inspired me to grep some x86 topology code. I found > arch/x86/kernel/smpboot.c:topology_update_package_map(), which uses > a counter to set the logical package id and Documentation/x86/topology.txt > states > > """ > - cpuinfo_x86.logical_id: > > The logical ID of the package. As we do not trust BIOSes to enumerate the > packages in a consistent way, we introduced the concept of logical package > ID so we can sanely calculate the number of maximum possible packages in > the system and have the packages enumerated linearly. > """ > > Which I see as x86 precedent for the consistency argument I made in my > other reply. > There are lots of things arm64 doesn't replicate x86. So I don't take that as valid argument. It's a new feature, we can use APCI PPTT UID feature to be consistent. I don't see counter being in anyway consistent. Even PPTT could be generated in the firmware and the order is not consistent. In which case the counter values in Linux might get things wrong while using UID from the firmware can keep it consistent(e.g. sockets removed,..etc) -- Regards, Sudeep ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] arm64: acpi: reenumerate topology ids 2018-06-29 13:38 ` Sudeep Holla @ 2018-06-29 16:03 ` Andrew Jones 0 siblings, 0 replies; 20+ messages in thread From: Andrew Jones @ 2018-06-29 16:03 UTC (permalink / raw) To: Sudeep Holla Cc: Jeremy Linton, linux-arm-kernel, linux-kernel, ard.biesheuvel, shunyong.yang, yu.zheng, catalin.marinas, will.deacon On Fri, Jun 29, 2018 at 02:38:49PM +0100, Sudeep Holla wrote: > On Fri, Jun 29, 2018 at 01:42:27PM +0200, Andrew Jones wrote: > > On Fri, Jun 29, 2018 at 11:53:34AM +0100, Sudeep Holla wrote: > > > On Thu, Jun 28, 2018 at 12:12:00PM -0500, Jeremy Linton wrote: > > > > Hi, > > > > > > > > On 06/28/2018 11:30 AM, Sudeep Holla wrote: > > > > > > [...] > > > > > > > >I am not sure if we can ever guarantee that DT and ACPI will get the > > > > >same ids whatever counter we use as it depends on the order presented in > > > > >the firmware(DT or ACPI). So I am not for generating ids for core and > > > > >threads in that way. > > > > > > > > > >So I would like to keep it simple and just have this counters for > > > > >package ids as demonstrated in Shunyong's patch. > > > > > > > > So, currently on a non threaded system, the core id's look nice because they > > > > are just the ACPI ids. Its the package id's that look strange, we could just > > > > fix the package ids, but on threaded machines the threads have the nice acpi > > > > ids, and the core ids are then funny numbers. So, I suspect that is driving > > > > this as much as the strange package ids. > > > > > > > > > > Yes, I know that and that's what made be look at topology_get_acpi_cpu_tag > > > For me, if the PPTT has valid ID, we should use that. Just becuase DT lacks > > > it and uses counter doesn't mean ACPI also needs to follow that. > > > > AFAIK, a valid ACPI UID doesn't need to be something derivable directly > > from the hardware, so it's just as arbitrary as the CPU phandle that is > > in the DT cpu-map, i.e. DT *does* have an analogous leaf node integer. > > > > Its platform specific, left to vendors to identify that. Are you referring > to reg property in DT for leaf nodes ? If so, that's MPIDR and is present > in ACPI MADT. No, I'm referring to the 'cpu' node in the cpu-map, which is a phandle pointing to a cpu node. The cpu node contains the reg with the MPIDR. A phandle links one DT node to another, thus it's analogous to an ACPI ID linkage. However, after reading your last mail that indicates these "ACPI processor IDs" may actually be derived from hardware - I guess by implementing the _UID method and having that method extract it in some vendor-specific way, then the analogy breaks down. A phandle is just a DT construct, while an ID from an ACPI method may actually be a useful hardware locator address. > > > > > > > I am sure some vendor will put valid UID and expect that to be in the > > > sysfs. > > > > I can't think of any reason that would be useful, especially when the > > UID is for a thread, which isn't even displayed by sysfs. > > > > You are mixing MPIDR and UID here. I wasn't talking about MPIDR at all. As I said above, I was was under the impression the UID was not derived from hardware, i.e. only an ACPI construct. If that's not the case, then it has more value than what I was giving it before, but only when the valid flag is set. And, since we have a [good?] chance that the valid flag won't be set, then I still think we're better off with counters to keep the user API consistent and unambiguous across systems. We should also keep a mapping to these UIDs, though, when we have something valid to map to. Thanks, drew ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] arm64: acpi: reenumerate topology ids 2018-06-28 16:30 ` Sudeep Holla 2018-06-28 17:12 ` Jeremy Linton @ 2018-06-28 17:32 ` Andrew Jones 2018-06-29 10:29 ` Sudeep Holla 1 sibling, 1 reply; 20+ messages in thread From: Andrew Jones @ 2018-06-28 17:32 UTC (permalink / raw) To: Sudeep Holla Cc: linux-arm-kernel, linux-kernel, jeremy.linton, ard.biesheuvel, shunyong.yang, yu.zheng, catalin.marinas, will.deacon On Thu, Jun 28, 2018 at 05:30:51PM +0100, Sudeep Holla wrote: > I am not sure if we can ever guarantee that DT and ACPI will get the > same ids whatever counter we use as it depends on the order presented in > the firmware(DT or ACPI). So I am not for generating ids for core and > threads in that way. I don't believe we have to guarantee that the exact (package,core,thread) triplet describing a PE with DT matches ACPI. We just need to guarantee that each triplet we select properly puts a PE in the same group as its peers. So, as long as we keep the grouping described by DT or ACPI, then the (package,core,thread) IDs assigned are pretty arbitrary. I could change the commit message to state we can generate IDs *like* DT does (i.e. with counters), even if they may not result in identical triplet to PE mappings. > > So I would like to keep it simple and just have this counters for > package ids as demonstrated in Shunyong's patch. > If we don't also handle cores when there are threads, then the cores will also end up having weird IDs. Thanks, drew ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] arm64: acpi: reenumerate topology ids 2018-06-28 17:32 ` Andrew Jones @ 2018-06-29 10:29 ` Sudeep Holla 2018-06-29 11:23 ` Andrew Jones 0 siblings, 1 reply; 20+ messages in thread From: Sudeep Holla @ 2018-06-29 10:29 UTC (permalink / raw) To: Andrew Jones Cc: linux-arm-kernel, linux-kernel, jeremy.linton, ard.biesheuvel, shunyong.yang, yu.zheng, catalin.marinas, will.deacon On Thu, Jun 28, 2018 at 07:32:43PM +0200, Andrew Jones wrote: > On Thu, Jun 28, 2018 at 05:30:51PM +0100, Sudeep Holla wrote: > > I am not sure if we can ever guarantee that DT and ACPI will get the > > same ids whatever counter we use as it depends on the order presented in > > the firmware(DT or ACPI). So I am not for generating ids for core and > > threads in that way. > > I don't believe we have to guarantee that the exact (package,core,thread) > triplet describing a PE with DT matches ACPI. We just need to guarantee > that each triplet we select properly puts a PE in the same group as its > peers. So, as long as we keep the grouping described by DT or ACPI, then > the (package,core,thread) IDs assigned are pretty arbitrary. > If that's the requirement, we already do that. The IDs are just too arbitrary :) > I could change the commit message to state we can generate IDs *like* > DT does (i.e. with counters), even if they may not result in identical > triplet to PE mappings. > Why we need to make it *like DT* ? > > > > So I would like to keep it simple and just have this counters for > > package ids as demonstrated in Shunyong's patch. > > > > If we don't also handle cores when there are threads, then the cores > will also end up having weird IDs. > Yes, but if PPTT says it has valid ID, I would prefer that over DT like generated. -- Regards, Sudeep ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] arm64: acpi: reenumerate topology ids 2018-06-29 10:29 ` Sudeep Holla @ 2018-06-29 11:23 ` Andrew Jones 2018-06-29 13:29 ` Sudeep Holla 0 siblings, 1 reply; 20+ messages in thread From: Andrew Jones @ 2018-06-29 11:23 UTC (permalink / raw) To: Sudeep Holla Cc: linux-arm-kernel, linux-kernel, jeremy.linton, ard.biesheuvel, shunyong.yang, yu.zheng, catalin.marinas, will.deacon On Fri, Jun 29, 2018 at 11:29:27AM +0100, Sudeep Holla wrote: > On Thu, Jun 28, 2018 at 07:32:43PM +0200, Andrew Jones wrote: > > On Thu, Jun 28, 2018 at 05:30:51PM +0100, Sudeep Holla wrote: > > > I am not sure if we can ever guarantee that DT and ACPI will get the > > > same ids whatever counter we use as it depends on the order presented in > > > the firmware(DT or ACPI). So I am not for generating ids for core and > > > threads in that way. > > > > I don't believe we have to guarantee that the exact (package,core,thread) > > triplet describing a PE with DT matches ACPI. We just need to guarantee > > that each triplet we select properly puts a PE in the same group as its > > peers. So, as long as we keep the grouping described by DT or ACPI, then > > the (package,core,thread) IDs assigned are pretty arbitrary. > > > > If that's the requirement, we already do that. The IDs are just too > arbitrary :) Right. The patch doesn't fix anything, it just makes the IDs less weird looking to humans, and brings some consistency to IDs across systems and hardware descriptions. > > > I could change the commit message to state we can generate IDs *like* > > DT does (i.e. with counters), even if they may not result in identical > > triplet to PE mappings. > > > > Why we need to make it *like DT* ? Because the DT parser considers human readers, which, IMHO, is nicer. The consistency remapping to counters brings shouldn't be overlooked. If both ACPI and DT present the CPUs in the same order (i.e. cpu0 is mpidr0, cpu1 is mpidr1, ...) using their respective descriptions, then this patch will ensure topology IDs are the same. Also, when going from machine to machine, it's nice to expect package/core/thread-id to be within the same range and to vary by a set difference (1), rather than have IDs that increment by 20 on one system and by 160 on another, depending on how the ACPI table nodes are laid out. > > > > > > > So I would like to keep it simple and just have this counters for > > > package ids as demonstrated in Shunyong's patch. > > > > > > > If we don't also handle cores when there are threads, then the cores > > will also end up having weird IDs. > > > > Yes, but if PPTT says it has valid ID, I would prefer that over DT like > generated. Valid *ACPI* ID, which just means it's a guaranteed unique ACPI UID, which isn't likely going to be anything useful to a user. Thanks, drew ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] arm64: acpi: reenumerate topology ids 2018-06-29 11:23 ` Andrew Jones @ 2018-06-29 13:29 ` Sudeep Holla 2018-06-29 15:46 ` Andrew Jones 0 siblings, 1 reply; 20+ messages in thread From: Sudeep Holla @ 2018-06-29 13:29 UTC (permalink / raw) To: Andrew Jones Cc: linux-arm-kernel, linux-kernel, jeremy.linton, ard.biesheuvel, shunyong.yang, yu.zheng, catalin.marinas, will.deacon On Fri, Jun 29, 2018 at 01:23:54PM +0200, Andrew Jones wrote: > On Fri, Jun 29, 2018 at 11:29:27AM +0100, Sudeep Holla wrote: > > On Thu, Jun 28, 2018 at 07:32:43PM +0200, Andrew Jones wrote: > > > On Thu, Jun 28, 2018 at 05:30:51PM +0100, Sudeep Holla wrote: > > > > I am not sure if we can ever guarantee that DT and ACPI will get the > > > > same ids whatever counter we use as it depends on the order presented in > > > > the firmware(DT or ACPI). So I am not for generating ids for core and > > > > threads in that way. > > > > > > I don't believe we have to guarantee that the exact (package,core,thread) > > > triplet describing a PE with DT matches ACPI. We just need to guarantee > > > that each triplet we select properly puts a PE in the same group as its > > > peers. So, as long as we keep the grouping described by DT or ACPI, then > > > the (package,core,thread) IDs assigned are pretty arbitrary. > > > > > > > If that's the requirement, we already do that. The IDs are just too > > arbitrary :) > > Right. The patch doesn't fix anything, it just makes the IDs less weird > looking to humans, and brings some consistency to IDs across systems > and hardware descriptions. > I agree, but don't think counter like this will be harmless. It can create some other inconsistency in some other way. > > > > > I could change the commit message to state we can generate IDs *like* > > > DT does (i.e. with counters), even if they may not result in identical > > > triplet to PE mappings. > > > > > > > Why we need to make it *like DT* ? > > Because the DT parser considers human readers, which, IMHO, is nicer. > > The consistency remapping to counters brings shouldn't be overlooked. If > both ACPI and DT present the CPUs in the same order (i.e. cpu0 is mpidr0, > cpu1 is mpidr1, ...) using their respective descriptions, then this patch > will ensure topology IDs are the same. Yes, it's all fine in theory. But firmware is the one presenting it and ACPI PPTT has UID for a reason and I want that to be the single source for these IDs. > Also, when going from machine to > machine, it's nice to expect package/core/thread-id to be within > the same range and to vary by a set difference (1), rather than have > IDs that increment by 20 on one system and by 160 on another, > depending on how the ACPI table nodes are laid out. > If it matters a lot, vendors must use UID for consistency. Since OS doesn't use those IDs for any particular reason, OS must not care. > > > > > > > > > > So I would like to keep it simple and just have this counters for > > > > package ids as demonstrated in Shunyong's patch. > > > > > > > > > > If we don't also handle cores when there are threads, then the cores > > > will also end up having weird IDs. > > > > > > > Yes, but if PPTT says it has valid ID, I would prefer that over DT like > > generated. > > Valid *ACPI* ID, which just means it's a guaranteed unique ACPI UID, > which isn't likely going to be anything useful to a user. > How is that different from OS generated one from user's perspective ? Vendors might assign sockets UID and he may help them to replace one. Having some generated counter based id is not helpful. So if someone needs to have valid IDs there, better to pass it via PPTT. OS will otherwise put any value(current offset is good choice) -- Regards, Sudeep ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] arm64: acpi: reenumerate topology ids 2018-06-29 13:29 ` Sudeep Holla @ 2018-06-29 15:46 ` Andrew Jones 2018-06-29 15:55 ` Sudeep Holla ` (2 more replies) 0 siblings, 3 replies; 20+ messages in thread From: Andrew Jones @ 2018-06-29 15:46 UTC (permalink / raw) To: Sudeep Holla Cc: linux-arm-kernel, linux-kernel, jeremy.linton, ard.biesheuvel, shunyong.yang, yu.zheng, catalin.marinas, will.deacon On Fri, Jun 29, 2018 at 02:29:34PM +0100, Sudeep Holla wrote: > If it matters a lot, vendors must use UID for consistency. Since OS doesn't > use those IDs for any particular reason, OS must not care. That depends. If you look at how topology_logical_package_id() is used in x86 code you'll see it gets used as an index to an array in a couple places. If we don't remap arbitrary IDs to counters than we may miss out on some opportunities to avoid lists. Also, we're talking about what's visible to users. I think it's much more likely to break a user app by exposing topology IDs that have values greater than the linear CPU numbers (even though properly written apps shouldn't expect them to be strictly <=), than the opposite. > > > > > > > > > > > > > > So I would like to keep it simple and just have this counters for > > > > > package ids as demonstrated in Shunyong's patch. > > > > > > > > > > > > > If we don't also handle cores when there are threads, then the cores > > > > will also end up having weird IDs. > > > > > > > > > > Yes, but if PPTT says it has valid ID, I would prefer that over DT like > > > generated. > > > > Valid *ACPI* ID, which just means it's a guaranteed unique ACPI UID, > > which isn't likely going to be anything useful to a user. > > > > How is that different from OS generated one from user's perspective ? > Vendors might assign sockets UID and he may help them to replace one. > Having some generated counter based id is not helpful. I agree with this. It's a good argument for maintaining a mapping of package-id to id-physically-printed-on-a-package somewhere. To avoid maintaining a mapping it could just be stored directly in cpu_topology[cpu].package_id, but then how can we tell the difference between a valid printed-on-package-id and an ACPI offset? We'd still have to maintain additional state to determine if it's valid or not, so we could just maintain a mapping instead. Thanks, drew ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] arm64: acpi: reenumerate topology ids 2018-06-29 15:46 ` Andrew Jones @ 2018-06-29 15:55 ` Sudeep Holla 2018-06-29 16:48 ` Jeremy Linton 2018-07-02 14:58 ` Jeffrey Hugo 2 siblings, 0 replies; 20+ messages in thread From: Sudeep Holla @ 2018-06-29 15:55 UTC (permalink / raw) To: Andrew Jones Cc: linux-arm-kernel, linux-kernel, jeremy.linton, ard.biesheuvel, shunyong.yang, yu.zheng, catalin.marinas, will.deacon, Sudeep Holla On Fri, Jun 29, 2018 at 05:46:08PM +0200, Andrew Jones wrote: > On Fri, Jun 29, 2018 at 02:29:34PM +0100, Sudeep Holla wrote: [..] > > > > How is that different from OS generated one from user's perspective ? > > Vendors might assign sockets UID and he may help them to replace one. > > Having some generated counter based id is not helpful. > > I agree with this. It's a good argument for maintaining a mapping of > package-id to id-physically-printed-on-a-package somewhere. To avoid > maintaining a mapping it could just be stored directly in > cpu_topology[cpu].package_id, but then how can we tell the difference > between a valid printed-on-package-id and an ACPI offset? We'd still > have to maintain additional state to determine if it's valid or not, > so we could just maintain a mapping instead. > x86 may have a architectural way to obtain it and hence they don't need to rely on PPTT. But for ARM, we need to rely on PPTT for it and if vendors/users need accurate information, it has to come from PPTT and any other place is never going to be consistent and hence unusable. So, even though specification doesn't mandate, I think OS should as it's the only robust way. We can get the firmware fixed/updated if random unique number hurts. Firmware is not upgradeable is no longer a valid argument. -- Regards, Sudeep ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] arm64: acpi: reenumerate topology ids 2018-06-29 15:46 ` Andrew Jones 2018-06-29 15:55 ` Sudeep Holla @ 2018-06-29 16:48 ` Jeremy Linton 2018-06-29 17:03 ` Andrew Jones 2018-07-02 14:58 ` Jeffrey Hugo 2 siblings, 1 reply; 20+ messages in thread From: Jeremy Linton @ 2018-06-29 16:48 UTC (permalink / raw) To: Andrew Jones, Sudeep Holla Cc: linux-arm-kernel, linux-kernel, ard.biesheuvel, shunyong.yang, yu.zheng, catalin.marinas, will.deacon Hi, On 06/29/2018 10:46 AM, Andrew Jones wrote: > On Fri, Jun 29, 2018 at 02:29:34PM +0100, Sudeep Holla wrote: >> If it matters a lot, vendors must use UID for consistency. Since OS doesn't >> use those IDs for any particular reason, OS must not care. > > That depends. If you look at how topology_logical_package_id() is used in > x86 code you'll see it gets used as an index to an array in a couple > places. If we don't remap arbitrary IDs to counters than we may miss out > on some opportunities to avoid lists. > > Also, we're talking about what's visible to users. I think it's much more > likely to break a user app by exposing topology IDs that have values > greater than the linear CPU numbers (even though properly written apps > shouldn't expect them to be strictly <=), than the opposite. > >> >>>> >>>>>> >>>>>> So I would like to keep it simple and just have this counters for >>>>>> package ids as demonstrated in Shunyong's patch. >>>>>> >>>>> >>>>> If we don't also handle cores when there are threads, then the cores >>>>> will also end up having weird IDs. >>>>> >>>> >>>> Yes, but if PPTT says it has valid ID, I would prefer that over DT like >>>> generated. >>> >>> Valid *ACPI* ID, which just means it's a guaranteed unique ACPI UID, >>> which isn't likely going to be anything useful to a user. >>> >> >> How is that different from OS generated one from user's perspective ? >> Vendors might assign sockets UID and he may help them to replace one. >> Having some generated counter based id is not helpful. > > I agree with this. It's a good argument for maintaining a mapping of > package-id to id-physically-printed-on-a-package somewhere. To avoid > maintaining a mapping it could just be stored directly in > cpu_topology[cpu].package_id, but then how can we tell the difference > between a valid printed-on-package-id and an ACPI offset? We'd still > have to maintain additional state to determine if it's valid or not, > so we could just maintain a mapping instead. Just to be clear, there isn't anything (AFAIK) in the ACPI specification which dictates what values should comprise the various ACPI id's. They are assumed only to be machine readable, which is why it seems some implementations are just using a sanitized version of mpidr for the core/MADT acpi id. That is why simply using the id flagged as valid in a PPTT node doesn't necessarily give you a more human readable value. If you want a human readable socket identifier that matches something stamped above the socket, that is what SMBIOS is for. Queue discussion about that tables reliability for functional ids. Either way, as the spec is written today (or any ECRs I've seen), your definitely not going to get both nice socket1, socket2, and cpu1, cpu2 out of the same PPTT/ACPIid name-space since the numerical id's conflict. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] arm64: acpi: reenumerate topology ids 2018-06-29 16:48 ` Jeremy Linton @ 2018-06-29 17:03 ` Andrew Jones 2018-06-29 17:23 ` Sudeep Holla 0 siblings, 1 reply; 20+ messages in thread From: Andrew Jones @ 2018-06-29 17:03 UTC (permalink / raw) To: Jeremy Linton Cc: Sudeep Holla, linux-arm-kernel, linux-kernel, ard.biesheuvel, shunyong.yang, yu.zheng, catalin.marinas, will.deacon On Fri, Jun 29, 2018 at 11:48:15AM -0500, Jeremy Linton wrote: > Just to be clear, there isn't anything (AFAIK) in the ACPI specification > which dictates what values should comprise the various ACPI id's. They are > assumed only to be machine readable, which is why it seems some > implementations are just using a sanitized version of mpidr for the > core/MADT acpi id. That is why simply using the id flagged as valid in a > PPTT node doesn't necessarily give you a more human readable value. I didn't expect them to be sequential integers, but I was starting to see value in them anyway if they were expected to be some something stamped on the socket. > > If you want a human readable socket identifier that matches something > stamped above the socket, that is what SMBIOS is for. Queue discussion about > that tables reliability for functional ids. Either way, as the spec is > written today (or any ECRs I've seen), your definitely not going to get both > nice socket1, socket2, and cpu1, cpu2 out of the same PPTT/ACPIid name-space > since the numerical id's conflict. > If we don't expect the ACPI processor ID to be something useful to users, then I'll revert back to lobbying for counters, as those arbitrary numbers can't be less useful than arbitrary offsets and ACPI IDs, and, IMO, are more likely to make users/user apps happy. Thanks, drew ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] arm64: acpi: reenumerate topology ids 2018-06-29 17:03 ` Andrew Jones @ 2018-06-29 17:23 ` Sudeep Holla 2018-06-29 18:03 ` Andrew Jones 0 siblings, 1 reply; 20+ messages in thread From: Sudeep Holla @ 2018-06-29 17:23 UTC (permalink / raw) To: Andrew Jones Cc: Jeremy Linton, linux-arm-kernel, linux-kernel, ard.biesheuvel, shunyong.yang, yu.zheng, catalin.marinas, will.deacon, Sudeep Holla On Fri, Jun 29, 2018 at 07:03:34PM +0200, Andrew Jones wrote: > On Fri, Jun 29, 2018 at 11:48:15AM -0500, Jeremy Linton wrote: [..] > > > > If you want a human readable socket identifier that matches something > > stamped above the socket, that is what SMBIOS is for. Queue discussion about > > that tables reliability for functional ids. Either way, as the spec is > > written today (or any ECRs I've seen), your definitely not going to get both > > nice socket1, socket2, and cpu1, cpu2 out of the same PPTT/ACPIid name-space > > since the numerical id's conflict. > > > > If we don't expect the ACPI processor ID to be something useful to users, > then I'll revert back to lobbying for counters, as those arbitrary numbers > can't be less useful than arbitrary offsets and ACPI IDs, and, IMO, are > more likely to make users/user apps happy. > I agree that ACPI processor ID may not be useful to the users, but providing some counter based ID which is highly dependent on the ordering the firmware table which can change between boots is highly inconsistent and unreliable and in some sense break user ABI. So I still NACK the counter based ID. -- Regards, Sudeep ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] arm64: acpi: reenumerate topology ids 2018-06-29 17:23 ` Sudeep Holla @ 2018-06-29 18:03 ` Andrew Jones 0 siblings, 0 replies; 20+ messages in thread From: Andrew Jones @ 2018-06-29 18:03 UTC (permalink / raw) To: Sudeep Holla Cc: Jeremy Linton, linux-arm-kernel, linux-kernel, ard.biesheuvel, shunyong.yang, yu.zheng, catalin.marinas, will.deacon On Fri, Jun 29, 2018 at 06:23:15PM +0100, Sudeep Holla wrote: > On Fri, Jun 29, 2018 at 07:03:34PM +0200, Andrew Jones wrote: > > On Fri, Jun 29, 2018 at 11:48:15AM -0500, Jeremy Linton wrote: > [..] > > > > > > > If you want a human readable socket identifier that matches something > > > stamped above the socket, that is what SMBIOS is for. Queue discussion about > > > that tables reliability for functional ids. Either way, as the spec is > > > written today (or any ECRs I've seen), your definitely not going to get both > > > nice socket1, socket2, and cpu1, cpu2 out of the same PPTT/ACPIid name-space > > > since the numerical id's conflict. > > > > > > > If we don't expect the ACPI processor ID to be something useful to users, > > then I'll revert back to lobbying for counters, as those arbitrary numbers > > can't be less useful than arbitrary offsets and ACPI IDs, and, IMO, are > > more likely to make users/user apps happy. > > > > I agree that ACPI processor ID may not be useful to the users, but providing > some counter based ID which is highly dependent on the ordering the firmware > table which can change between boots is highly inconsistent and unreliable > and in some sense break user ABI. So I still NACK the counter based ID. > If the firmware tables change order between boots, then so will ACPI table offsets - unless you mean only the leaf nodes may change order. Leaf nodes would indeed keep the same ID (by using ACPI processor IDs), but when the leaves are threads, those IDs aren't visible to the user anyway, as thread-id is not in sysfs. Anyway, what would be the harm if PE_x (PE not a thread) was identified by the pair (1,1) on one boot and (1,2) on the second boot? It's still clearly in the same package, and I don't believe there's any need nor desire to allow a user to tell the difference between two peer PEs. Users should never know that the core-id of a specific physical core changed, nor should they care. I'm racking my brain trying to think of scenario, maybe something with elaborate cpu pinning, that could be affected, but can't see how. Please provide an example. If you can, then we obviously need to modify how the DT cpu-map is parsed, as DT boots would be vulnerable to the same issue. Thanks, drew ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] arm64: acpi: reenumerate topology ids 2018-06-29 15:46 ` Andrew Jones 2018-06-29 15:55 ` Sudeep Holla 2018-06-29 16:48 ` Jeremy Linton @ 2018-07-02 14:58 ` Jeffrey Hugo 2 siblings, 0 replies; 20+ messages in thread From: Jeffrey Hugo @ 2018-07-02 14:58 UTC (permalink / raw) To: Andrew Jones, Sudeep Holla Cc: shunyong.yang, ard.biesheuvel, catalin.marinas, will.deacon, linux-kernel, jeremy.linton, yu.zheng, linux-arm-kernel On 6/29/2018 9:46 AM, Andrew Jones wrote: > On Fri, Jun 29, 2018 at 02:29:34PM +0100, Sudeep Holla wrote: >> If it matters a lot, vendors must use UID for consistency. Since OS doesn't >> use those IDs for any particular reason, OS must not care. > > That depends. If you look at how topology_logical_package_id() is used in > x86 code you'll see it gets used as an index to an array in a couple > places. If we don't remap arbitrary IDs to counters than we may miss out > on some opportunities to avoid lists. > > Also, we're talking about what's visible to users. I think it's much more > likely to break a user app by exposing topology IDs that have values > greater than the linear CPU numbers (even though properly written apps > shouldn't expect them to be strictly <=), than the opposite. Libvirt has the assumption already that the sysfs numbers correspond to linear CPU numbers, and has an arbitrary limit of 4k. When spinning up a VM, if libvirt sees a CPU ID > 4k, it fails to init the VM since it assumes the host has more than 4k CPUs, which is unsupported. We found this when we were making our UIDs to be the same as MPIDR in MADT. We changed our UIDs to be sequential 0-N numbering to workaround the issue. -- Jeffrey Hugo Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2018-07-02 14:58 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-28 14:51 [PATCH] arm64: acpi: reenumerate topology ids Andrew Jones 2018-06-28 16:30 ` Sudeep Holla 2018-06-28 17:12 ` Jeremy Linton 2018-06-29 10:53 ` Sudeep Holla 2018-06-29 11:42 ` Andrew Jones 2018-06-29 11:55 ` Andrew Jones 2018-06-29 13:48 ` Sudeep Holla 2018-06-29 13:38 ` Sudeep Holla 2018-06-29 16:03 ` Andrew Jones 2018-06-28 17:32 ` Andrew Jones 2018-06-29 10:29 ` Sudeep Holla 2018-06-29 11:23 ` Andrew Jones 2018-06-29 13:29 ` Sudeep Holla 2018-06-29 15:46 ` Andrew Jones 2018-06-29 15:55 ` Sudeep Holla 2018-06-29 16:48 ` Jeremy Linton 2018-06-29 17:03 ` Andrew Jones 2018-06-29 17:23 ` Sudeep Holla 2018-06-29 18:03 ` Andrew Jones 2018-07-02 14:58 ` Jeffrey Hugo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).