linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH] arm64: topology: Map PPTT node offset to logic physical package id
@ 2018-06-28  9:18 Shunyong Yang
  2018-06-28  9:38 ` Sudeep Holla
  2018-07-12 11:06 ` Will Deacon
  0 siblings, 2 replies; 11+ messages in thread
From: Shunyong Yang @ 2018-06-28  9:18 UTC (permalink / raw)
  To: catalin.marinas
  Cc: will.deacon, jeremy.linton, linux-arm-kernel, linux-kernel,
	Shunyong Yang, Joey Zheng

As PPTT spec doesn't define the physical package id,
find_acpi_cpu_topology_package() will return offset of the node with
Physical package field set when querying physical package id. So, it
returns 162(0xA2) in following example.

[0A2h 0162   1]                Subtable Type : 00 [Processor Hierarchy
Node]
[0A3h 0163   1]                       Length : 1C
[0A4h 0164   2]                     Reserved : 0000
[0A6h 0166   4]        Flags (decoded below) : 00000003
                            Physical package : 1
                     ACPI Processor ID valid : 1
[0AAh 0170   4]                       Parent : 00000000
[0AEh 0174   4]            ACPI Processor ID : 00001000
[0B2h 0178   4]      Private Resource Number : 00000002
[0B6h 0182   4]             Private Resource : 0000006C
[0BAh 0186   4]             Private Resource : 00000084

So, when "cat physical_package" in /sys/devices/system/cpu/cpu0/topology/,
it will output 162(0xA2). And if some items are added before the node
above, the output will change to other value.

This patch maps the node offset to a logic package id. It maps the first
node offset to 0, the second to 1, and so on.

Then, it will not output a big value, such as 162 above. And it will
not change when some nodes(Physical package not set) are added.

And as long as the nodes with Physical package field set in PPTT keeps
the real hardware order, the logic id can map to hardware package id to
some extent.

Hope to get feedback from you.

Cc: Joey Zheng <yu.zheng@hxt-semitech.com>
Signed-off-by: Shunyong Yang <shunyong.yang@hxt-semitech.com>
---
 arch/arm64/kernel/topology.c | 53 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 52 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index f845a8617812..c219224b36e8 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -320,11 +320,59 @@ static void __init reset_cpu_topology(void)
  * Propagate the topology information of the processor_topology_node tree to the
  * cpu_topology array.
  */
+
+struct package_id_map {
+	int topology_id;
+	int package_id;
+	struct list_head list;
+};
+
+static int map_package_id(int topology_id, int *max_package_id,
+			  struct list_head *head)
+{
+	struct list_head *pos;
+	struct package_id_map *entry;
+
+	list_for_each(pos, head) {
+		entry = container_of(pos, struct package_id_map, list);
+		if (entry->topology_id == topology_id)
+			goto done;
+	}
+
+	/* topology_id not found in the list */
+	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+	if (!entry)
+		return topology_id;
+
+	entry->topology_id = topology_id;
+	entry->package_id = *max_package_id;
+	list_add(&entry->list, head);
+	*max_package_id = *max_package_id + 1;
+
+done:
+	return entry->package_id;
+}
+
+static void destroy_map(struct list_head *head)
+{
+	struct package_id_map *entry;
+	struct list_head *pos, *q;
+
+	list_for_each_safe(pos, q, head) {
+		entry = container_of(pos, struct package_id_map, list);
+		list_del(pos);
+		kfree(entry);
+	}
+}
+
 static int __init parse_acpi_topology(void)
 {
 	bool is_threaded;
 	int cpu, topology_id;
+	struct list_head package_id_list;
+	int max_package_id = 0;
 
+	INIT_LIST_HEAD(&package_id_list);
 	is_threaded = read_cpuid_mpidr() & MPIDR_MT_BITMASK;
 
 	for_each_possible_cpu(cpu) {
@@ -343,7 +391,9 @@ static int __init parse_acpi_topology(void)
 			cpu_topology[cpu].core_id    = topology_id;
 		}
 		topology_id = find_acpi_cpu_topology_package(cpu);
-		cpu_topology[cpu].package_id = topology_id;
+		cpu_topology[cpu].package_id = map_package_id(topology_id,
+							&max_package_id,
+							&package_id_list);
 
 		i = acpi_find_last_cache_level(cpu);
 
@@ -358,6 +408,7 @@ static int __init parse_acpi_topology(void)
 		}
 	}
 
