Re: [Intel-xe] [PATCH] drm/xe: add lockdep annotation for xe_device_mem_access_put()

[Matthew Auld <matthew.auld@intel.com>]

On 21/07/2023 19:59, Rodrigo Vivi wrote:
> On Fri, Jul 21, 2023 at 01:34:25PM +0100, Matthew Auld wrote:
>> The main motivation is with d3cold which will make the suspend and
>> resume callbacks even more scary, but is useful regardless. We already
>> have the needed annotation on the acquire side with
>> xe_device_mem_access_get(), and by adding the annotation on the release
>> side we should have a lot more confidence that our locking hierarchy is
>> correct.
> 
> good idea!
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Thanks. After sleeping on this, wondering if both callbacks should 
instead just be wrapped with lock_map_acquire/lock_map_release, and then 
in mem_access_get() we only do lock_map_acquire/lock_map_release at the 
start, without holding it over xe_pm_runtime_get()? I think that makes 
it clearer what why are trying to express:

"Any locks held when calling mem_access_get() are not allowed to be 
grabbed by the rpm suspend-resume callbacks".

Also slightly worried that something in rpm core will get confused by 
the annotation if we are holding it over xe_pm_runtime_get(), since it 
might look like a deadlock if there are more annotations deeper down. 
i.e lock(A) would be our lock_map_acquire in the diagram below.

> 
>>
>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> Cc: Anshuman Gupta <anshuman.gupta@intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_device.c |  2 +-
>>   drivers/gpu/drm/xe/xe_device.h |  4 ++++
>>   drivers/gpu/drm/xe/xe_pm.c     | 24 ++++++++++++++++++++++++
>>   3 files changed, 29 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
>> index 1c57944014e0..fed3015e2d96 100644
>> --- a/drivers/gpu/drm/xe/xe_device.c
>> +++ b/drivers/gpu/drm/xe/xe_device.c
>> @@ -36,7 +36,7 @@
>>   #include "xe_wait_user_fence.h"
>>   
>>   #ifdef CONFIG_LOCKDEP
>> -static struct lockdep_map xe_device_mem_access_lockdep_map = {
>> +struct lockdep_map xe_device_mem_access_lockdep_map = {
>>   	.name = "xe_device_mem_access_lockdep_map"
>>   };
>>   #endif
>> diff --git a/drivers/gpu/drm/xe/xe_device.h b/drivers/gpu/drm/xe/xe_device.h
>> index 8b085ffdc5f8..593accb68281 100644
>> --- a/drivers/gpu/drm/xe/xe_device.h
>> +++ b/drivers/gpu/drm/xe/xe_device.h
>> @@ -16,6 +16,10 @@ struct xe_file;
>>   #include "xe_force_wake.h"
>>   #include "xe_macros.h"
>>   
>> +#ifdef CONFIG_LOCKDEP
>> +extern struct lockdep_map xe_device_mem_access_lockdep_map;
>> +#endif
>> +
>>   static inline struct xe_device *to_xe_device(const struct drm_device *dev)
>>   {
>>   	return container_of(dev, struct xe_device, drm);
>> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
>> index 17a69b7af155..d1b2aa52ea03 100644
>> --- a/drivers/gpu/drm/xe/xe_pm.c
>> +++ b/drivers/gpu/drm/xe/xe_pm.c
>> @@ -199,6 +199,29 @@ int xe_pm_runtime_suspend(struct xe_device *xe)
>>   	/* Disable access_ongoing asserts and prevent recursive pm calls */
>>   	xe_pm_write_callback_task(xe, current);
>>   
>> +	/*
>> +	 * The actual xe_device_mem_access_put() is always async underneath, so
>> +	 * exactly where that is called should makes no difference to us. However
>> +	 * we still need to be very careful with the locks that this callback
>> +	 * acquires and the locks that are acquired and held by any callers of
>> +	 * xe_device_mem_access_get(). We already have the matching annotation
>> +	 * on that side, but we also need it here. For example lockdep should be
>> +	 * able to tell us if the following scenario is in theory possible:
>> +	 *
>> +	 * CPU0                          | CPU1 (kworker)
>> +	 * lock(A)                       |
>> +	 *                               | xe_pm_runtime_suspend()
>> +	 *                               |      lock(A)
>> +	 * xe_device_mem_access_get()    |
>> +	 *
>> +	 * This will clearly deadlock since rpm core needs to wait for
>> +	 * xe_pm_runtime_suspend() to complete, but here we are holding lock(A)
>> +	 * on CPU0 which prevents CPU1 making forward progress.  With the
>> +	 * annotation here and in xe_device_mem_access_get() lockdep will see
>> +	 * the potential lock inversion and give us a nice splat.
>> +	 */
>> +	lock_map_acquire(&xe_device_mem_access_lockdep_map);
>> +
>>   	if (xe->d3cold.allowed) {
>>   		err = xe_bo_evict_all(xe);
>>   		if (err)
>> @@ -213,6 +236,7 @@ int xe_pm_runtime_suspend(struct xe_device *xe)
>>   
>>   	xe_irq_suspend(xe);
>>   out:
>> +	lock_map_release(&xe_device_mem_access_lockdep_map);
>>   	xe_pm_write_callback_task(xe, NULL);
>>   	return err;
>>   }
>> -- 
>> 2.41.0
>>