[PATCH v2 0/4] Revert works for the mapping of cpuid <-> nodeid

[PATCH v2 0/4] Revert works for the mapping of cpuid <-> nodeid

[Dou Liyang]

Currently, We make the mapping of "cpuid <-> nodeid" fixed at the booting time.
It keeps consistent with the WorkQueue and avoids some bugs which may be caused
by the dynamic assignment.
As we know, It is implemented by the patches as follows: 2532fc318d, f7c28833c2,
8f54969dc8, 8ad893faf2, dc6db24d24, which depend on ACPI table. Simply speaking:

Step 1. Make the "Logical CPU ID <-> Processor ID/UID" fixed Using MADT:
We generate the logical CPU IDs by the Local APIC/x2APIC IDs orderly and
get the mapping of Processor ID/UID <-> Local Apic ID directly in MADT.
So, we get the mapping of
*Processor ID/UID <-> Local Apic ID <-> Logical CPU ID*

Step 2. Make the "Processor ID/UID <-> Node ID(_PXM)" fixed Using DSDT:
The maaping of "Processor ID/UID <-> Node ID(_PXM)" is ready-made in
each entities. we just use it directly.

So, at last we get the maaping of *Node ID <-> Logical CPU ID* according to
step1 and step2:
*Node ID(_PXM) <-> Processor ID/UID <-> Local Apic ID <-> Logical CPU ID*

But, The ACPI table is unreliable and it is very risky that we use the entity
which isn't related to a physical device at booting time. Here has already two
bugs we found.
1. Duplicated Processor IDs in DSDT.
	It has been fixed by commit 8e089eaa19, fd74da217d.
2. The _PXM in DSDT is inconsistent with the one in MADT.
	It may cause the bug, which is shown in:
		https://lkml.org/lkml/2017/2/12/200
There may be more later. We shouldn't just only fix them everytime, we should
solve this problem from the source to avoid such problems happend again and
again.

Now, a simple and easy way is found, we revert our patches. Do the Step 2 
at hot-plug time, not at booting time where we did some useless work.

It also can make the mapping of "cpuid <-> nodeid" fixed and avoid excessive
use of the ACPI table.

We have tested them in our box: Fujitsu PQ2000 with 2 nodes for hot-plug.
To Xiaolong: 
	Please help me to test it in the special machine.

Change log:
  v1 -> v2: 1. fix some comments.
            2. add the verification of duplicate processor id.

Dou Liyang (4):
  Revert"x86/acpi: Set persistent cpuid <-> nodeid mapping when booting"
  Revert"x86/acpi: Enable MADT APIs to return disabled apicids"
  acpi: Fix the check handle in case of declaring processors using the
    Device operator
  acpi: Move the verification of duplicate proc_id from booting time to
    hot-plug time

 arch/x86/kernel/acpi/boot.c   |   2 +-
 drivers/acpi/acpi_processor.c |  50 +++++++++++-----
 drivers/acpi/bus.c            |   1 -
 drivers/acpi/processor_core.c | 133 +++++++-----------------------------------
 include/linux/acpi.h          |   5 +-
 5 files changed, 59 insertions(+), 132 deletions(-)

-- 
2.5.5

[PATCH v2 1/4] Revert"x86/acpi: Set persistent cpuid <-> nodeid mapping when booting"

[Dou Liyang]

Currently, We make the mapping of "cpuid <-> nodeid" fixed at the booting time.
It keeps consistent with the WorkQueue and avoids some bugs which may be caused
by the dynamic assignment.

But, The ACPI table is unreliable and it is very risky that we use the entity
which isn't related to a physical device at booting time.

Now, we revert our patches. Do the last mapping of "cpuid <-> nodeid" at
hot-plug time, not at booting time where we did some useless work.
It also can make the mapping of "cpuid <-> nodeid" fixed and avoid excessive
use of the ACPI table.

The patch revert the commit dc6db24d24:
  "x86/acpi: Set persistent cpuid <-> nodeid mapping when booting".

Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
---
 arch/x86/kernel/acpi/boot.c   |  2 +-
 drivers/acpi/acpi_processor.c |  5 ---
 drivers/acpi/bus.c            |  1 -
 drivers/acpi/processor_core.c | 73 -------------------------------------------
 include/linux/acpi.h          |  3 --
 5 files changed, 1 insertion(+), 83 deletions(-)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 64422f8..32846a2 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -709,7 +709,7 @@ static void __init acpi_set_irq_model_ioapic(void)
 #ifdef CONFIG_ACPI_HOTPLUG_CPU
 #include <acpi/processor.h>
 
-int acpi_map_cpu2node(acpi_handle handle, int cpu, int physid)
+static int acpi_map_cpu2node(acpi_handle handle, int cpu, int physid)
 {
 #ifdef CONFIG_ACPI_NUMA
 	int nid;
diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index 3de3b6b..f43a586 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -182,11 +182,6 @@ int __weak arch_register_cpu(int cpu)
 
 void __weak arch_unregister_cpu(int cpu) {}
 
-int __weak acpi_map_cpu2node(acpi_handle handle, int cpu, int physid)
-{
-	return -ENODEV;
-}
-
 static int acpi_processor_hotadd_init(struct acpi_processor *pr)
 {
 	unsigned long long sta;
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 95855cb..d4455e4 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -1207,7 +1207,6 @@ static int __init acpi_init(void)
 	acpi_wakeup_device_init();
 	acpi_debugger_init();
 	acpi_setup_sb_notify_handler();
-	acpi_set_processor_mapping();
 	return 0;
 }
 
diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
index 611a558..a843862 100644
--- a/drivers/acpi/processor_core.c
+++ b/drivers/acpi/processor_core.c
@@ -278,79 +278,6 @@ int acpi_get_cpuid(acpi_handle handle, int type, u32 acpi_id)
 }
 EXPORT_SYMBOL_GPL(acpi_get_cpuid);
 
-#ifdef CONFIG_ACPI_HOTPLUG_CPU
-static bool __init
-map_processor(acpi_handle handle, phys_cpuid_t *phys_id, int *cpuid)
-{
-	int type, id;
-	u32 acpi_id;
-	acpi_status status;
-	acpi_object_type acpi_type;
-	unsigned long long tmp;
-	union acpi_object object = { 0 };
-	struct acpi_buffer buffer = { sizeof(union acpi_object), &object };
-
-	status = acpi_get_type(handle, &acpi_type);
-	if (ACPI_FAILURE(status))
-		return false;
-
-	switch (acpi_type) {
-	case ACPI_TYPE_PROCESSOR:
-		status = acpi_evaluate_object(handle, NULL, NULL, &buffer);
-		if (ACPI_FAILURE(status))
-			return false;
-		acpi_id = object.processor.proc_id;
-
-		/* validate the acpi_id */
-		if(acpi_processor_validate_proc_id(acpi_id))
-			return false;
-		break;
-	case ACPI_TYPE_DEVICE:
-		status = acpi_evaluate_integer(handle, "_UID", NULL, &tmp);
-		if (ACPI_FAILURE(status))
-			return false;
-		acpi_id = tmp;
-		break;
-	default:
-		return false;
-	}
-
-	type = (acpi_type == ACPI_TYPE_DEVICE) ? 1 : 0;
-
-	*phys_id = __acpi_get_phys_id(handle, type, acpi_id, false);
-	id = acpi_map_cpuid(*phys_id, acpi_id);
-
-	if (id < 0)
-		return false;
-	*cpuid = id;
-	return true;
-}
-
-static acpi_status __init
-set_processor_node_mapping(acpi_handle handle, u32 lvl, void *context,
-			   void **rv)
-{
-	phys_cpuid_t phys_id;
-	int cpu_id;
-
-	if (!map_processor(handle, &phys_id, &cpu_id))
-		return AE_ERROR;
-
-	acpi_map_cpu2node(handle, cpu_id, phys_id);
-	return AE_OK;
-}
-
-void __init acpi_set_processor_mapping(void)
-{
-	/* Set persistent cpu <-> node mapping for all processors. */
-	acpi_walk_namespace(ACPI_TYPE_PROCESSOR, ACPI_ROOT_OBJECT,
-			    ACPI_UINT32_MAX, set_processor_node_mapping,
-			    NULL, NULL, NULL);
-}
-#else
-void __init acpi_set_processor_mapping(void) {}
-#endif /* CONFIG_ACPI_HOTPLUG_CPU */
-
 #ifdef CONFIG_ACPI_HOTPLUG_IOAPIC
 static int get_ioapic_id(struct acpi_subtable_header *entry, u32 gsi_base,
 			 u64 *phys_addr, int *ioapic_id)
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 5b36974..d180cbd 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -293,11 +293,8 @@ bool acpi_processor_validate_proc_id(int proc_id);
 /* Arch dependent functions for cpu hotplug support */
 int acpi_map_cpu(acpi_handle handle, phys_cpuid_t physid, int *pcpu);
 int acpi_unmap_cpu(int cpu);