+	destroy_map(&package_id_list);
 	return 0;
 }
 
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [RFC PATCH] arm64: topology: Map PPTT node offset to logic physical package id
  2018-06-28  9:18 [RFC PATCH] arm64: topology: Map PPTT node offset to logic physical package id Shunyong Yang
@ 2018-06-28  9:38 ` Sudeep Holla
  2018-06-28 11:57   ` Andrew Jones
  2018-07-12 11:06 ` Will Deacon
  1 sibling, 1 reply; 11+ messages in thread
From: Sudeep Holla @ 2018-06-28  9:38 UTC (permalink / raw)
  To: Shunyong Yang, catalin.marinas
  Cc: Sudeep Holla, will.deacon, linux-kernel, jeremy.linton,
	Joey Zheng, Andrew Jones

Hi Shunyong,

On 28/06/18 10:18, Shunyong Yang wrote:
> As PPTT spec doesn't define the physical package id,
> find_acpi_cpu_topology_package() will return offset of the node with
> Physical package field set when querying physical package id. So, it
> returns 162(0xA2) in following example.
> 
> [0A2h 0162   1]                Subtable Type : 00 [Processor Hierarchy
> Node]
> [0A3h 0163   1]                       Length : 1C
> [0A4h 0164   2]                     Reserved : 0000
> [0A6h 0166   4]        Flags (decoded below) : 00000003
>                             Physical package : 1
>                      ACPI Processor ID valid : 1
> [0AAh 0170   4]                       Parent : 00000000
> [0AEh 0174   4]            ACPI Processor ID : 00001000
> [0B2h 0178   4]      Private Resource Number : 00000002
> [0B6h 0182   4]             Private Resource : 0000006C
> [0BAh 0186   4]             Private Resource : 00000084
> 
> So, when "cat physical_package" in /sys/devices/system/cpu/cpu0/topology/,
> it will output 162(0xA2). And if some items are added before the node
> above, the output will change to other value.
> 
> This patch maps the node offset to a logic package id. It maps the first
> node offset to 0, the second to 1, and so on.
> 
> Then, it will not output a big value, such as 162 above. And it will
> not change when some nodes(Physical package not set) are added.
> 
> And as long as the nodes with Physical package field set in PPTT keeps
> the real hardware order, the logic id can map to hardware package id to
> some extent.
> 
> Hope to get feedback from you.

Thanks for the patch, but Andrew Jones has also posted a patch[1] which
I had a look but was not sure what is the best approach to fix it yet.
I will think about it and respond to that.

--
Regards,
Sudeep

[1] https://patchwork.kernel.org/patch/10482261

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC PATCH] arm64: topology: Map PPTT node offset to logic physical package id
  2018-06-28  9:38 ` Sudeep Holla
@ 2018-06-28 11:57   ` Andrew Jones
  2018-06-28 12:12     ` Sudeep Holla
  0 siblings, 1 reply; 11+ messages in thread
From: Andrew Jones @ 2018-06-28 11:57 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: Shunyong Yang, catalin.marinas, will.deacon, linux-kernel,
	jeremy.linton, Joey Zheng

