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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Joseph Salisbury-3
BugLink: http://bugs.launchpad.net/bugs/1719697

== SRU Justification ==
Kernel crashes when installation of Ubuntu-16.04.3 with HWE (ISO).
Same observation while booting to 4.10.0-28 HWE kerenl of Ubuntu-16.04.3
and 4.10.0-33 HWE as well.

Seen only with 4.10 HWE kernels of Ubuntu-16.04.3.  4.4 kernels of
Ubuntu-16.04.3 works fine. Daily builds of Ubuntu Server 17.10 works fine.

Reducing the core count to <26 cores helps here. Boot & installation of
HWE-kernel works fine.

This bug was introduced by commit:
dc6db24d2476 ("x86/acpi: Set persistent cpuid <-> nodeid mapping when booting")

It is resolved by reverting commit dc6db24d2476, which was done in mainline by
commit c962cff17df as of v4.11-rc3.

There are three additiona commits introduced by the same patch author when
commit c962cff17df was submitted.  However, it was confirmed that only the single
revert is needed to fix this particular bug.  Upstream thread:
 https://lkml.org/lkml/2017/2/20/66

== Fix ==
commit c962cff17dfa11f4a8227ac16de2b28aea3312e4
Author: Dou Liyang <[hidden email]>
Date:   Fri Mar 3 16:02:23 2017 +0800

    Revert "x86/acpi: Set persistent cpuid <-> nodeid mapping when booting"

== Regression Potential ==
This is reverting a commit that introduced a bug.  This commit has also
been reverted upstream.

== Test Case ==
A test kernel was built with this patch and tested by the original bug reporter.
The bug reporter states the test kernel resolved the bug.


Dou Liyang (1):
  Revert "x86/acpi: Set persistent cpuid <-> nodeid mapping when
    booting"

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

--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

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

Joseph Salisbury-3
From: Dou Liyang <[hidden email]>

BugLink: http://bugs.launchpad.net/bugs/1719697

Revert: dc6db24d2476 ("x86/acpi: Set persistent cpuid <-> nodeid mapping when booting")

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.

Signed-off-by: Dou Liyang <[hidden email]>
Tested-by: Xiaolong Ye <[hidden email]>
Cc: [hidden email]
Cc: [hidden email]
Cc: [hidden email]
Cc: [hidden email]
Cc: [hidden email]
Link: http://lkml.kernel.org/r/1488528147-2279-2-git-send-email-douly.fnst@...
Signed-off-by: Thomas Gleixner <[hidden email]>

(cherry picked from commit c962cff17dfa11f4a8227ac16de2b28aea3312e4)
Signed-off-by: Joseph Salisbury <[hidden email]>
---
 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 7479785..5cbc119 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.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [SRU][Zesty][PATCH 0/1] Revert "x86/acpi: Set persistent cpuid <-> nodeid mapping when booting"

Marcelo Henrique Cerri
In reply to this post by Joseph Salisbury-3
Acked-by: Marcelo Henrique Cerri <[hidden email]>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (484 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Stefan Bader-2
In reply to this post by Joseph Salisbury-3
On 27.10.2017 21:23, Joseph Salisbury wrote:

> From: Dou Liyang <[hidden email]>
>
> BugLink: http://bugs.launchpad.net/bugs/1719697
>
> Revert: dc6db24d2476 ("x86/acpi: Set persistent cpuid <-> nodeid mapping when booting")
>
> 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.
>
> Signed-off-by: Dou Liyang <[hidden email]>
> Tested-by: Xiaolong Ye <[hidden email]>
> Cc: [hidden email]
> Cc: [hidden email]
> Cc: [hidden email]
> Cc: [hidden email]
> Cc: [hidden email]
> Link: http://lkml.kernel.org/r/1488528147-2279-2-git-send-email-douly.fnst@...
> Signed-off-by: Thomas Gleixner <[hidden email]>
>
> (cherry picked from commit c962cff17dfa11f4a8227ac16de2b28aea3312e4)
> Signed-off-by: Joseph Salisbury <[hidden email]>
Acked-by: Stefan Bader <[hidden email]>

> ---

Positive testing.

>  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 7479785..5cbc119 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
>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

APPLIED: [SRU][Zesty][PATCH 0/1] Revert "x86/acpi: Set persistent cpuid <-> nodeid mapping when booting"

Thadeu Lima de Souza Cascardo-3
In reply to this post by Joseph Salisbury-3
Applied to zesty master-next branch.

Thanks.
Cascardo.

Applied-to: zesty/master-next

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team