linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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 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-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: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 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: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 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 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 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 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-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).