On Thu, Jun 28, 2018 at 10:38:24AM +0100, Sudeep Holla wrote:
> Hi Shunyong,
> 
> On 28/06/18 10:18, Shunyong Yang wrote:
> > As PPTT spec doesn't define the physical package id,
> > find_acpi_cpu_topology_package() will return offset of the node with
> > Physical package field set when querying physical package id. So, it
> > returns 162(0xA2) in following example.
> > 
> > [0A2h 0162   1]                Subtable Type : 00 [Processor Hierarchy
> > Node]
> > [0A3h 0163   1]                       Length : 1C
> > [0A4h 0164   2]                     Reserved : 0000
> > [0A6h 0166   4]        Flags (decoded below) : 00000003
> >                             Physical package : 1
> >                      ACPI Processor ID valid : 1
> > [0AAh 0170   4]                       Parent : 00000000
> > [0AEh 0174   4]            ACPI Processor ID : 00001000
> > [0B2h 0178   4]      Private Resource Number : 00000002
> > [0B6h 0182   4]             Private Resource : 0000006C
> > [0BAh 0186   4]             Private Resource : 00000084
> > 
> > So, when "cat physical_package" in /sys/devices/system/cpu/cpu0/topology/,
> > it will output 162(0xA2). And if some items are added before the node
> > above, the output will change to other value.
> > 
> > This patch maps the node offset to a logic package id. It maps the first
> > node offset to 0, the second to 1, and so on.
> > 
> > Then, it will not output a big value, such as 162 above. And it will
> > not change when some nodes(Physical package not set) are added.
> > 
> > And as long as the nodes with Physical package field set in PPTT keeps
> > the real hardware order, the logic id can map to hardware package id to
> > some extent.
> > 
> > Hope to get feedback from you.
> 
> Thanks for the patch, but Andrew Jones has also posted a patch[1] which
> I had a look but was not sure what is the best approach to fix it yet.
> I will think about it and respond to that.
>

I'll send a v1 yet today. The RFC version was actually OK, as the concern
with ACPI nodes not being in the expected order wasn't actually a problem.
The thread-id or core-id would only be reset to zero when a yet to be
remapped core-id (and all its peers) was found when iterating the PEs.
Since all peers were handled at the same time, the counter reset was
correct, even when the ACPI nodes were out-of-order. The code didn't make
that very obvious, though, and there was some room for other cleanups,
so I've reworked it. Once I run it through a couple more rounds of testing
I'll repost.

FYI, I'm able to easily test a variety of configs using a KVM guest and
this QEMU branch[*]. To use threads it's necessary to revert the last
QEMU patch and to hack KVM to set MPIDR.MT for the VCPUs.

Thanks,
drew

[*] https://github.com/rhdrjones/qemu/commits/virt-cpu-topology

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC PATCH] arm64: topology: Map PPTT node offset to logic physical package id
  2018-06-28 11:57   ` Andrew Jones
@ 2018-06-28 12:12     ` Sudeep Holla
  2018-06-28 13:19       ` Jeremy Linton
  0 siblings, 1 reply; 11+ messages in thread
From: Sudeep Holla @ 2018-06-28 12:12 UTC (permalink / raw)
  To: Andrew Jones
  Cc: Sudeep Holla, Shunyong Yang, catalin.marinas, will.deacon,
	linux-kernel, jeremy.linton, Joey Zheng



On 28/06/18 12:57, Andrew Jones wrote:
> On Thu, Jun 28, 2018 at 10:38:24AM +0100, Sudeep Holla wrote:
>> Hi Shunyong,
>>
>> On 28/06/18 10:18, Shunyong Yang wrote:
>>> As PPTT spec doesn't define the physical package id,
>>> find_acpi_cpu_topology_package() will return offset of the node with
>>> Physical package field set when querying physical package id. So, it
>>> returns 162(0xA2) in following example.
>>>
>>> [0A2h 0162   1]                Subtable Type : 00 [Processor Hierarchy
>>> Node]
>>> [0A3h 0163   1]                       Length : 1C
>>> [0A4h 0164   2]                     Reserved : 0000
>>> [0A6h 0166   4]        Flags (decoded below) : 00000003
>>>                             Physical package : 1
>>>                      ACPI Processor ID valid : 1
>>> [0AAh 0170   4]                       Parent : 00000000
>>> [0AEh 0174   4]            ACPI Processor ID : 00001000
>>> [0B2h 0178   4]      Private Resource Number : 00000002
>>> [0B6h 0182   4]             Private Resource : 0000006C
>>> [0BAh 0186   4]             Private Resource : 00000084
>>>
>>> So, when "cat physical_package" in /sys/devices/system/cpu/cpu0/topology/,
>>> it will output 162(0xA2). And if some items are added before the node
>>> above, the output will change to other value.
>>>
>>> This patch maps the node offset to a logic package id. It maps the first
>>> node offset to 0, the second to 1, and so on.
>>>
>>> Then, it will not output a big value, such as 162 above. And it will
>>> not change when some nodes(Physical package not set) are added.
>>>
>>> And as long as the nodes with Physical package field set in PPTT keeps
>>> the real hardware order, the logic id can map to hardware package id to
>>> some extent.
>>>
>>> Hope to get feedback from you.
>>
>> Thanks for the patch, but Andrew Jones has also posted a patch[1] which
>> I had a look but was not sure what is the best approach to fix it yet.
>> I will think about it and respond to that.
>>
> 
> I'll send a v1 yet today. The RFC version was actually OK, as the concern
> with ACPI nodes not being in the expected order wasn't actually a problem.
> The thread-id or core-id would only be reset to zero when a yet to be
> remapped core-id (and all its peers) was found when iterating the PEs.
> Since all peers were handled at the same time, the counter reset was
> correct, even when the ACPI nodes were out-of-order. The code didn't make
> that very obvious, though, and there was some room for other cleanups,
> so I've reworked it. Once I run it through a couple more rounds of testing
> I'll repost.
> 

OK sure. I liked the approach in Shunyong's patch. I was thinking if we
can avoid the list and dynamic allocation on each addition and make it
more simpler.

-- 
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC PATCH] arm64: topology: Map PPTT node offset to logic physical package id
  2018-06-28 12:12     ` Sudeep Holla
