[PATCH 1/2] drm/i915/gem: Avoid taking runtime-pm under the shrinker
Matthew Auld
matthew.auld at intel.com
Tue Jul 26 18:14:05 UTC 2022
On 20/07/2022 11:16, Janusz Krzysztofik wrote:
> From: Chris Wilson <chris at chris-wilson.co.uk>
>
> Inside the shrinker, we cannot wake the device as that may cause
> recursion into fs-reclaim, so instead we only unbind vma if the device
> is currently awake. (In order to provide reclaim while asleep, we do
> wake the device up during kswapd -- we probably want to limit that wake
> up if we have anything to shrink though!)
>
> To avoid the same fs_reclaim recursion potential during
> i915_gem_object_unbind, we acquire a wakeref there, see commit
> 3e817471a34c ("drm/i915/gem: Take runtime-pm wakeref prior to unbinding").
> However, we use i915_gem_object_unbind from the shrinker path to make the
> object available for shrinking and so we must make the wakeref acquisition
> here conditional.
>
> <4> [437.542172] ======================================================
> <4> [437.542174] WARNING: possible circular locking dependency detected
> <4> [437.542176] 5.19.0-rc6-CI_DRM_11876-g2305e0d00665+ #1 Tainted: G U
> <4> [437.542179] ------------------------------------------------------
> <4> [437.542181] kswapd0/93 is trying to acquire lock:
> <4> [437.542183] ffffffff827a7608 (acpi_wakeup_lock){+.+.}-{3:3}, at: acpi_device_wakeup_disable+0x12/0x50
> <4> [437.542191]
> but task is already holding lock:
> <4> [437.542194] ffffffff8275d360 (fs_reclaim){+.+.}-{0:0}, at: balance_pgdat+0x91/0x5c0
> <4> [437.542199]
> which lock already depends on the new lock.
> <4> [437.542202]
> the existing dependency chain (in reverse order) is:
> <4> [437.542204]
> -> #2 (fs_reclaim){+.+.}-{0:0}:
> <4> [437.542207] fs_reclaim_acquire+0x9d/0xd0
> <4> [437.542211] kmem_cache_alloc_trace+0x2a/0x250
> <4> [437.542214] __acpi_device_add+0x263/0x3a0
> <4> [437.542217] acpi_add_single_object+0x3ea/0x710
> <4> [437.542220] acpi_bus_check_add+0xf7/0x240
> <4> [437.542222] acpi_bus_scan+0x34/0xf0
> <4> [437.542224] acpi_scan_init+0xf5/0x241
> <4> [437.542228] acpi_init+0x449/0x4aa
> <4> [437.542230] do_one_initcall+0x53/0x2e0
> <4> [437.542233] kernel_init_freeable+0x18f/0x1dd
> <4> [437.542236] kernel_init+0x11/0x110
> <4> [437.542239] ret_from_fork+0x1f/0x30
> <4> [437.542241]
> -> #1 (acpi_device_lock){+.+.}-{3:3}:
> <4> [437.542245] __mutex_lock+0x97/0xf20
> <4> [437.542246] acpi_enable_wakeup_device_power+0x30/0xf0
> <4> [437.542249] __acpi_device_wakeup_enable+0x31/0x110
> <4> [437.542252] acpi_pm_set_device_wakeup+0x55/0x100
> <4> [437.542254] __pci_enable_wake+0x5e/0xa0
> <4> [437.542257] pci_finish_runtime_suspend+0x32/0x70
> <4> [437.542259] pci_pm_runtime_suspend+0xa3/0x160
> <4> [437.542262] __rpm_callback+0x3d/0x110
> <4> [437.542265] rpm_callback+0x54/0x60
> <4> [437.542268] rpm_suspend.part.10+0x105/0x5a0
> <4> [437.542270] pm_runtime_work+0x7d/0x1e0
> <4> [437.542273] process_one_work+0x272/0x5c0
> <4> [437.542276] worker_thread+0x37/0x370
> <4> [437.542278] kthread+0xed/0x120
> <4> [437.542280] ret_from_fork+0x1f/0x30
> <4> [437.542282]
> -> #0 (acpi_wakeup_lock){+.+.}-{3:3}:
> <4> [437.542285] __lock_acquire+0x15ad/0x2940
> <4> [437.542288] lock_acquire+0xd3/0x310
> <4> [437.542291] __mutex_lock+0x97/0xf20
> <4> [437.542293] acpi_device_wakeup_disable+0x12/0x50
> <4> [437.542295] acpi_pm_set_device_wakeup+0x6e/0x100
> <4> [437.542297] __pci_enable_wake+0x73/0xa0
> <4> [437.542300] pci_pm_runtime_resume+0x45/0x90
> <4> [437.542302] __rpm_callback+0x3d/0x110
> <4> [437.542304] rpm_callback+0x54/0x60
> <4> [437.542307] rpm_resume+0x54f/0x750
> <4> [437.542309] __pm_runtime_resume+0x42/0x80
> <4> [437.542311] __intel_runtime_pm_get+0x19/0x80 [i915]
> <4> [437.542386] i915_gem_object_unbind+0x8f/0x3b0 [i915]
> <4> [437.542487] i915_gem_shrink+0x634/0x850 [i915]
> <4> [437.542584] i915_gem_shrinker_scan+0x3a/0xc0 [i915]
> <4> [437.542679] shrink_slab.constprop.97+0x1a4/0x4f0
> <4> [437.542684] shrink_node+0x21e/0x420
> <4> [437.542687] balance_pgdat+0x241/0x5c0
> <4> [437.542690] kswapd+0x229/0x4f0
> <4> [437.542694] kthread+0xed/0x120
> <4> [437.542697] ret_from_fork+0x1f/0x30
> <4> [437.542701]
> other info that might help us debug this:
> <4> [437.542705] Chain exists of:
> acpi_wakeup_lock --> acpi_device_lock --> fs_reclaim
> <4> [437.542713] Possible unsafe locking scenario:
> <4> [437.542716] CPU0 CPU1
> <4> [437.542719] ---- ----
> <4> [437.542721] lock(fs_reclaim);
> <4> [437.542725] lock(acpi_device_lock);
> <4> [437.542728] lock(fs_reclaim);
> <4> [437.542732] lock(acpi_wakeup_lock);
> <4> [437.542736]
> *** DEADLOCK ***
>
> Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/6449
> Fixes: 3e817471a34c ("drm/i915/gem: Take runtime-pm wakeref prior to unbinding")
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Matthew Auld <matthew.auld at intel.com>
> Cc: <stable at vger.kernel.org> # v5.6+
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
Reviewed-by: Matthew Auld <matthew.auld at intel.com>
Can we go ahead and merge this patch without the second?
> ---
> drivers/gpu/drm/i915/i915_gem.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 702e5b89be22..910a6fde5726 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -119,8 +119,8 @@ int i915_gem_object_unbind(struct drm_i915_gem_object *obj,
> {
> struct intel_runtime_pm *rpm = &to_i915(obj->base.dev)->runtime_pm;
> bool vm_trylock = !!(flags & I915_GEM_OBJECT_UNBIND_VM_TRYLOCK);
> + intel_wakeref_t wakeref = 0;
> LIST_HEAD(still_in_list);
> - intel_wakeref_t wakeref;
> struct i915_vma *vma;
> int ret;
>
> @@ -135,7 +135,8 @@ int i915_gem_object_unbind(struct drm_i915_gem_object *obj,
> * as they are required by the shrinker. Ergo, we wake the device up
> * first just in case.
> */
> - wakeref = intel_runtime_pm_get(rpm);
> + if (!(flags & I915_GEM_OBJECT_UNBIND_TEST))
> + wakeref = intel_runtime_pm_get(rpm);
>
> try_again:
> ret = 0;
> @@ -200,7 +201,8 @@ int i915_gem_object_unbind(struct drm_i915_gem_object *obj,
> goto try_again;
> }
>
> - intel_runtime_pm_put(rpm, wakeref);
> + if (wakeref)
> + intel_runtime_pm_put(rpm, wakeref);
>
> return ret;
> }
More information about the dri-devel
mailing list