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

[Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>]

Hi Matthew,

Thanks for review.

On Tuesday, 26 July 2022 20:14:05 CEST Matthew Auld wrote:
> On 20/07/2022 11:16, Janusz Krzysztofik wrote:
> > 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>
> 
> Can we go ahead and merge this patch without the second?

Rather not.  I labelled the second patch as RFC and people may read that as 
"not important" while it is important.  Since there were no comments on it, 
let me resubmit the series without that label.

Thanks,
Janusz

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