@ 2018-06-28 13:19       ` Jeremy Linton
  2018-06-28 14:09         ` Sudeep Holla
  0 siblings, 1 reply; 11+ messages in thread
From: Jeremy Linton @ 2018-06-28 13:19 UTC (permalink / raw)
  To: Sudeep Holla, Andrew Jones
  Cc: Shunyong Yang, catalin.marinas, will.deacon, linux-kernel, Joey Zheng

Hi,

On 06/28/2018 07:12 AM, Sudeep Holla wrote:
> 
> 
> On 28/06/18 12:57, Andrew Jones wrote:
>> On Thu, Jun 28, 2018 at 10:38:24AM +0100, Sudeep Holla wrote:
>>> Hi Shunyong,
>>>
>>> On 28/06/18 10:18, Shunyong Yang wrote:
>>>> As PPTT spec doesn't define the physical package id,
>>>> find_acpi_cpu_topology_package() will return offset of the node with
>>>> Physical package field set when querying physical package id. So, it
>>>> returns 162(0xA2) in following example.
>>>>
>>>> [0A2h 0162   1]                Subtable Type : 00 [Processor Hierarchy
>>>> Node]
>>>> [0A3h 0163   1]                       Length : 1C
>>>> [0A4h 0164   2]                     Reserved : 0000
>>>> [0A6h 0166   4]        Flags (decoded below) : 00000003
>>>>                              Physical package : 1
>>>>                       ACPI Processor ID valid : 1
>>>> [0AAh 0170   4]                       Parent : 00000000
>>>> [0AEh 0174   4]            ACPI Processor ID : 00001000
>>>> [0B2h 0178   4]      Private Resource Number : 00000002
>>>> [0B6h 0182   4]             Private Resource : 0000006C
>>>> [0BAh 0186   4]             Private Resource : 00000084
>>>>
>>>> So, when "cat physical_package" in /sys/devices/system/cpu/cpu0/topology/,
>>>> it will output 162(0xA2). And if some items are added before the node
>>>> above, the output will change to other value.
>>>>
>>>> This patch maps the node offset to a logic package id. It maps the first
>>>> node offset to 0, the second to 1, and so on.
>>>>
>>>> Then, it will not output a big value, such as 162 above. And it will
>>>> not change when some nodes(Physical package not set) are added.
>>>>
>>>> And as long as the nodes with Physical package field set in PPTT keeps
>>>> the real hardware order, the logic id can map to hardware package id to
>>>> some extent.
>>>>
>>>> Hope to get feedback from you.
>>>
>>> Thanks for the patch, but Andrew Jones has also posted a patch[1] which
>>> I had a look but was not sure what is the best approach to fix it yet.
>>> I will think about it and respond to that.
>>>
>>
>> I'll send a v1 yet today. The RFC version was actually OK, as the concern
>> with ACPI nodes not being in the expected order wasn't actually a problem.
>> The thread-id or core-id would only be reset to zero when a yet to be
>> remapped core-id (and all its peers) was found when iterating the PEs.
>> Since all peers were handled at the same time, the counter reset was
>> correct, even when the ACPI nodes were out-of-order. The code didn't make
>> that very obvious, though, and there was some room for other cleanups,
>> so I've reworked it. Once I run it through a couple more rounds of testing
>> I'll repost.
>>
> 
> OK sure. I liked the approach in Shunyong's patch. I was thinking if we
> can avoid the list and dynamic allocation on each addition and make it
> more simpler.
> 