-int acpi_map_cpu2node(acpi_handle handle, int cpu, int physid);
 #endif /* CONFIG_ACPI_HOTPLUG_CPU */
 
-void acpi_set_processor_mapping(void);
-
 #ifdef CONFIG_ACPI_HOTPLUG_IOAPIC
 int acpi_get_ioapic_id(acpi_handle handle, u32 gsi_base, u64 *phys_addr);
 #endif
-- 
2.5.5

Re: [PATCH v2 1/4] Revert"x86/acpi: Set persistent cpuid <-> nodeid mapping when booting"

[Thomas Gleixner]

On Mon, 20 Feb 2017, Dou Liyang wrote:

> Currently, We make the mapping of "cpuid <-> nodeid" fixed at the booting time.
> It keeps consistent with the WorkQueue and avoids some bugs which may be caused
> by the dynamic assignment.
> 
> But, The ACPI table is unreliable and it is very risky that we use the entity
> which isn't related to a physical device at booting time.
> 
> Now, we revert our patches. Do the last mapping of "cpuid <-> nodeid" at
> hot-plug time, not at booting time where we did some useless work.
> It also can make the mapping of "cpuid <-> nodeid" fixed and avoid excessive
> use of the ACPI table.
> 
> The patch revert the commit dc6db24d24:
>   "x86/acpi: Set persistent cpuid <-> nodeid mapping when booting".

That changelog needs some massaging. Something like this:

  The mapping of "cpuid <-> nodeid" is established at boot time via ACPI
  tables to keep associations of workqueues and other node related items
  consistent across cpu hotplug.

  But, ACPI tables are unreliable and failures with that boot time mapping
  have been reported on machines where the ACPI table and the physical
  information which is retrieved at actual hotplug is inconsistent.

  Revert the mapping implementation so it can be replaced with a less error
  prone approach.

This clearly describes:

  1) The context

  2) The problem

  3) The solution (revert)

You don't have to explain what the new solution will be in the changelog of
the revert. For the revert it's only relevant WHY we do the revert.

Please avoid writing changelogs in 'we' form. Write it pure technical, like
a manual.

Also avoid phrases like: "The patch/This patch". We all know already that
this is a patch, otherwise it wouldn't have been sent.

Documentation/process/submitting-patches.rst says:

  Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
  instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
  to do frotz", as if you are giving orders to the codebase to change
  its behaviour.

Thanks,

	tglx

Re: [PATCH v2 1/4] Revert"x86/acpi: Set persistent cpuid <-> nodeid mapping when booting"

[Dou Liyang]

Hi tglx,

Thank you very much for your guidance! It makes me more profound
understanding of the changelog. And you also rewrote my changelog for
giving me an example.

I am so grateful that you can help me so carefully.
Once I heard the charm of the open source community, Now i can
really feel it. I love it so much.

I will try to improve myself and help others.  :)

Thanks,
	Liyang.

At 03/01/2017 06:51 PM, Thomas Gleixner wrote:
> On Mon, 20 Feb 2017, Dou Liyang wrote:
>
>> Currently, We make the mapping of "cpuid <-> nodeid" fixed at the booting time.
>> It keeps consistent with the WorkQueue and avoids some bugs which may be caused
>> by the dynamic assignment.
>>
>> But, The ACPI table is unreliable and it is very risky that we use the entity
>> which isn't related to a physical device at booting time.
>>
>> Now, we revert our patches. Do the last mapping of "cpuid <-> nodeid" at
>> hot-plug time, not at booting time where we did some useless work.
>> It also can make the mapping of "cpuid <-> nodeid" fixed and avoid excessive
>> use of the ACPI table.
>>
>> The patch revert the commit dc6db24d24:
>>   "x86/acpi: Set persistent cpuid <-> nodeid mapping when booting".
>
> That changelog needs some massaging. Something like this:
>
>   The mapping of "cpuid <-> nodeid" is established at boot time via ACPI
>   tables to keep associations of workqueues and other node related items
>   consistent across cpu hotplug.
>
>   But, ACPI tables are unreliable and failures with that boot time mapping
>   have been reported on machines where the ACPI table and the physical
>   information which is retrieved at actual hotplug is inconsistent.
>
>   Revert the mapping implementation so it can be replaced with a less error
>   prone approach.
>
> This clearly describes:
>
>   1) The context
>
>   2) The problem
>
>   3) The solution (revert)
>
> You don't have to explain what the new solution will be in the changelog of
> the revert. For the revert it's only relevant WHY we do the revert.
>
> Please avoid writing changelogs in 'we' form. Write it pure technical, like
> a manual.
>
> Also avoid phrases like: "The patch/This patch". We all know already that
> this is a patch, otherwise it wouldn't have been sent.
>
> Documentation/process/submitting-patches.rst says:
>
>   Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
>   instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
>   to do frotz", as if you are giving orders to the codebase to change
>   its behaviour.
>
> Thanks,
>
> 	tglx
>
>
>
>

[PATCH v2 2/4] Revert"x86/acpi: Enable MADT APIs to return disabled apicids"

[Dou Liyang]

After we never do the last mapping of "cpuid <-> nodeid" at booting time. we
also no need to enable MADT APIs to return disabled apicid.

So, The patch work for reverting the commit 8ad893faf2:
"x86/acpi: Enable MADT APIs to return disabled apicids"

Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
---
 drivers/acpi/processor_core.c | 60 ++++++++++++++++---------------------------
 1 file changed, 22 insertions(+), 38 deletions(-)

diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
index a843862..b933061 100644
--- a/drivers/acpi/processor_core.c
+++ b/drivers/acpi/processor_core.c
@@ -32,12 +32,12 @@ static struct acpi_table_madt *get_madt_table(void)
 }
 
 static int map_lapic_id(struct acpi_subtable_header *entry,
-		 u32 acpi_id, phys_cpuid_t *apic_id, bool ignore_disabled)
+		 u32 acpi_id, phys_cpuid_t *apic_id)
 {
 	struct acpi_madt_local_apic *lapic =
 		container_of(entry, struct acpi_madt_local_apic, header);
 
-	if (ignore_disabled && !(lapic->lapic_flags & ACPI_MADT_ENABLED))
+	if (!(lapic->lapic_flags & ACPI_MADT_ENABLED))
 		return -ENODEV;
 
 	if (lapic->processor_id != acpi_id)
@@ -48,13 +48,12 @@ static int map_lapic_id(struct acpi_subtable_header *entry,
 }
 
 static int map_x2apic_id(struct acpi_subtable_header *entry,
-		int device_declaration, u32 acpi_id, phys_cpuid_t *apic_id,
-		bool ignore_disabled)
+		int device_declaration, u32 acpi_id, phys_cpuid_t *apic_id)
 {
 	struct acpi_madt_local_x2apic *apic =
 		container_of(entry, struct acpi_madt_local_x2apic, header);
 
-	if (ignore_disabled && !(apic->lapic_flags & ACPI_MADT_ENABLED))
+	if (!(apic->lapic_flags & ACPI_MADT_ENABLED))
 		return -ENODEV;
 
 	if (device_declaration && (apic->uid == acpi_id)) {
@@ -66,13 +65,12 @@ static int map_x2apic_id(struct acpi_subtable_header *entry,
 }
 
 static int map_lsapic_id(struct acpi_subtable_header *entry,
-		int device_declaration, u32 acpi_id, phys_cpuid_t *apic_id,
-		bool ignore_disabled)
+		int device_declaration, u32 acpi_id, phys_cpuid_t *apic_id)
 {
 	struct acpi_madt_local_sapic *lsapic =
 		container_of(entry, struct acpi_madt_local_sapic, header);
 
-	if (ignore_disabled && !(lsapic->lapic_flags & ACPI_MADT_ENABLED))
+	if (!(lsapic->lapic_flags & ACPI_MADT_ENABLED))
 		return -ENODEV;
 
 	if (device_declaration) {
@@ -89,13 +87,12 @@ static int map_lsapic_id(struct acpi_subtable_header *entry,
  * Retrieve the ARM CPU physical identifier (MPIDR)
  */
 static int map_gicc_mpidr(struct acpi_subtable_header *entry,
-		int device_declaration, u32 acpi_id, phys_cpuid_t *mpidr,
-		bool ignore_disabled)
+		int device_declaration, u32 acpi_id, phys_cpuid_t *mpidr)
 {
 	struct acpi_madt_generic_interrupt *gicc =
 	    container_of(entry, struct acpi_madt_generic_interrupt, header);
 
-	if (ignore_disabled && !(gicc->flags & ACPI_MADT_ENABLED))
+	if (!(gicc->flags & ACPI_MADT_ENABLED))
 		return -ENODEV;
 
 	/* device_declaration means Device object in DSDT, in the
@@ -112,7 +109,7 @@ static int map_gicc_mpidr(struct acpi_subtable_header *entry,
 }
 
 static phys_cpuid_t map_madt_entry(struct acpi_table_madt *madt,
-				   int type, u32 acpi_id, bool ignore_disabled)
+				   int type, u32 acpi_id)
 {
 	unsigned long madt_end, entry;
 	phys_cpuid_t phys_id = PHYS_CPUID_INVALID;	/* CPU hardware ID */
@@ -130,20 +127,16 @@ static phys_cpuid_t map_madt_entry(struct acpi_table_madt *madt,
 		struct acpi_subtable_header *header =
 			(struct acpi_subtable_header *)entry;
 		if (header->type == ACPI_MADT_TYPE_LOCAL_APIC) {
-			if (!map_lapic_id(header, acpi_id, &phys_id,
-					  ignore_disabled))
+			if (!map_lapic_id(header, acpi_id, &phys_id))
 				break;
 		} else if (header->type == ACPI_MADT_TYPE_LOCAL_X2APIC) {
-			if (!map_x2apic_id(header, type, acpi_id, &phys_id,
-					   ignore_disabled))
+			if (!map_x2apic_id(header, type, acpi_id, &phys_id))
 				break;
 		} else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC) {
-			if (!map_lsapic_id(header, type, acpi_id, &phys_id,
-					   ignore_disabled))
+			if (!map_lsapic_id(header, type, acpi_id, &phys_id))
 				break;
 		} else if (header->type == ACPI_MADT_TYPE_GENERIC_INTERRUPT) {
-			if (!map_gicc_mpidr(header, type, acpi_id, &phys_id,
-					    ignore_disabled))
+			if (!map_gicc_mpidr(header, type, acpi_id, &phys_id))
 				break;
 		}
 		entry += header->length;
@@ -161,15 +154,14 @@ phys_cpuid_t __init acpi_map_madt_entry(u32 acpi_id)
 	if (!madt)
 		return PHYS_CPUID_INVALID;
 
-	rv = map_madt_entry(madt, 1, acpi_id, true);
+	rv = map_madt_entry(madt, 1, acpi_id);
 
 	acpi_put_table((struct acpi_table_header *)madt);
 
 	return rv;
 }
 
-static phys_cpuid_t map_mat_entry(acpi_handle handle, int type, u32 acpi_id,
-				  bool ignore_disabled)
+static phys_cpuid_t map_mat_entry(acpi_handle handle, int type, u32 acpi_id)
 {
 	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
 	union acpi_object *obj;
@@ -190,38 +182,30 @@ static phys_cpuid_t map_mat_entry(acpi_handle handle, int type, u32 acpi_id,
 
 	header = (struct acpi_subtable_header *)obj->buffer.pointer;
 	if (header->type == ACPI_MADT_TYPE_LOCAL_APIC)
-		map_lapic_id(header, acpi_id, &phys_id, ignore_disabled);
+		map_lapic_id(header, acpi_id, &phys_id);
 	else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC)
-		map_lsapic_id(header, type, acpi_id, &phys_id, ignore_disabled);
+		map_lsapic_id(header, type, acpi_id, &phys_id);
 	else if (header->type == ACPI_MADT_TYPE_LOCAL_X2APIC)
-		map_x2apic_id(header, type, acpi_id, &phys_id, ignore_disabled);
+		map_x2apic_id(header, type, acpi_id, &phys_id);
 	else if (header->type == ACPI_MADT_TYPE_GENERIC_INTERRUPT)
-		map_gicc_mpidr(header, type, acpi_id, &phys_id,
-			       ignore_disabled);
+		map_gicc_mpidr(header, type, acpi_id, &phys_id);
 
 exit:
 	kfree(buffer.pointer);
 	return phys_id;
 }
 
-static phys_cpuid_t __acpi_get_phys_id(acpi_handle handle, int type,
-				       u32 acpi_id, bool ignore_disabled)
+phys_cpuid_t acpi_get_phys_id(acpi_handle handle, int type, u32 acpi_id)
 {
 	phys_cpuid_t phys_id;
 
-	phys_id = map_mat_entry(handle, type, acpi_id, ignore_disabled);
+	phys_id = map_mat_entry(handle, type, acpi_id);
 	if (invalid_phys_cpuid(phys_id))
-		phys_id = map_madt_entry(get_madt_table(), type, acpi_id,
-					   ignore_disabled);
+		phys_id = map_madt_entry(get_madt_table(), type, acpi_id);
 
 	return phys_id;
 }
 
-phys_cpuid_t acpi_get_phys_id(acpi_handle handle, int type, u32 acpi_id)
-{
-	return __acpi_get_phys_id(handle, type, acpi_id, true);
-}
-
 int acpi_map_cpuid(phys_cpuid_t phys_id, u32 acpi_id)
 {
 #ifdef CONFIG_SMP
-- 
2.5.5

Re: [PATCH v2 2/4] Revert"x86/acpi: Enable MADT APIs to return disabled apicids"

[Thomas Gleixner]

On Mon, 20 Feb 2017, Dou Liyang wrote:

> After we never do the last mapping of "cpuid <-> nodeid" at booting time. we
> also no need to enable MADT APIs to return disabled apicid.
> 
> So, The patch work for reverting the commit 8ad893faf2:
> "x86/acpi: Enable MADT APIs to return disabled apicids"

Again, this changelog is confusing. A simple:

  Remove the leftovers of the boot time 'cpuid <-> nodeid' mapping approach.

would be sufficient and entirely clear.

Thanks,

	tglx

Re: [PATCH v2 2/4] Revert"x86/acpi: Enable MADT APIs to return disabled apicids"

[Dou Liyang]

Hi tglx,

At 03/01/2017 06:52 PM, Thomas Gleixner wrote:
> On Mon, 20 Feb 2017, Dou Liyang wrote:
>
>> After we never do the last mapping of "cpuid <-> nodeid" at booting time. we
>> also no need to enable MADT APIs to return disabled apicid.
>>
>> So, The patch work for reverting the commit 8ad893faf2:
>> "x86/acpi: Enable MADT APIs to return disabled apicids"
>
> Again, this changelog is confusing. A simple:
>
>   Remove the leftovers of the boot time 'cpuid <-> nodeid' mapping approach.
>
> would be sufficient and entirely clear.
>

Yes, I see, I will rewrite it in next version.

Thanks.

	Liyang.

> Thanks,
>
> 	tglx
>
>
>
>

[PATCH v2 3/4] acpi: Fix the check handle in case of declaring processors using the Device operator

[Dou Liyang]

In ACPI spec, we can declare processors using both Processor and
Device operator. And before we use the ACPI table, we should check
the correctness for all processors in ACPI namespace.

But, Currently, the check handle is just include only the processors
which are declared by Processor operator. It misses the processors
declared by Device operator.

The patch adds the case of Device operator.

Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
---
 drivers/acpi/acpi_processor.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index f43a586..eb500e1 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -633,25 +633,43 @@ static acpi_status __init acpi_processor_ids_walk(acpi_handle handle,
 						  void **rv)
 {
 	acpi_status status;
+	acpi_object_type acpi_type;
+	unsigned long long uid;
 	union acpi_object object = { 0 };
 	struct acpi_buffer buffer = { sizeof(union acpi_object), &object };
 
-	status = acpi_evaluate_object(handle, NULL, NULL, &buffer);
-	if (ACPI_FAILURE(status))
-		acpi_handle_info(handle, "Not get the processor object\n");
-	else
-		processor_validated_ids_update(object.processor.proc_id);
+	status = acpi_get_type(handle, &acpi_type);
+	switch (acpi_type) {
+	case ACPI_TYPE_PROCESSOR:
+		status = acpi_evaluate_object(handle, NULL, NULL, &buffer);
+		if (ACPI_FAILURE(status))
+			acpi_handle_info(handle, "Not get the processor object\n");
+		else
+			processor_validated_ids_update(
+						object.processor.proc_id);
+		break;
+	case ACPI_TYPE_DEVICE:
+		status = acpi_evaluate_integer(handle, "_UID", NULL, &uid);
+		if (ACPI_FAILURE(status))
+			return false;
+		processor_validated_ids_update(uid);
+		break;
+	default:
+		return false;
+	}
 
 	return AE_OK;
 }
 
-static void __init acpi_processor_check_duplicates(void)
+void __init acpi_processor_check_duplicates(void)
 {
-	/* Search all processor nodes in ACPI namespace */
+	/* check the correctness for all processors in ACPI namespace */
 	acpi_walk_namespace(ACPI_TYPE_PROCESSOR, ACPI_ROOT_OBJECT,
 						ACPI_UINT32_MAX,
 						acpi_processor_ids_walk,
 						NULL, NULL, NULL);
+	acpi_get_devices(ACPI_PROCESSOR_DEVICE_HID, acpi_processor_ids_walk,
+						NULL, NULL);
 }
 
 bool __init acpi_processor_validate_proc_id(int proc_id)
-- 
2.5.5

Re: [PATCH v2 3/4] acpi: Fix the check handle in case of declaring processors using the Device operator

[Thomas Gleixner]

On Mon, 20 Feb 2017, Dou Liyang wrote:

> In ACPI spec, we can declare processors using both Processor and
> Device operator. And before we use the ACPI table, we should check
> the correctness for all processors in ACPI namespace.
> 
> But, Currently, the check handle is just include only the processors
> which are declared by Processor operator. It misses the processors
> declared by Device operator.
> 
> The patch adds the case of Device operator.

See the comments in the previous mails. They apply here as well.

Though this changelog is actively confusing. The subject line says:

  acpi: Fix the check handle in case of declaring processors using the Device
  	operator

Aside of being a way too long subject, it suggests that there is just a
missing check for the case where a processor is declared via the Device
operator. But that's not what the patch is doing.

It implements the distinction between Device and Processor operator, which
is missing in acpi_processor_ids_walk() right now.

So the proper changelog (if I understand the patch correctly) would be:

Subject: acpi/processor: Implement DEVICE operator for processor enumeration

  ACPI allows to declare processors either with the PROCESSOR or with the
  DEVICE operator. The current implementation handles only the PROCESSOR
  operator.

  On a system which uses the DEVICE operator for processor enumeration the
  evaluation fails.

  Check for the ACPI type of the ACPI handle and evaluate PROCESSOR and
  DEVICE types seperately.

Hmm?

>  {
>  	acpi_status status;
> +	acpi_object_type acpi_type;
> +	unsigned long long uid;
>  	union acpi_object object = { 0 };
>  	struct acpi_buffer buffer = { sizeof(union acpi_object), &object };
>  
> -	status = acpi_evaluate_object(handle, NULL, NULL, &buffer);
> -	if (ACPI_FAILURE(status))
> -		acpi_handle_info(handle, "Not get the processor object\n");
> -	else
> -		processor_validated_ids_update(object.processor.proc_id);
> +	status = acpi_get_type(handle, &acpi_type);

Shouldn't the status be checked here?

> +	switch (acpi_type) {
> +	case ACPI_TYPE_PROCESSOR:
> +		status = acpi_evaluate_object(handle, NULL, NULL, &buffer);
> +		if (ACPI_FAILURE(status))
> +			acpi_handle_info(handle, "Not get the processor object\n");
> +		else
> +			processor_validated_ids_update(
> +						object.processor.proc_id);
> +		break;
> +	case ACPI_TYPE_DEVICE:
> +		status = acpi_evaluate_integer(handle, "_UID", NULL, &uid);
> +		if (ACPI_FAILURE(status))
> +			return false;
> +		processor_validated_ids_update(uid);
> +		break;
> +	default:
> +		return false;

This is inconsistent vs. the failure handling in the PROCESSOR and DEVICE
case and the default case does not give any information either.

What about this:

	switch (acpi_type) {
	case ACPI_TYPE_PROCESSOR:
		status = acpi_evaluate_object(handle, NULL, NULL, &buffer);
		if (ACPI_FAILURE(status))
			goto err;
		uid = object.processor.proc_id;
		break;
		
	case ACPI_TYPE_DEVICE:
		status = acpi_evaluate_integer(handle, "_UID", NULL, &uid);
		if (ACPI_FAILURE(status))
			goto err;
		break;
	default:
		goto err;
	}

	processor_validated_ids_update(uid);
	return true;

err:
	acpi_handle_info(handle, "Invalid processor object\n");
	return false;
}

Thanks,

	tglx

Re: [PATCH v2 3/4] acpi: Fix the check handle in case of declaring processors using the Device operator

[Dou Liyang]

Hi tglx,

At 03/01/2017 07:12 PM, Thomas Gleixner wrote:
> On Mon, 20 Feb 2017, Dou Liyang wrote:
>
>> In ACPI spec, we can declare processors using both Processor and
>> Device operator. And before we use the ACPI table, we should check
>> the correctness for all processors in ACPI namespace.
>>
>> But, Currently, the check handle is just include only the processors
>> which are declared by Processor operator. It misses the processors
>> declared by Device operator.
>>
>> The patch adds the case of Device operator.
>
> See the comments in the previous mails. They apply here as well.
>
> Though this changelog is actively confusing. The subject line says:
>
>   acpi: Fix the check handle in case of declaring processors using the Device
>   	operator
>
> Aside of being a way too long subject, it suggests that there is just a
> missing check for the case where a processor is declared via the Device
> operator. But that's not what the patch is doing.
>
> It implements the distinction between Device and Processor operator, which
> is missing in acpi_processor_ids_walk() right now.
>
> So the proper changelog (if I understand the patch correctly) would be:
>
> Subject: acpi/processor: Implement DEVICE operator for processor enumeration
>
>   ACPI allows to declare processors either with the PROCESSOR or with the
>   DEVICE operator. The current implementation handles only the PROCESSOR
>   operator.
>
>   On a system which uses the DEVICE operator for processor enumeration the
>   evaluation fails.
>
>   Check for the ACPI type of the ACPI handle and evaluate PROCESSOR and
>   DEVICE types seperately.
>
> Hmm?
>

Yes, you are right. I didn't explain clearly.
I will modify in my next version.

>>  {
>>  	acpi_status status;
>> +	acpi_object_type acpi_type;
>> +	unsigned long long uid;
>>  	union acpi_object object = { 0 };
>>  	struct acpi_buffer buffer = { sizeof(union acpi_object), &object };
>>
>> -	status = acpi_evaluate_object(handle, NULL, NULL, &buffer);
>> -	if (ACPI_FAILURE(status))
>> -		acpi_handle_info(handle, "Not get the processor object\n");
>> -	else
>> -		processor_validated_ids_update(object.processor.proc_id);
>> +	status = acpi_get_type(handle, &acpi_type);
>
> Shouldn't the status be checked here?

oops, Yes. Need to be checked.

>
>> +	switch (acpi_type) {
>> +	case ACPI_TYPE_PROCESSOR:
>> +		status = acpi_evaluate_object(handle, NULL, NULL, &buffer);
>> +		if (ACPI_FAILURE(status))
>> +			acpi_handle_info(handle, "Not get the processor object\n");
>> +		else
>> +			processor_validated_ids_update(
>> +						object.processor.proc_id);
>> +		break;
>> +	case ACPI_TYPE_DEVICE:
>> +		status = acpi_evaluate_integer(handle, "_UID", NULL, &uid);
>> +		if (ACPI_FAILURE(status))
>> +			return false;
>> +		processor_validated_ids_update(uid);
>> +		break;
>> +	default:
>> +		return false;
>
> This is inconsistent vs. the failure handling in the PROCESSOR and DEVICE
> case and the default case does not give any information either.
>
> What about this:
>
> 	switch (acpi_type) {
> 	case ACPI_TYPE_PROCESSOR:
> 		status = acpi_evaluate_object(handle, NULL, NULL, &buffer);
> 		if (ACPI_FAILURE(status))
> 			goto err;
> 		uid = object.processor.proc_id;
> 		break;
> 		
> 	case ACPI_TYPE_DEVICE:
> 		status = acpi_evaluate_integer(handle, "_UID", NULL, &uid);
> 		if (ACPI_FAILURE(status))
> 			goto err;
> 		break;
> 	default:
> 		goto err;
> 	}
>
> 	processor_validated_ids_update(uid);
> 	return true;
>
> err:
> 	acpi_handle_info(handle, "Invalid processor object\n");
> 	return false;
> }
>

Looks good than mine.

Thanks,
	Liyang.

> Thanks,
>
> 	tglx
>
>
>

[PATCH v2 4/4] acpi: Move the verification of duplicate proc_id from booting time to hot-plug time

[Dou Liyang]

After we revert the the mapping of "cpuid <-> nodeid" fixed at the
booting time. and do it at the hot-plug time. we should also do the
verification of duplicate proc_id at the time.

The patch rename the verfication function and move it to
drivers/acpi::acpi_processor_get_info.

Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
---
 drivers/acpi/acpi_processor.c | 13 ++++++++++---
 include/linux/acpi.h          |  2 +-
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index eb500e1..2483383 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -280,6 +280,13 @@ static int acpi_processor_get_info(struct acpi_device *device)
 		pr->acpi_id = value;
 	}
 
+	if(duplicate_processor_id(pr->acpi_id)) {
+		dev_err(&device->dev,
+			"Failed to get unique processor _UID (0x%x)\n",
+			pr->acpi_id);
+		return -ENODEV;
+	}
+
 	pr->phys_id = acpi_get_phys_id(pr->handle, device_declaration,
 					pr->acpi_id);
 	if (invalid_phys_cpuid(pr->phys_id))
@@ -580,7 +587,7 @@ static struct acpi_scan_handler processor_container_handler = {
 static int nr_unique_ids __initdata;
 
 /* The number of the duplicate processor IDs */
-static int nr_duplicate_ids __initdata;
+static int nr_duplicate_ids;
 
 /* Used to store the unique processor IDs */
 static int unique_processor_ids[] __initdata = {
@@ -588,7 +595,7 @@ static int unique_processor_ids[] __initdata = {
 };
 
 /* Used to store the duplicate processor IDs */
-static int duplicate_processor_ids[] __initdata = {
+static int duplicate_processor_ids[] = {
 	[0 ... NR_CPUS - 1] = -1,
 };
 
@@ -672,7 +679,7 @@ void __init acpi_processor_check_duplicates(void)
 						NULL, NULL);
 }
 
-bool __init acpi_processor_validate_proc_id(int proc_id)
+bool duplicate_processor_id(int proc_id)
 {
 	int i;
 
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index d180cbd..b692a70 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -287,7 +287,7 @@ static inline bool invalid_phys_cpuid(phys_cpuid_t phys_id)
 }
 
 /* Validate the processor object's proc_id */
-bool acpi_processor_validate_proc_id(int proc_id);
+bool duplicate_processor_id(int proc_id);
 
 #ifdef CONFIG_ACPI_HOTPLUG_CPU
 /* Arch dependent functions for cpu hotplug support */
-- 
2.5.5

Re: [PATCH v2 4/4] acpi: Move the verification of duplicate proc_id from booting time to hot-plug time

[Thomas Gleixner]

On Mon, 20 Feb 2017, Dou Liyang wrote:

Please make your subject line short and a precise summary phrase, not an
overlong sentence.

> After we revert the the mapping of "cpuid <-> nodeid" fixed at the
> booting time. and do it at the hot-plug time. we should also do the
> verification of duplicate proc_id at the time.

The revert is completely irrelevant to this change, really. The reference
is just confusing.

> The patch rename the verfication function and move it to
> drivers/acpi::acpi_processor_get_info.

See previous mails ....

Let me give you another changelog example:

Subject: acpi/processor: Check for duplicate processor ids at hotplug time

  The check for duplicate processor ids happens at boot time based on the
  ACPI table contents, but the final sanity checks for a processor happen
  at hotplug time.

  At hotplug time, where the physical information is available, which might
  differ from the ACPI table information, a check for duplicate processor
  ids is missing.

  Add it to the hotplug checks and rename the function so it better
  reflects its purpose.

Hmm?

>  
> -bool __init acpi_processor_validate_proc_id(int proc_id)
> +bool duplicate_processor_id(int proc_id)

Please keep the acpi_ prefix. acpi_duplicate_processor_id().

Thanks,

	tglx

Re: [PATCH v2 4/4] acpi: Move the verification of duplicate proc_id from booting time to hot-plug time

[Dou Liyang]

Hi tglx,

At 03/01/2017 07:26 PM, Thomas Gleixner wrote:
> On Mon, 20 Feb 2017, Dou Liyang wrote:
>
> Please make your subject line short and a precise summary phrase, not an
> overlong sentence.
>
>> After we revert the the mapping of "cpuid <-> nodeid" fixed at the
>> booting time. and do it at the hot-plug time. we should also do the
>> verification of duplicate proc_id at the time.
>
> The revert is completely irrelevant to this change, really. The reference
> is just confusing.
>

Yes, Maybe I should split them like before.


>> The patch rename the verfication function and move it to
>> drivers/acpi::acpi_processor_get_info.
>
> See previous mails ....
>
> Let me give you another changelog example:
>

Thanks again.

> Subject: acpi/processor: Check for duplicate processor ids at hotplug time
>
>   The check for duplicate processor ids happens at boot time based on the
>   ACPI table contents, but the final sanity checks for a processor happen
>   at hotplug time.
>
>   At hotplug time, where the physical information is available, which might
>   differ from the ACPI table information, a check for duplicate processor
>   ids is missing.
>
>   Add it to the hotplug checks and rename the function so it better
>   reflects its purpose.
>
> Hmm?

Yes, thanks again. I learned a lot in that patchset.

>
>>
>> -bool __init acpi_processor_validate_proc_id(int proc_id)
>> +bool duplicate_processor_id(int proc_id)
>
> Please keep the acpi_ prefix. acpi_duplicate_processor_id().

OK, I will.

Thanks,

	Liyang.
>
> Thanks,
>
> 	tglx
>
>
>

Re: [PATCH v2 0/4] Revert works for the mapping of cpuid <-> nodeid

[Ye Xiaolong]

On 02/20, Dou Liyang wrote:
>Currently, We make the mapping of "cpuid <-> nodeid" fixed at the booting time.
>It keeps consistent with the WorkQueue and avoids some bugs which may be caused
>by the dynamic assignment.
>As we know, It is implemented by the patches as follows: 2532fc318d, f7c28833c2,
>8f54969dc8, 8ad893faf2, dc6db24d24, which depend on ACPI table. Simply speaking:
>
>Step 1. Make the "Logical CPU ID <-> Processor ID/UID" fixed Using MADT:
>We generate the logical CPU IDs by the Local APIC/x2APIC IDs orderly and
>get the mapping of Processor ID/UID <-> Local Apic ID directly in MADT.
>So, we get the mapping of
>*Processor ID/UID <-> Local Apic ID <-> Logical CPU ID*
>
>Step 2. Make the "Processor ID/UID <-> Node ID(_PXM)" fixed Using DSDT:
>The maaping of "Processor ID/UID <-> Node ID(_PXM)" is ready-made in
>each entities. we just use it directly.
>
>So, at last we get the maaping of *Node ID <-> Logical CPU ID* according to
>step1 and step2:
>*Node ID(_PXM) <-> Processor ID/UID <-> Local Apic ID <-> Logical CPU ID*
>
>But, The ACPI table is unreliable and it is very risky that we use the entity
>which isn't related to a physical device at booting time. Here has already two
>bugs we found.
>1. Duplicated Processor IDs in DSDT.
>	It has been fixed by commit 8e089eaa19, fd74da217d.
>2. The _PXM in DSDT is inconsistent with the one in MADT.
>	It may cause the bug, which is shown in:
>		https://lkml.org/lkml/2017/2/12/200
>There may be more later. We shouldn't just only fix them everytime, we should
>solve this problem from the source to avoid such problems happend again and
>again.
>
>Now, a simple and easy way is found, we revert our patches. Do the Step 2 
>at hot-plug time, not at booting time where we did some useless work.
>
>It also can make the mapping of "cpuid <-> nodeid" fixed and avoid excessive
>use of the ACPI table.
>
>We have tested them in our box: Fujitsu PQ2000 with 2 nodes for hot-plug.
>To Xiaolong: 
>	Please help me to test it in the special machine.

Got it, I'll queue the tests on the previous machine and let you know the result
once I get it.

Thanks,
Xiaolong
>
>Change log:
>  v1 -> v2: 1. fix some comments.
>            2. add the verification of duplicate processor id.
>
>Dou Liyang (4):
>  Revert"x86/acpi: Set persistent cpuid <-> nodeid mapping when booting"
>  Revert"x86/acpi: Enable MADT APIs to return disabled apicids"
>  acpi: Fix the check handle in case of declaring processors using the
>    Device operator
>  acpi: Move the verification of duplicate proc_id from booting time to
>    hot-plug time
>
> arch/x86/kernel/acpi/boot.c   |   2 +-
> drivers/acpi/acpi_processor.c |  50 +++++++++++-----
> drivers/acpi/bus.c            |   1 -
> drivers/acpi/processor_core.c | 133 +++++++-----------------------------------
> include/linux/acpi.h          |   5 +-
> 5 files changed, 59 insertions(+), 132 deletions(-)
>
>-- 
>2.5.5
>
>
>

Re: [PATCH v2 0/4] Revert works for the mapping of cpuid <-> nodeid

[Ye Xiaolong]

On 02/21, Ye Xiaolong wrote:
>On 02/20, Dou Liyang wrote:
>>Currently, We make the mapping of "cpuid <-> nodeid" fixed at the booting time.
>>It keeps consistent with the WorkQueue and avoids some bugs which may be caused
>>by the dynamic assignment.
>>As we know, It is implemented by the patches as follows: 2532fc318d, f7c28833c2,
>>8f54969dc8, 8ad893faf2, dc6db24d24, which depend on ACPI table. Simply speaking:
>>
>>Step 1. Make the "Logical CPU ID <-> Processor ID/UID" fixed Using MADT:
>>We generate the logical CPU IDs by the Local APIC/x2APIC IDs orderly and
>>get the mapping of Processor ID/UID <-> Local Apic ID directly in MADT.
>>So, we get the mapping of
>>*Processor ID/UID <-> Local Apic ID <-> Logical CPU ID*
>>
>>Step 2. Make the "Processor ID/UID <-> Node ID(_PXM)" fixed Using DSDT:
>>The maaping of "Processor ID/UID <-> Node ID(_PXM)" is ready-made in
>>each entities. we just use it directly.
>>
>>So, at last we get the maaping of *Node ID <-> Logical CPU ID* according to
>>step1 and step2:
>>*Node ID(_PXM) <-> Processor ID/UID <-> Local Apic ID <-> Logical CPU ID*
>>
>>But, The ACPI table is unreliable and it is very risky that we use the entity
>>which isn't related to a physical device at booting time. Here has already two
>>bugs we found.
>>1. Duplicated Processor IDs in DSDT.
>>	It has been fixed by commit 8e089eaa19, fd74da217d.
>>2. The _PXM in DSDT is inconsistent with the one in MADT.
>>	It may cause the bug, which is shown in:
>>		https://lkml.org/lkml/2017/2/12/200
>>There may be more later. We shouldn't just only fix them everytime, we should
>>solve this problem from the source to avoid such problems happend again and
>>again.
>>
>>Now, a simple and easy way is found, we revert our patches. Do the Step 2 
>>at hot-plug time, not at booting time where we did some useless work.
>>
>>It also can make the mapping of "cpuid <-> nodeid" fixed and avoid excessive
>>use of the ACPI table.
>>
>>We have tested them in our box: Fujitsu PQ2000 with 2 nodes for hot-plug.
>>To Xiaolong: 
>>	Please help me to test it in the special machine.
>
>Got it, I'll queue the tests on the previous machine and let you know the result
>once I get it.

Previous kernel panic and incomplete run issue (described in [1]) in 0day
system is gone with this series.

Tested-by: Xiaolong Ye <xiaolong.ye@intel.com>

Here is the comparison:

$ compare -at dc6db24d2476cd09c0ecf2b8d80313539f737a89 2e61bac54fad4c018afd23c118bce2399e504020
tests: 1
testcase/path_params/tbox_group/run: vm-scalability/300-never-never-1-1-swap-w-rand-performance/lkp-hsw-ep2

Here dc6db24d24 is previous first bad commit, 2e61bac54 is the head commit of your series
applied on top of latest tip of linus/master c945d0227d ("Merge branch 'x86-platform-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip")

dc6db24d2476cd09  2e61bac54fad4c018afd23c118  
----------------  --------------------------  
       fail:runs  %reproduction    fail:runs
           |             |             |    
           :12          12%           1:8     last_state.OOM
           :12          12%           1:8     dmesg.page_allocation_failure:order:#,mode:#(GFP_USER|GFP_DMA32|__GFP_ZERO)
           :12          12%           1:8     dmesg.Mem-Info
         12:12        -100%            :8     dmesg.BUG:unable_to_handle_kernel
         12:12        -100%            :8     dmesg.Oops
         12:12        -100%            :8     dmesg.RIP:get_partial_node
          9:12         -75%            :8     dmesg.RIP:_raw_spin_lock_irqsave
          3:12         -25%            :8     dmesg.general_protection_fault:#[##]SMP
          3:12         -25%            :8     dmesg.RIP:native_queued_spin_lock_slowpath
          3:12         -25%            :8     dmesg.Kernel_panic-not_syncing:Hard_LOCKUP
          2:12         -17%            :8     dmesg.RIP:load_balance
          2:12         -17%            :8     dmesg.Kernel_panic-not_syncing:Fatal_exception_in_interrupt
          1:12          -8%            :8     dmesg.RIP:resched_curr
          1:12          -8%            :8     dmesg.Kernel_panic-not_syncing:Fatal_exception
          5:12         -42%            :8     dmesg.WARNING:at_include/linux/uaccess.h:#__probe_kernel_read
          1:12          -8%            :8     dmesg.WARNING:at_lib/list_debug.c:#__list_add


[1] https://lkml.org/lkml/2017/2/12/200

Thanks,
Xiaolong

>
>Thanks,
>Xiaolong
>>
>>Change log:
>>  v1 -> v2: 1. fix some comments.
>>            2. add the verification of duplicate processor id.
>>
>>Dou Liyang (4):
>>  Revert"x86/acpi: Set persistent cpuid <-> nodeid mapping when booting"
>>  Revert"x86/acpi: Enable MADT APIs to return disabled apicids"
>>  acpi: Fix the check handle in case of declaring processors using the
>>    Device operator
>>  acpi: Move the verification of duplicate proc_id from booting time to
>>    hot-plug time
>>
>> arch/x86/kernel/acpi/boot.c   |   2 +-
>> drivers/acpi/acpi_processor.c |  50 +++++++++++-----
>> drivers/acpi/bus.c            |   1 -
>> drivers/acpi/processor_core.c | 133 +++++++-----------------------------------
>> include/linux/acpi.h          |   5 +-
>> 5 files changed, 59 insertions(+), 132 deletions(-)
>>
>>-- 
>>2.5.5
>>
>>
>>

Re: [PATCH v2 0/4] Revert works for the mapping of cpuid <-> nodeid

[Dou Liyang]

Hi, Xiaolong

At 02/21/2017 03:10 PM, Ye Xiaolong wrote:
> On 02/21, Ye Xiaolong wrote:
>> On 02/20, Dou Liyang wrote:
>>> Currently, We make the mapping of "cpuid <-> nodeid" fixed at the booting time.
>>> It keeps consistent with the WorkQueue and avoids some bugs which may be caused
>>> by the dynamic assignment.
>>> As we know, It is implemented by the patches as follows: 2532fc318d, f7c28833c2,
>>> 8f54969dc8, 8ad893faf2, dc6db24d24, which depend on ACPI table. Simply speaking:
>>>
>>> Step 1. Make the "Logical CPU ID <-> Processor ID/UID" fixed Using MADT:
>>> We generate the logical CPU IDs by the Local APIC/x2APIC IDs orderly and
>>> get the mapping of Processor ID/UID <-> Local Apic ID directly in MADT.
>>> So, we get the mapping of
>>> *Processor ID/UID <-> Local Apic ID <-> Logical CPU ID*
>>>
>>> Step 2. Make the "Processor ID/UID <-> Node ID(_PXM)" fixed Using DSDT:
>>> The maaping of "Processor ID/UID <-> Node ID(_PXM)" is ready-made in
>>> each entities. we just use it directly.
>>>
>>> So, at last we get the maaping of *Node ID <-> Logical CPU ID* according to
>>> step1 and step2:
>>> *Node ID(_PXM) <-> Processor ID/UID <-> Local Apic ID <-> Logical CPU ID*
>>>
>>> But, The ACPI table is unreliable and it is very risky that we use the entity
>>> which isn't related to a physical device at booting time. Here has already two
>>> bugs we found.
>>> 1. Duplicated Processor IDs in DSDT.
>>> 	It has been fixed by commit 8e089eaa19, fd74da217d.
>>> 2. The _PXM in DSDT is inconsistent with the one in MADT.
>>> 	It may cause the bug, which is shown in:
>>> 		https://lkml.org/lkml/2017/2/12/200
>>> There may be more later. We shouldn't just only fix them everytime, we should
>>> solve this problem from the source to avoid such problems happend again and
>>> again.
>>>
>>> Now, a simple and easy way is found, we revert our patches. Do the Step 2
>>> at hot-plug time, not at booting time where we did some useless work.
>>>
>>> It also can make the mapping of "cpuid <-> nodeid" fixed and avoid excessive
>>> use of the ACPI table.
>>>
>>> We have tested them in our box: Fujitsu PQ2000 with 2 nodes for hot-plug.
>>> To Xiaolong:
>>> 	Please help me to test it in the special machine.
>>
>> Got it, I'll queue the tests on the previous machine and let you know the result
>> once I get it.
>
> Previous kernel panic and incomplete run issue (described in [1]) in 0day
> system is gone with this series.
>

Thanks very much, I am glad to hear that!

> Tested-by: Xiaolong Ye <xiaolong.ye@intel.com>
>

I will add it in my next version.

Thanks,
Liyang

> Here is the comparison:
>
> $ compare -at dc6db24d2476cd09c0ecf2b8d80313539f737a89 2e61bac54fad4c018afd23c118bce2399e504020
> tests: 1
> testcase/path_params/tbox_group/run: vm-scalability/300-never-never-1-1-swap-w-rand-performance/lkp-hsw-ep2
>
> Here dc6db24d24 is previous first bad commit, 2e61bac54 is the head commit of your series
> applied on top of latest tip of linus/master c945d0227d ("Merge branch 'x86-platform-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip")
>
> dc6db24d2476cd09  2e61bac54fad4c018afd23c118
> ----------------  --------------------------
>        fail:runs  %reproduction    fail:runs
>            |             |             |
>            :12          12%           1:8     last_state.OOM
>            :12          12%           1:8     dmesg.page_allocation_failure:order:#,mode:#(GFP_USER|GFP_DMA32|__GFP_ZERO)
>            :12          12%           1:8     dmesg.Mem-Info
>          12:12        -100%            :8     dmesg.BUG:unable_to_handle_kernel
>          12:12        -100%            :8     dmesg.Oops
>          12:12        -100%            :8     dmesg.RIP:get_partial_node
>           9:12         -75%            :8     dmesg.RIP:_raw_spin_lock_irqsave
>           3:12         -25%            :8     dmesg.general_protection_fault:#[##]SMP
>           3:12         -25%            :8     dmesg.RIP:native_queued_spin_lock_slowpath
>           3:12         -25%            :8     dmesg.Kernel_panic-not_syncing:Hard_LOCKUP
>           2:12         -17%            :8     dmesg.RIP:load_balance
>           2:12         -17%            :8     dmesg.Kernel_panic-not_syncing:Fatal_exception_in_interrupt
>           1:12          -8%            :8     dmesg.RIP:resched_curr
>           1:12          -8%            :8     dmesg.Kernel_panic-not_syncing:Fatal_exception
>           5:12         -42%            :8     dmesg.WARNING:at_include/linux/uaccess.h:#__probe_kernel_read
>           1:12          -8%            :8     dmesg.WARNING:at_lib/list_debug.c:#__list_add
>
>
> [1] https://lkml.org/lkml/2017/2/12/200
>
> Thanks,
> Xiaolong
>
>>
>> Thanks,
>> Xiaolong
>>>
>>> Change log:
>>>  v1 -> v2: 1. fix some comments.
>>>            2. add the verification of duplicate processor id.
>>>
>>> Dou Liyang (4):
>>>  Revert"x86/acpi: Set persistent cpuid <-> nodeid mapping when booting"
>>>  Revert"x86/acpi: Enable MADT APIs to return disabled apicids"
>>>  acpi: Fix the check handle in case of declaring processors using the
>>>    Device operator
>>>  acpi: Move the verification of duplicate proc_id from booting time to
>>>    hot-plug time
>>>
>>> arch/x86/kernel/acpi/boot.c   |   2 +-
>>> drivers/acpi/acpi_processor.c |  50 +++++++++++-----
>>> drivers/acpi/bus.c            |   1 -
>>> drivers/acpi/processor_core.c | 133 +++++++-----------------------------------
>>> include/linux/acpi.h          |   5 +-
>>> 5 files changed, 59 insertions(+), 132 deletions(-)
>>>
>>> --
>>> 2.5.5
>>>
>>>
>>>
>
>
>

Re: [LKP] [PATCH v2 0/4] Revert works for the mapping of cpuid <-> nodeid

[Aaron Lu]

On Wed, Feb 22, 2017 at 09:56:51AM +0800, Dou Liyang wrote:
> Hi, Xiaolong
> 
> At 02/21/2017 03:10 PM, Ye Xiaolong wrote:
> > On 02/21, Ye Xiaolong wrote:
> > > On 02/20, Dou Liyang wrote:
> > > > Currently, We make the mapping of "cpuid <-> nodeid" fixed at the booting time.
> > > > It keeps consistent with the WorkQueue and avoids some bugs which may be caused
> > > > by the dynamic assignment.
> > > > As we know, It is implemented by the patches as follows: 2532fc318d, f7c28833c2,
> > > > 8f54969dc8, 8ad893faf2, dc6db24d24, which depend on ACPI table. Simply speaking:
> > > > 
> > > > Step 1. Make the "Logical CPU ID <-> Processor ID/UID" fixed Using MADT:
> > > > We generate the logical CPU IDs by the Local APIC/x2APIC IDs orderly and
> > > > get the mapping of Processor ID/UID <-> Local Apic ID directly in MADT.
> > > > So, we get the mapping of
> > > > *Processor ID/UID <-> Local Apic ID <-> Logical CPU ID*
> > > > 
> > > > Step 2. Make the "Processor ID/UID <-> Node ID(_PXM)" fixed Using DSDT:
> > > > The maaping of "Processor ID/UID <-> Node ID(_PXM)" is ready-made in
> > > > each entities. we just use it directly.
> > > > 
> > > > So, at last we get the maaping of *Node ID <-> Logical CPU ID* according to
> > > > step1 and step2:
> > > > *Node ID(_PXM) <-> Processor ID/UID <-> Local Apic ID <-> Logical CPU ID*
> > > > 
> > > > But, The ACPI table is unreliable and it is very risky that we use the entity
> > > > which isn't related to a physical device at booting time. Here has already two
> > > > bugs we found.
> > > > 1. Duplicated Processor IDs in DSDT.
> > > > 	It has been fixed by commit 8e089eaa19, fd74da217d.
> > > > 2. The _PXM in DSDT is inconsistent with the one in MADT.
> > > > 	It may cause the bug, which is shown in:
> > > > 		https://lkml.org/lkml/2017/2/12/200
> > > > There may be more later. We shouldn't just only fix them everytime, we should
> > > > solve this problem from the source to avoid such problems happend again and
> > > > again.
> > > > 
> > > > Now, a simple and easy way is found, we revert our patches. Do the Step 2
> > > > at hot-plug time, not at booting time where we did some useless work.
> > > > 
> > > > It also can make the mapping of "cpuid <-> nodeid" fixed and avoid excessive
> > > > use of the ACPI table.
> > > > 
> > > > We have tested them in our box: Fujitsu PQ2000 with 2 nodes for hot-plug.
> > > > To Xiaolong:
> > > > 	Please help me to test it in the special machine.
> > > 
> > > Got it, I'll queue the tests on the previous machine and let you know the result
> > > once I get it.
> > 
> > Previous kernel panic and incomplete run issue (described in [1]) in 0day
> > system is gone with this series.
> > 
> 
> Thanks very much, I am glad to hear that!
> 
> > Tested-by: Xiaolong Ye <xiaolong.ye@intel.com>
> > 
> 
> I will add it in my next version.

What is the status of the patch?

I still get oops during boot on a EP machine with today's Linus tree's
head commit 69eea5a4ab9c("Merge branch 'for-linus' of git://git.kernel.dk/linux-block")

The first oops call trace:

... ...
[    8.599850] pci_bus 0000:80: on NUMA node 2
[    8.605611] ACPI: Enabled 4 GPEs in block 00 to 3F
[    8.645521] BUG: unable to handle kernel paging request at 000000000001f768
[    8.653585] IP: get_partial_node+0x2c/0x1f0
[    8.659302] PGD 0 
[    8.659303] 
[    8.663724] Oops: 0000 [#1] SMP
[    8.667499] Modules linked in:
[    8.671181] CPU: 60 PID: 1 Comm: swapper/0 Not tainted 4.11.0-rc1 #1
[    8.678554] Hardware name: Intel Corporation S2600WTT/S2600WTT, BIOS SE5C610.86B.01.01.0008.021120151325 02/11/2015
[    8.690672] task: ffff88202bc10000 task.stack: ffffc9000002c000
[    8.697542] RIP: 0010:get_partial_node+0x2c/0x1f0
[    8.703844] RSP: 0000:ffffc9000002fb20 EFLAGS: 00010006
[    8.709944] RAX: 0000000000000002 RBX: 0000000000000000 RCX: 00000000014080c0
[    8.718184] RDX: ffff88203281f740 RSI: 000000000001f760 RDI: ffff88202e548280
[    8.726422] RBP: ffffc9000002fbc0 R08: 0000000000000000 R09: 0000000100220022
[    8.734661] R10: ffffea0080a99600 R11: 0000000000000000 R12: ffff88202e548280
[    8.742896] R13: ffffea0080a991c0 R14: ffff88202e548280 R15: ffff88203281f730
[    8.751144] FS:  0000000000000000(0000) GS:ffff882032800000(0000) knlGS:0000000000000000
[    8.760633] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    8.767312] CR2: 000000000001f768 CR3: 0000000001e09000 CR4: 00000000001406e0
[    8.775550] Call Trace:
[    8.778548]  ? acpi_os_release_lock+0xe/0x10
[    8.783590]  ? acpi_ut_update_ref_count+0x5a/0x6b3
[    8.789210]  ___slab_alloc+0x28a/0x4b0
[    8.793660]  ? __kernfs_new_node+0x41/0xc0
[    8.798505]  ? __kernfs_new_node+0x41/0xc0
[    8.803348]  __slab_alloc+0x20/0x40
[    8.807501]  kmem_cache_alloc+0x17f/0x1c0
[    8.812231]  __kernfs_new_node+0x41/0xc0
[    8.816882]  kernfs_new_node+0x26/0x50
[    8.821338]  __kernfs_create_file+0x2c/0xa0
[    8.826269]  sysfs_add_file_mode_ns+0x99/0x180
[    8.831500]  sysfs_create_file_ns+0x2a/0x30
[    8.836433]  bus_create_file+0x47/0x70
[    8.840893]  bus_register+0xe4/0x280
[    8.845157]  ? sfi_init+0x1b0/0x1b0
[    8.849321]  ? set_debug_rodata+0x12/0x12
[    8.854064]  pnp_init+0x10/0x12
[    8.857829]  do_one_initcall+0x43/0x180
[    8.862383]  ? set_debug_rodata+0x12/0x12
[    8.867118]  kernel_init_freeable+0x19d/0x22a
[    8.872259]  ? rest_init+0x90/0x90
[    8.876324]  kernel_init+0xe/0x100
[    8.880389]  ret_from_fork+0x2c/0x40
[    8.884643] Code: 1f 44 00 00 55 48 89 e5 41 57 41 56 41 55 41 54 53 48 83 e4 f0 48 83 ec 70 48 85 f6 48 c7 44 24 20 00 00 00 00 0f 84 87 01 00 00 <48> 83 7e 08 00 0f 84 7c 01 00 00 48 89 f3 49 89 fd 48 89 f7 89 
[    8.906422] RIP: get_partial_node+0x2c/0x1f0 RSP: ffffc9000002fb20
[    8.914356] CR2: 000000000001f768
... ...

Thanks,
Aaron

Re: [LKP] [PATCH v2 0/4] Revert works for the mapping of cpuid <-> nodeid

[Thomas Gleixner]

On Thu, 16 Mar 2017, Aaron Lu wrote:
> 
> What is the status of the patch?
> 
> I still get oops during boot on a EP machine with today's Linus tree's
> head commit 69eea5a4ab9c("Merge branch 'for-linus' of git://git.kernel.dk/linux-block")

I have it applied to tip/x86/acpi. I was not aware that this is a urgent
issue to be forwarded to Linus ASAP.

I'll send it Linus wards in the next days.

Thanks,

	tglx

Re: [LKP] [PATCH v2 0/4] Revert works for the mapping of cpuid <-> nodeid

[Aaron Lu]

On Thu, Mar 16, 2017 at 09:28:55AM +0100, Thomas Gleixner wrote:
> On Thu, 16 Mar 2017, Aaron Lu wrote:
> > 
> > What is the status of the patch?
> > 
> > I still get oops during boot on a EP machine with today's Linus tree's
> > head commit 69eea5a4ab9c("Merge branch 'for-linus' of git://git.kernel.dk/linux-block")
> 
> I have it applied to tip/x86/acpi. I was not aware that this is a urgent
> issue to be forwarded to Linus ASAP.
> 
> I'll send it Linus wards in the next days.

That would be great, thanks.