[RESUBMIT][PATCH 1/2] drm/i915/gem: Avoid taking runtime-pm under the shrinker

* [RESUBMIT][PATCH 1/2] drm/i915/gem: Avoid taking runtime-pm under the shrinker
@ 2022-07-29 10:56 ` Janusz Krzysztofik
  0 siblings, 0 replies; 8+ messages in thread
From: Janusz Krzysztofik @ 2022-07-29 10:56 UTC (permalink / raw)
  To: intel-gfx
  Cc: Thomas Hellström, Tvrtko Ursulin, dri-devel, Chris Wilson,
	Matthew Auld, Janusz Krzysztofik

From: Chris Wilson <chris@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@chris-wilson.co.uk>
Cc: Matthew Auld <matthew.auld@intel.com>
Cc: <stable@vger.kernel.org> # v5.6+
Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
---
Resubmit reason: keep in series with the other patch while dropping RFC
label from it.

 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;
 }
-- 
2.25.1


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