This one reads simpler, but yes I agree we should try to avoid the 
dynamic allocation.

OTOH, I think that dropping the dynamic allocation leads to an algorithm 
that picks a value and replaces all the matches. Which of course is 
Andrew's patch, although I did have to read it a couple times to get a 
grasp how it works. I'm guessing that is due to the fact that he seems 
to have optimized 3 double loops into a single loop with two individual 
nested loops. AKA its probably more efficient than the naive 
implementation, but readability seems to have suffered a bit in the 
initial version he posted. I'm not sure the optimization is worth it, 
but I'm guessing there is a middle ground which makes it more readable.

Finally, @Shunyong, thanks for putting the effort into this...

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC PATCH] arm64: topology: Map PPTT node offset to logic physical package id
  2018-06-28 13:19       ` Jeremy Linton
@ 2018-06-28 14:09         ` Sudeep Holla
  2018-06-28 14:51           ` Andrew Jones
  0 siblings, 1 reply; 11+ messages in thread
From: Sudeep Holla @ 2018-06-28 14:09 UTC (permalink / raw)
  To: Jeremy Linton, Andrew Jones
  Cc: Sudeep Holla, Shunyong Yang, catalin.marinas, will.deacon,
	linux-kernel, Joey Zheng



On 28/06/18 14:19, Jeremy Linton wrote:
> Hi,
> 
> On 06/28/2018 07:12 AM, Sudeep Holla wrote:

[...]

>>
>> OK sure. I liked the approach in Shunyong's patch. I was thinking if we
>> can avoid the list and dynamic allocation on each addition and make it
>> more simpler.
>>
> 
> This one reads simpler, but yes I agree we should try to avoid the
> dynamic allocation.
> 
> OTOH, I think that dropping the dynamic allocation leads to an algorithm
> that picks a value and replaces all the matches. Which of course is
> Andrew's patch, although I did have to read it a couple times to get a
> grasp how it works. I'm guessing that is due to the fact that he seems
> to have optimized 3 double loops into a single loop with two individual
> nested loops. AKA its probably more efficient than the naive
> implementation, but readability seems to have suffered a bit in the
> initial version he posted. I'm not sure the optimization is worth it,
> but I'm guessing there is a middle ground which makes it more readable.
> 

Completely agree. RFC from Andrew is not so readable and easy to understand.

-- 
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC PATCH] arm64: topology: Map PPTT node offset to logic physical package id
  2018-06-28 14:09         ` Sudeep Holla
@ 2018-06-28 14:51           ` Andrew Jones
  2018-06-28 15:44             ` Yang, Shunyong
  0 siblings, 1 reply; 11+ messages in thread
From: Andrew Jones @ 2018-06-28 14:51 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: Jeremy Linton, Shunyong Yang, catalin.marinas, will.deacon,
	linux-kernel, Joey Zheng

