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 <[email protected]>
Cc: Sudeep Holla <[email protected]>
Signed-off-by: Andrew Jones <[email protected]>
---
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
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 <[email protected]>
> Cc: Sudeep Holla <[email protected]>
> Signed-off-by: Andrew Jones <[email protected]>
> ---
>
> 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);
}
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 <[email protected]>
>> Cc: Sudeep Holla <[email protected]>
>> Signed-off-by: Andrew Jones <[email protected]>
>> ---
>>
>> 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...
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
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
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
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
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
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
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
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
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
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
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
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.
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
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
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
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
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.