acpi/hmat: hmat_register_target() refactor to address lockdep warning
Subsystems (2)
Dave Jiang
maintainerv3 cover acpi/hmat: hmat_register_target() refactor to address lockdep warning
This series has changes that refactor the hmat_register_target() function to clean up a lockdep warning.
Dave Jiang (2):
acpi/hmat: Return when generic target is updated acpi/hmat: Fix lockdep warning for hmem_register_resource()
drivers/acpi/numa/hmat.c | 47 ++++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 21 deletions(-)
base-commit: 3a8660878839faadb4f1a6dd72c3179c1df56787
Signature · 1 line
2.51.0
Dave Jiang
maintainerPATCH v3 1/2 acpi/hmat: Return when generic target is updated
With the current code flow, once the generic target is updated target->registered is set and the remaining code is skipped. So return immediately instead of going through the checks and then skip.
--- drivers/acpi/numa/hmat.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
drivers/acpi/numa/hmat.c +6 -5
@@ -888,12 +888,13 @@ static void hmat_register_target(struct memory_target *target)* Register generic port perf numbers. The nid may not be * initialized and is still NUMA_NO_NODE. */ -mutex_lock (&target_lock); -if (*(u16 *)target->gen_port_device_handle) { -hmat_update_generic_target (target); - target->registered = true; + scoped_guard(mutex, &target_lock) { +if (*(u16 *)target->gen_port_device_handle) { +hmat_update_generic_target (target); + target->registered = true; +return ; + } } -mutex_unlock (&target_lock);/* * Skip offline nodes. This can happen when memory
Signature · 1 line
2.51.0
Dave Jiang
maintainerPATCH v3 2/2 acpi/hmat: Fix lockdep warning for hmem_register_resource()
In reply to Dave JiangThe following lockdep splat was observed while kernel auto-online a CXL memory region:
======================================================
WARNING: possible circular locking dependency detected
6.17.0djtest+ #53 Tainted: G W
------------------------------------------------------
systemd-udevd/3334 is trying to acquire lock:ffffffff90346188 (hmem_resource_lock){+.+.}-{4:4}, at: hmem_register_resource+0x31/0x50
but task is already holding lock:ffffffff90338890 ((node_chain).rwsem){++++}-{4:4}, at: blocking_notifier_call_chain+0x2e/0x70
which lock already depends on the new lock.
[..]
Chain exists of: hmem_resource_lock --> mem_hotplug_lock --> (node_chain).rwsem Possible unsafe locking scenario:
CPU0 CPU1
---- ----
rlock((node_chain).rwsem);
lock(mem_hotplug_lock);
lock((node_chain).rwsem); lock(hmem_resource_lock);
The lock ordering can cause potential deadlock. There are instances where hmem_resource_lock is taken after (node_chain).rwsem, and vice versa.
Split out the target update section of hmat_register_target() so that hmat_callback() only envokes that section instead of attempt to register hmem devices that it does not need to.
n "hmem" device")
notmuch/
---
v3:
- Refactor to split out target device setup vs target update (Dan)
--- drivers/acpi/numa/hmat.c | 48 ++++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 22 deletions(-)
drivers/acpi/numa/hmat.c +26 -22
@@ -874,28 +874,10 @@ static void hmat_register_target_devices(struct memory_target *target) } } -static void hmat_register_target (struct memory_target *target) +static void hmat_hotplug_target (struct memory_target *target) {int nid =pxm_to_node (target->memory_pxm); -/* -* Devices may belong to either an offline or online -* node, so unconditionally add them. -*/ -hmat_register_target_devices (target); - -/* -* Register generic port perf numbers. The nid may not be -* initialized and is still NUMA_NO_NODE. -*/ - scoped_guard(mutex, &target_lock) { -if (*(u16 *)target->gen_port_device_handle) { -hmat_update_generic_target (target); - target->registered = true; -return ; - } - } -/* * Skip offline nodes. This can happen when memory * marked EFI_MEMORY_SP, "specific purpose", is applied
@@ -906,7 +888,7 @@ static void hmat_register_target(struct memory_target *target)if (nid ==NUMA_NO_NODE || !node_online (nid))return ; -mutex_lock (&target_lock); +guard (mutex)(&target_lock);if (!target->registered) {hmat_register_target_initiators (target);hmat_register_target_cache (target);
@@ -914,7 +896,29 @@ static void hmat_register_target(struct memory_target *target)hmat_register_target_perf (target,ACCESS_COORDINATE_CPU ); target->registered = true; } -mutex_unlock (&target_lock); +} + +static void hmat_register_target (struct memory_target *target) +{ +/* +* Devices may belong to either an offline or online +* node, so unconditionally add them. +*/ +hmat_register_target_devices (target); + +/* +* Register generic port perf numbers. The nid may not be +* initialized and is still NUMA_NO_NODE. +*/ + scoped_guard(mutex, &target_lock) { +if (*(u16 *)target->gen_port_device_handle) { +hmat_update_generic_target (target); + target->registered = true; +return ; + } + } + +hmat_hotplug_target (target); }static void hmat_register_targets (void )
@@ -940,7 +944,7 @@ static int hmat_callback(struct notifier_block *self,if (!target)return NOTIFY_OK ; -hmat_register_target (target); +hmat_hotplug_target (target);return NOTIFY_OK ; }
Signature · 1 line
2.51.0
Dave Jiang
maintainerv4 cover acpi/hmat: hmat_register_target() refactor to address lockdep warning
In reply to Dave Jiangv4:
- Refactor hmat_hotplug_target() (Jonathan)
- Fix fixes tag. (Jonathan)
This series has changes that refactor the hmat_register_target() function to clean up a lockdep warning.
Dave Jiang (2):
acpi/hmat: Return when generic target is updated acpi/hmat: Fix lockdep warning for hmem_register_resource()
drivers/acpi/numa/hmat.c | 60 ++++++++++++++++++++++------------------ 1 file changed, 33 insertions(+), 27 deletions(-)
base-commit: 6146a0f1dfae5d37442a9ddcba012add260bceb0
Signature · 1 line
2.51.1
Dave Jiang
maintainerPATCH v4 1/2 acpi/hmat: Return when generic target is updated
In reply to Dave JiangWith the current code flow, once the generic target is updated target->registered is set and the remaining code is skipped. So return immediately instead of going through the checks and then skip.
---
no changes
--- drivers/acpi/numa/hmat.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
drivers/acpi/numa/hmat.c +6 -5
@@ -888,12 +888,13 @@ static void hmat_register_target(struct memory_target *target)* Register generic port perf numbers. The nid may not be * initialized and is still NUMA_NO_NODE. */ -mutex_lock (&target_lock); -if (*(u16 *)target->gen_port_device_handle) { -hmat_update_generic_target (target); - target->registered = true; + scoped_guard(mutex, &target_lock) { +if (*(u16 *)target->gen_port_device_handle) { +hmat_update_generic_target (target); + target->registered = true; +return ; + } } -mutex_unlock (&target_lock);/* * Skip offline nodes. This can happen when memory
Signature · 1 line
2.51.1
Dave Jiang
maintainerPATCH v4 2/2 acpi/hmat: Fix lockdep warning for hmem_register_resource()
In reply to Dave JiangThe following lockdep splat was observed while kernel auto-online a CXL memory region:
======================================================
WARNING: possible circular locking dependency detected
6.17.0djtest+ #53 Tainted: G W
------------------------------------------------------
systemd-udevd/3334 is trying to acquire lock:ffffffff90346188 (hmem_resource_lock){+.+.}-{4:4}, at: hmem_register_resource+0x31/0x50
but task is already holding lock:ffffffff90338890 ((node_chain).rwsem){++++}-{4:4}, at: blocking_notifier_call_chain+0x2e/0x70
which lock already depends on the new lock.
[..]
Chain exists of: hmem_resource_lock --> mem_hotplug_lock --> (node_chain).rwsem Possible unsafe locking scenario:
CPU0 CPU1
---- ----
rlock((node_chain).rwsem);
lock(mem_hotplug_lock);
lock((node_chain).rwsem); lock(hmem_resource_lock);
The lock ordering can cause potential deadlock. There are instances where hmem_resource_lock is taken after (node_chain).rwsem, and vice versa.
Split out the target update section of hmat_register_target() so that hmat_callback() only envokes that section instead of attempt to register hmem devices that it does not need to.
---
v4:
- Fix fixes tag. (Jonathan)
- Refactor hmat_hotplug_target(). (Jonathan)
--- drivers/acpi/numa/hmat.c | 47 ++++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 21 deletions(-)
drivers/acpi/numa/hmat.c +26 -21
@@ -874,10 +874,33 @@ static void hmat_register_target_devices(struct memory_target *target) } } -static void hmat_register_target (struct memory_target *target) +static void hmat_hotplug_target (struct memory_target *target) {int nid =pxm_to_node (target->memory_pxm); +/* +* Skip offline nodes. This can happen when memory +* marked EFI_MEMORY_SP, "specific purpose", is applied +* to all the memory in a proximity domain leading to +* the node being marked offline / unplugged, or if +* memory-only "hotplug" node is offline. +*/ +if (nid ==NUMA_NO_NODE || !node_online (nid)) +return ; + +guard (mutex)(&target_lock); +if (target->registered) +return ; + +hmat_register_target_initiators (target); +hmat_register_target_cache (target); +hmat_register_target_perf (target,ACCESS_COORDINATE_LOCAL ); +hmat_register_target_perf (target,ACCESS_COORDINATE_CPU ); + target->registered = true; +} + +static void hmat_register_target (struct memory_target *target) +{/* * Devices may belong to either an offline or online * node, so unconditionally add them.
@@ -896,25 +919,7 @@ static void hmat_register_target(struct memory_target *target) } } -/* -* Skip offline nodes. This can happen when memory -* marked EFI_MEMORY_SP, "specific purpose", is applied -* to all the memory in a proximity domain leading to -* the node being marked offline / unplugged, or if -* memory-only "hotplug" node is offline. -*/ -if (nid ==NUMA_NO_NODE || !node_online (nid)) -return ; - -mutex_lock (&target_lock); -if (!target->registered) { -hmat_register_target_initiators (target); -hmat_register_target_cache (target); -hmat_register_target_perf (target,ACCESS_COORDINATE_LOCAL ); -hmat_register_target_perf (target,ACCESS_COORDINATE_CPU ); - target->registered = true; - } -mutex_unlock (&target_lock); +hmat_hotplug_target (target); }static void hmat_register_targets (void )
@@ -940,7 +945,7 @@ static int hmat_callback(struct notifier_block *self,if (!target)return NOTIFY_OK ; -hmat_register_target (target); +hmat_hotplug_target (target);return NOTIFY_OK ; }
Signature · 1 line
2.51.1
Koralahalli Channabasappa, Smita
Tested-by Smita Koralahalli Reviewed-by Smita Koralahalli on v4 2/2Re: [PATCH v4 2/2] acpi/hmat: Fix lockdep warning for hmem_register_resource()
Hi Dave,
Quoted 31 lines · Dave Jiang
The following lockdep splat was observed while kernel auto-online a CXL memory region:
======================================================
WARNING: possible circular locking dependency detected
6.17.0djtest+ #53 Tainted: G W
------------------------------------------------------
systemd-udevd/3334 is trying to acquire lock:ffffffff90346188(hmem_resource_lock){+.+.}-{4:4}, at:hmem_register_resource+0x31/0x50but task is already holding lock:
ffffffff90338890((node_chain).rwsem){++++}-{4:4}, at:blocking_notifier_call_chain+0x2e/0x70which lock already depends on the new lock.
[..]Chain exists of: hmem_resource_lock --> mem_hotplug_lock --> (node_chain).rwsem Possible unsafe locking scenario:CPU0 CPU1 ---- ---- rlock((node_chain).rwsem); lock(mem_hotplug_lock); lock((node_chain).rwsem);
lock(hmem_resource_lock);The lock ordering can cause potential deadlock. There are instances where
hmem_resource_lockis taken after (node_chain).rwsem, and vice versa.
Quoted 7 lines
Split out the target update section of
hmat_register_target()so thathmat_callback()only envokes that section instead of attempt to register hmem devices that it does not need to.Fixes cf8741ac57ed ("ACPI: NUMA: HMAT: Register "soft reserved" memory as an "hmem" device")Signed-off-by Dave Jiang <[email protected]>---
Thanks for this fix.
This resolves a deadlock I was hitting while working on the Soft Reserved series.
Before this change, the HMEM deferred work path could hold hmem_resource_lock while eventually triggering memory onlining where udev holds mem_hotplug_lock.
Quoted 82 lines
v4:
- Fix fixes tag. (Jonathan)
- Refactorhmat_hotplug_target(). (Jonathan)--- drivers/acpi/numa/hmat.c | 47 ++++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 21 deletions(-)drivers/acpi/numa/hmat.c +26
-21@@ -874,10 +874,33 @@ static void hmat_register_target_devices(struct memory_target *target) } } -static void hmat_register_target (struct memory_target *target) +static void hmat_hotplug_target (struct memory_target *target) {int nid =pxm_to_node (target->memory_pxm); +/* +* Skip offline nodes. This can happen when memory +* marked EFI_MEMORY_SP, "specific purpose", is applied +* to all the memory in a proximity domain leading to +* the node being marked offline / unplugged, or if +* memory-only "hotplug" node is offline. +*/ +if (nid ==NUMA_NO_NODE || !node_online (nid)) +return ; + +guard (mutex)(&target_lock); +if (target->registered) +return ; + +hmat_register_target_initiators (target); +hmat_register_target_cache (target); +hmat_register_target_perf (target,ACCESS_COORDINATE_LOCAL ); +hmat_register_target_perf (target,ACCESS_COORDINATE_CPU ); + target->registered = true; +} + +static void hmat_register_target (struct memory_target *target) +{/* * Devices may belong to either an offline or online * node, so unconditionally add them. @@ -896,25 +919,7 @@ static void hmat_register_target(struct memory_target *target) } } -/* -* Skip offline nodes. This can happen when memory -* marked EFI_MEMORY_SP, "specific purpose", is applied -* to all the memory in a proximity domain leading to -* the node being marked offline / unplugged, or if -* memory-only "hotplug" node is offline. -*/ -if (nid ==NUMA_NO_NODE || !node_online (nid)) -return ; - -mutex_lock (&target_lock); -if (!target->registered) { -hmat_register_target_initiators (target); -hmat_register_target_cache (target); -hmat_register_target_perf (target,ACCESS_COORDINATE_LOCAL ); -hmat_register_target_perf (target,ACCESS_COORDINATE_CPU ); - target->registered = true; - } -mutex_unlock (&target_lock); +hmat_hotplug_target (target); }static void hmat_register_targets (void )@@ -940,7 +945,7 @@ static int hmat_callback(struct notifier_block *self,if (!target)return NOTIFY_OK ; -hmat_register_target (target); +hmat_hotplug_target (target);return NOTIFY_OK ; }
Dave Jiang
maintainerQuoted 7 lines · Dave Jiang
This series has changes that refactor the
hmat_register_target()function to clean up a lockdep warning.Dave Jiang (2):
acpi/hmat: Return when generic target is updated acpi/hmat: Fix lockdep warning forhmem_register_resource()
Quoted 5 lines
drivers/acpi/numa/hmat.c | 47 ++++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 21 deletions(-)base-commit:
3a8660878839faadb4f1a6dd72c3179c1df56787
Ignore please. See v4 resend series.