On Thu, Jun 28, 2018 at 03:09:19PM +0100, Sudeep Holla wrote:
> 
> 
> On 28/06/18 14:19, Jeremy Linton wrote:
> > Hi,
> > 
> > On 06/28/2018 07:12 AM, Sudeep Holla wrote:
> 
> [...]
> 
> >>
> >> OK sure. I liked the approach in Shunyong's patch. I was thinking if we
> >> can avoid the list and dynamic allocation on each addition and make it
> >> more simpler.
> >>
> > 
> > This one reads simpler, but yes I agree we should try to avoid the
> > dynamic allocation.
> > 
> > OTOH, I think that dropping the dynamic allocation leads to an algorithm
> > that picks a value and replaces all the matches. Which of course is
> > Andrew's patch, although I did have to read it a couple times to get a
> > grasp how it works. I'm guessing that is due to the fact that he seems
> > to have optimized 3 double loops into a single loop with two individual
> > nested loops. AKA its probably more efficient than the naive
> > implementation, but readability seems to have suffered a bit in the
> > initial version he posted. I'm not sure the optimization is worth it,
> > but I'm guessing there is a middle ground which makes it more readable.
> > 
> 
> Completely agree. RFC from Andrew is not so readable and easy to understand.

Middle ground coming up. At the expense of a triple-nested loop (which
will never be N^3 iterations due to conditions at the start of each loop),
we can avoid dynamic allocations and list iterations and still gain
readability.

Thanks,
drew

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC PATCH] arm64: topology: Map PPTT node offset to logic physical package id
  2018-06-28 14:51           ` Andrew Jones
@ 2018-06-28 15:44             ` Yang, Shunyong
  2018-06-28 16:45               ` Sudeep Holla
  0 siblings, 1 reply; 11+ messages in thread
From: Yang, Shunyong @ 2018-06-28 15:44 UTC (permalink / raw)
  To: Andrew Jones
  Cc: Sudeep Holla, Jeremy Linton, catalin.marinas, will.deacon,
	linux-kernel, Zheng, Joey

Hi, All

> On Jun 28, 2018, at 22:51, Andrew Jones <drjones@redhat.com> wrote:
> 
>> On Thu, Jun 28, 2018 at 03:09:19PM +0100, Sudeep Holla wrote:
>> 
>> 
>>> On 28/06/18 14:19, Jeremy Linton wrote:
>>> Hi,
>>> 
>>> On 06/28/2018 07:12 AM, Sudeep Holla wrote:
>> 
>> [...]
>> 
>>>> 
>>>> OK sure. I liked the approach in Shunyong's patch. I was thinking if we
>>>> can avoid the list and dynamic allocation on each addition and make it
>>>> more simpler.
>>>> 
>>> 
>>> This one reads simpler, but yes I agree we should try to avoid the
>>> dynamic allocation.
>>> 
>>> OTOH, I think that dropping the dynamic allocation leads to an algorithm
>>> that picks a value and replaces all the matches. Which of course is
>>> Andrew's patch, although I did have to read it a couple times to get a
>>> grasp how it works. I'm guessing that is due to the fact that he seems
>>> to have optimized 3 double loops into a single loop with two individual
>>> nested loops. AKA its probably more efficient than the naive
>>> implementation, but readability seems to have suffered a bit in the
>>> initial version he posted. I'm not sure the optimization is worth it,
>>> but I'm guessing there is a middle ground which makes it more readable.
>>> 
>> 
>> Completely agree. RFC from Andrew is not so readable and easy to understand.
> 
> Middle ground coming up. At the expense of a triple-nested loop (which
> will never be N^3 iterations due to conditions at the start of each loop),
> we can avoid dynamic allocations and list iterations and still gain
> readability.
> 
> Thanks,
> drew

I have a new approach. As we've already got the offset of the node with physical package bit set, which is the parent of the cpu we are querying. We can iterate from the begining of PPTT to count the nodes with physical package bit set till we reach the offset we've got. Then, the count value is the package id. This avoid list and dynamic allocation. And PPTT provides length for each node, iteration should be easy.
I think this may implemented in pptt.c. 
I am writing this mail on my phone. Maybe I should try to write a patch to test in office tomorrow if you think it's feasible.

Thanks.
Shunyong.




^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC PATCH] arm64: topology: Map PPTT node offset to logic physical package id
  2018-06-28 15:44             ` Yang, Shunyong
