Re: [Intel-xe] [PATCH v12 01/13] drm/xe: fix xe_device_mem_access_get() races

["Gupta, Anshuman" <anshuman.gupta@intel.com>]

On 6/26/2023 4:20 PM, Matthew Auld wrote:
> It looks like there is at least one race here, given that the
> pm_runtime_suspended() check looks to return false if we are in the
> process of suspending the device (RPM_SUSPENDING vs RPM_SUSPENDED).  We
> later also do xe_pm_runtime_get_if_active(), but since the device is
> suspending or has now suspended, this doesn't do anything either.
> Following from this we can potentially return from
> xe_device_mem_access_get() with the device suspended or about to be,
> leading to broken behaviour.
> 
> Attempt to fix this by always grabbing the runtime ref when our internal
> ref transitions from 0 -> 1. The hard part is then dealing with the
> runtime_pm callbacks also calling xe_device_mem_access_get() and
> deadlocking, which the pm_runtime_suspended() check prevented.
> 
> v2:
>   - ct->lock looks to be primed with fs_reclaim, so holding that and then
>     allocating memory will cause lockdep to complain. Now that we
>     unconditionally grab the mem_access.lock around mem_access_{get,put}, we
>     need to change the ordering wrt to grabbing the ct->lock, since some of
>     the runtime_pm routines can allocate memory (or at least that's what
>     lockdep seems to suggest). Hopefully not a big deal.  It might be that
>     there were already issues with this, just that the atomics where
>     "hiding" the potential issues.
> v3:
>   - Use Thomas Hellström' idea with tracking the active task that is
>     executing in the resume or suspend callback, in order to avoid
>     recursive resume/suspend calls deadlocking on itself.
>   - Split the ct->lock change.
> v4:
>   - Add smb_mb() around accessing the pm_callback_task for extra safety.
>     (Thomas Hellström)
> v5:
>   - Clarify the kernel-doc for the mem_access.lock, given that it is quite
>     strange in what it protects (data vs code). The real motivation is to
>     aid lockdep. (Rodrigo Vivi)
> v6:
>   - Split out the lock change. We still want this as a lockdep aid but
>     only for the xe_device_mem_access_get() path. Sticking a lock on the
>     put() looks be a no-go, also the runtime_put() there is always async.
>   - Now that the lock is gone move to atomics and rely on the pm code
>     serialising multiple callers on the 0 -> 1 transition.
>   - g2h_worker_func() looks to be the next issue, given that
>     suspend-resume callbacks are using CT, so try to handle that.
> v7:
>   - Add xe_device_mem_access_get_if_ongoing(), and use it in
>     g2h_worker_func().
> 
> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/258
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_device.c       | 58 +++++++++++++++++++-----
>   drivers/gpu/drm/xe/xe_device.h       | 12 ++---
>   drivers/gpu/drm/xe/xe_device_types.h |  6 +++
>   drivers/gpu/drm/xe/xe_guc_ct.c       | 34 +++++++++++++-
>   drivers/gpu/drm/xe/xe_pm.c           | 66 +++++++++++++++++++---------
>   drivers/gpu/drm/xe/xe_pm.h           |  3 +-
>   6 files changed, 134 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index c7985af85a53..1dc552da434f 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -411,27 +411,61 @@ u32 xe_device_ccs_bytes(struct xe_device *xe, u64 size)
>   		DIV_ROUND_UP(size, NUM_BYTES_PER_CCS_BYTE) : 0;
>   }
>   
> +bool xe_device_mem_access_ongoing(struct xe_device *xe)
> +{
> +	if (xe_pm_read_callback_task(xe) != NULL)
> +		return true;
> +
> +	return atomic_read(&xe->mem_access.ref);
> +}
> +
> +void xe_device_assert_mem_access(struct xe_device *xe)
> +{
> +	XE_WARN_ON(!xe_device_mem_access_ongoing(xe));
> +}
> +
> +bool xe_device_mem_access_get_if_ongoing(struct xe_device *xe)
> +{
> +	return atomic_inc_not_zero(&xe->mem_access.ref);
> +}
> +
>   void xe_device_mem_access_get(struct xe_device *xe)
>   {
> -	bool resumed = xe_pm_runtime_resume_if_suspended(xe);
> -	int ref = atomic_inc_return(&xe->mem_access.ref);
> +	/*
> +	 * This looks racy, but should be fine since the pm_callback_task only
> +	 * transitions from NULL -> current (and back to NULL again), during the
> +	 * runtime_resume() or runtime_suspend() callbacks, for which there can
> +	 * only be a single one running for our device. We only need to prevent
> +	 * recursively calling the runtime_get or runtime_put from those
> +	 * callbacks, as well as preventing triggering any access_ongoing
> +	 * asserts.
> +	 */
two runtime_suspend() can run in parallel for two different pci device 
those worker thread pooled by pm_wq workqueue, it is not guaranteed 
that tast_struct will be different for two worker spawned by same pm_wq ?

> +	if (xe_pm_read_callback_task(xe) == current)
> +		return;
>   
> -	if (ref == 1)
> -		xe->mem_access.hold_rpm = xe_pm_runtime_get_if_active(xe);
> +	if (!atomic_inc_not_zero(&xe->mem_access.ref)) {
> +		bool hold_rpm = xe_pm_runtime_resume_and_get(xe);
> +		int ref;
>   
> -	/* The usage counter increased if device was immediately resumed */
> -	if (resumed)
> -		xe_pm_runtime_put(xe);
> -
> -	XE_WARN_ON(ref == S32_MAX);
> +		ref = atomic_inc_return(&xe->mem_access.ref);
> +		if (ref == 1)
> +			xe->mem_access.hold_rpm = hold_rpm;
> +		else
> +			xe_pm_runtime_put(xe);
> +	} else {
> +		XE_WARN_ON(atomic_read(&xe->mem_access.ref) == S32_MAX);
> +	}
>   }
>   
>   void xe_device_mem_access_put(struct xe_device *xe)
>   {
> -	bool hold = xe->mem_access.hold_rpm;
> -	int ref = atomic_dec_return(&xe->mem_access.ref);
> +	int ref;
>   
> -	if (!ref && hold)
> +	if (xe_pm_read_callback_task(xe) == current)
> +		return;
> +
> +	ref = atomic_dec_return(&xe->mem_access.ref);
> +	if (ref == 0 && xe->mem_access.hold_rpm)
>   		xe_pm_runtime_put(xe);
>   
>   	XE_WARN_ON(ref < 0);
/snip
> +
>   int xe_pm_runtime_suspend(struct xe_device *xe)
>   {
>   	struct xe_gt *gt;
>   	u8 id;
> -	int err;
> +	int err = 0;
> +
> +	if (xe->d3cold_allowed && xe_device_mem_access_ongoing(xe))
> +		return -EBUSY;
Not related to this patch but We should return -EBUSY even for d3hot as 
well.
Br,
Anshuman Gupta
> +
> +	/* Disable access_ongoing asserts and prevent recursive pm calls */
> +	xe_pm_write_callback_task(xe, current);
>   
>   	if (xe->d3cold_allowed) {
> -		if (xe_device_mem_access_ongoing(xe))
> -			return -EBUSY;
> -
>   		err = xe_bo_evict_all(xe);
>   		if (err)
> -			return err;
> +			goto out;
>   	}
>   
>   	for_each_gt(gt, xe, id) {
>   		err = xe_gt_suspend(gt);
>   		if (err)
> -			return err;
> +			goto out;
>   	}
>   
>   	xe_irq_suspend(xe);
> -
> -	return 0;
> +out:
> +	xe_pm_write_callback_task(xe, NULL);
> +	return err;
>   }
>   
>   int xe_pm_runtime_resume(struct xe_device *xe)
>   {
>   	struct xe_gt *gt;
>   	u8 id;
> -	int err;
> +	int err = 0;
> +
> +	/* Disable access_ongoing asserts and prevent recursive pm calls */
> +	xe_pm_write_callback_task(xe, current);
>   
>   	if (xe->d3cold_allowed) {
>   		for_each_gt(gt, xe, id) {
>   			err = xe_pcode_init(gt);
>   			if (err)
> -				return err;
> +				goto out;
>   		}
>   
>   		/*
> @@ -182,7 +210,7 @@ int xe_pm_runtime_resume(struct xe_device *xe)
>   		 */
>   		err = xe_bo_restore_kernel(xe);
>   		if (err)
> -			return err;
> +			goto out;
>   	}
>   
>   	xe_irq_resume(xe);
> @@ -193,10 +221,11 @@ int xe_pm_runtime_resume(struct xe_device *xe)
>   	if (xe->d3cold_allowed) {
>   		err = xe_bo_restore_user(xe);
>   		if (err)
> -			return err;
> +			goto out;
>   	}
> -
> -	return 0;
> +out:
> +	xe_pm_write_callback_task(xe, NULL);
> +	return err;
>   }
>   
>   int xe_pm_runtime_get(struct xe_device *xe)
> @@ -210,14 +239,9 @@ int xe_pm_runtime_put(struct xe_device *xe)
>   	return pm_runtime_put_autosuspend(xe->drm.dev);
>   }
>   
> -/* Return true if resume operation happened and usage count was increased */
> -bool xe_pm_runtime_resume_if_suspended(struct xe_device *xe)
> +bool xe_pm_runtime_resume_and_get(struct xe_device *xe)
>   {
> -	/* In case we are suspended we need to immediately wake up */
> -	if (pm_runtime_suspended(xe->drm.dev))
> -		return !pm_runtime_resume_and_get(xe->drm.dev);
> -
> -	return false;
> +	return !pm_runtime_resume_and_get(xe->drm.dev);
>   }
>   
>   int xe_pm_runtime_get_if_active(struct xe_device *xe)
> diff --git a/drivers/gpu/drm/xe/xe_pm.h b/drivers/gpu/drm/xe/xe_pm.h
> index 6a885585f653..e92c508d44b9 100644
> --- a/drivers/gpu/drm/xe/xe_pm.h
> +++ b/drivers/gpu/drm/xe/xe_pm.h
> @@ -19,7 +19,8 @@ int xe_pm_runtime_suspend(struct xe_device *xe);
>   int xe_pm_runtime_resume(struct xe_device *xe);
>   int xe_pm_runtime_get(struct xe_device *xe);
>   int xe_pm_runtime_put(struct xe_device *xe);
> -bool xe_pm_runtime_resume_if_suspended(struct xe_device *xe);
> +bool xe_pm_runtime_resume_and_get(struct xe_device *xe);
>   int xe_pm_runtime_get_if_active(struct xe_device *xe);
> +struct task_struct *xe_pm_read_callback_task(struct xe_device *xe);
>   
>   #endif