@ 2018-06-28 16:45               ` Sudeep Holla
  0 siblings, 0 replies; 11+ messages in thread
From: Sudeep Holla @ 2018-06-28 16:45 UTC (permalink / raw)
  To: Yang, Shunyong, Andrew Jones
  Cc: Sudeep Holla, Jeremy Linton, catalin.marinas, will.deacon,
	linux-kernel, Zheng, Joey



On 28/06/18 16:44, Yang, Shunyong wrote:
> Hi, All
>
>> On Jun 28, 2018, at 22:51, Andrew Jones <drjones@redhat.com>
>> wrote:
>>
>>> On Thu, Jun 28, 2018 at 03:09:19PM +0100, Sudeep Holla wrote:
>>>
>>>
>>>> On 28/06/18 14:19, Jeremy Linton wrote: Hi,
>>>>
>>>> On 06/28/2018 07:12 AM, Sudeep Holla wrote:
>>>
>>> [...]
>>>
>>>>>
>>>>> OK sure. I liked the approach in Shunyong's patch. I was
>>>>> thinking if we can avoid the list and dynamic allocation on
>>>>> each addition and make it more simpler.
>>>>>
>>>>
>>>> This one reads simpler, but yes I agree we should try to avoid
>>>> the dynamic allocation.
>>>>
>>>> OTOH, I think that dropping the dynamic allocation leads to an
>>>> algorithm that picks a value and replaces all the matches.
>>>> Which of course is Andrew's patch, although I did have to read
>>>> it a couple times to get a grasp how it works. I'm guessing
>>>> that is due to the fact that he seems to have optimized 3
>>>> double loops into a single loop with two individual nested
>>>> loops. AKA its probably more efficient than the naive
>>>> implementation, but readability seems to have suffered a bit in
>>>> the initial version he posted. I'm not sure the optimization is
>>>> worth it, but I'm guessing there is a middle ground which makes
>>>> it more readable.
>>>>
>>>
>>> Completely agree. RFC from Andrew is not so readable and easy to
>>> understand.
>>
>> Middle ground coming up. At the expense of a triple-nested loop
>> (which will never be N^3 iterations due to conditions at the start
>> of each loop), we can avoid dynamic allocations and list iterations
>> and still gain readability.
>>
>> Thanks, drew
>
> I have a new approach. As we've already got the offset of the node
> with physical package bit set, which is the parent of the cpu we are
> querying. We can iterate from the begining of PPTT to count the nodes
> with physical package bit set till we reach the offset we've got.
> Then, the count value is the package id.

I was thinking of simple solution like add the offset to sorted array
and assign the index to that. In this way if ACPI_PROCESSOR_ID_VALID
flag is set at the package level too and they start and increase
linearly from 0, we are matching them(requires 1 line change I posted in
the other thread)

--
Regards,
Sudeep
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC PATCH] arm64: topology: Map PPTT node offset to logic physical package id
  2018-06-28  9:18 [RFC PATCH] arm64: topology: Map PPTT node offset to logic physical package id Shunyong Yang
  2018-06-28  9:38 ` Sudeep Holla
@ 2018-07-12 11:06 ` Will Deacon
  2018-07-12 11:20   ` Sudeep Holla
  1 sibling, 1 reply; 11+ messages in thread
From: Will Deacon @ 2018-07-12 11:06 UTC (permalink / raw)
  To: Shunyong Yang
  Cc: catalin.marinas, jeremy.linton, linux-arm-kernel, linux-kernel,
	Joey Zheng, sudeep.holla

On Thu, Jun 28, 2018 at 05:18:28PM +0800, Shunyong Yang wrote:
> As PPTT spec doesn't define the physical package id,
> find_acpi_cpu_topology_package() will return offset of the node with
> Physical package field set when querying physical package id. So, it
> returns 162(0xA2) in following example.
> 
> [0A2h 0162   1]                Subtable Type : 00 [Processor Hierarchy
> Node]
> [0A3h 0163   1]                       Length : 1C
> [0A4h 0164   2]                     Reserved : 0000
> [0A6h 0166   4]        Flags (decoded below) : 00000003
>                             Physical package : 1
>                      ACPI Processor ID valid : 1
> [0AAh 0170   4]                       Parent : 00000000
> [0AEh 0174   4]            ACPI Processor ID : 00001000
> [0B2h 0178   4]      Private Resource Number : 00000002
> [0B6h 0182   4]             Private Resource : 0000006C
> [0BAh 0186   4]             Private Resource : 00000084
> 
> So, when "cat physical_package" in /sys/devices/system/cpu/cpu0/topology/,
> it will output 162(0xA2). And if some items are added before the node
> above, the output will change to other value.
> 
> This patch maps the node offset to a logic package id. It maps the first
> node offset to 0, the second to 1, and so on.
> 
> Then, it will not output a big value, such as 162 above. And it will
> not change when some nodes(Physical package not set) are added.
> 
> And as long as the nodes with Physical package field set in PPTT keeps
> the real hardware order, the logic id can map to hardware package id to
> some extent.
> 
> Hope to get feedback from you.

I'm assuming this is no longer needed now that we have queued the series
from Sudeep?

Will

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC PATCH] arm64: topology: Map PPTT node offset to logic physical package id
  2018-07-12 11:06 ` Will Deacon
@ 2018-07-12 11:20   ` Sudeep Holla
  0 siblings, 0 replies; 11+ messages in thread
From: Sudeep Holla @ 2018-07-12 11:20 UTC (permalink / raw)
  To: Will Deacon, Shunyong Yang
  Cc: Sudeep Holla, catalin.marinas, jeremy.linton, linux-arm-kernel,
	linux-kernel, Joey Zheng

Hi Will,

On 12/07/18 12:06, Will Deacon wrote:
> On Thu, Jun 28, 2018 at 05:18:28PM +0800, Shunyong Yang wrote:

[..]

>>
>> And as long as the nodes with Physical package field set in PPTT keeps
>> the real hardware order, the logic id can map to hardware package id to
>> some extent.
>>
>> Hope to get feedback from you.
> 
> I'm assuming this is no longer needed now that we have queued the series
> from Sudeep?
> 

The series relating to topology/numa that you have queued helps us to
re-introduces numa mask check that was reverted partial in v4.18 and is
not related to the issue reported or addressed by this patch.

However, there's no proper solution to the issue reported in this thread
and the one from Andrew Jones[1]. The only solution is to rely on ACPI
firmware for that instead of trying to fix in OS and we have already
merged the patch to fix that in acpi/pptt.c (Commit 30998033f62a ("ACPI
/ PPTT: use ACPI ID whenever ACPI_PPTT_ACPI_PROCESSOR_ID_VALID is set"))

In short, all the know issues are addressed so far and nothing else
needs to be queued for now.

-- 
Regards,
Sudeep

[1]
https://lore.kernel.org/lkml/20180629180308.zdl4taihzv2zwarc@kamzik.brq.redhat.com/T/#t

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2018-07-12 11:20 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-28  9:18 [RFC PATCH] arm64: topology: Map PPTT node offset to logic physical package id Shunyong Yang
2018-06-28  9:38 ` Sudeep Holla
2018-06-28 11:57   ` Andrew Jones
2018-06-28 12:12     ` Sudeep Holla
2018-06-28 13:19       ` Jeremy Linton
2018-06-28 14:09         ` Sudeep Holla
2018-06-28 14:51           ` Andrew Jones
2018-06-28 15:44             ` Yang, Shunyong
2018-06-28 16:45               ` Sudeep Holla
2018-07-12 11:06 ` Will Deacon
2018-07-12 11:20   ` Sudeep Holla

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).