[Intel-xe] [PATCH v2 2/3] drm/xe: fix xe_device_mem_access_get() race
Matthew Auld
matthew.auld at intel.com
Fri May 5 19:04:16 UTC 2023
On 05/05/2023 19:20, Rodrigo Vivi wrote:
> On Fri, May 05, 2023 at 07:03:48PM +0100, 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, and then wrap the whole thing with a lock
>> to ensure callers are serialized.
>>
>> 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.
>
> it could've be done with a separated patch coming before, but anyway,
Yeah, something is still very broken with lockdep though according to
CI...now it's complaining about a different lock. Will look again at
this next week.
>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>
>>
>> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/258
>> Signed-off-by: Matthew Auld <matthew.auld at intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> Cc: Thomas Hellström <thomas.hellstrom at linux.intel.com>
>> Cc: Matthew Brost <matthew.brost at intel.com>
>> ---
>> drivers/gpu/drm/xe/xe_device.c | 11 +++--------
>> drivers/gpu/drm/xe/xe_device_types.h | 5 ++++-
>> drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c | 4 ++++
>> drivers/gpu/drm/xe/xe_guc_ct.c | 13 +++++++------
>> drivers/gpu/drm/xe/xe_pm.c | 9 ++-------
>> drivers/gpu/drm/xe/xe_pm.h | 2 +-
>> 6 files changed, 21 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
>> index 01c497bcf9a5..0a18b41a0e1a 100644
>> --- a/drivers/gpu/drm/xe/xe_device.c
>> +++ b/drivers/gpu/drm/xe/xe_device.c
>> @@ -406,17 +406,12 @@ u32 xe_device_ccs_bytes(struct xe_device *xe, u64 size)
>>
>> void xe_device_mem_access_get(struct xe_device *xe)
>> {
>> - bool resumed = xe_pm_runtime_resume_if_suspended(xe);
>> -
>> mutex_lock(&xe->mem_access.lock);
>> - if (xe->mem_access.ref++ == 0)
>> - xe->mem_access.hold_rpm = xe_pm_runtime_get_if_active(xe);
>> + if (xe->mem_access.ref == 0)
>> + xe->mem_access.hold_rpm = xe_pm_runtime_resume_and_get(xe);
>> + xe->mem_access.ref++;
>> mutex_unlock(&xe->mem_access.lock);
>>
>> - /* The usage counter increased if device was immediately resumed */
>> - if (resumed)
>> - xe_pm_runtime_put(xe);
>> -
>> XE_WARN_ON(xe->mem_access.ref == S32_MAX);
>> }
>>
>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>> index 59462933f67a..9e37189d5745 100644
>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>> @@ -256,7 +256,10 @@ struct xe_device {
>> * triggering additional actions when they occur.
>> */
>> struct {
>> - /** @lock: protect the ref count */
>> + /**
>> + * @lock: Serialize xe_device_mem_access users,
>> + * and protect the below internal state, like @ref.
>> + */
>> struct mutex lock;
>> /** @ref: ref count of memory accesses */
>> s32 ref;
>> diff --git a/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c b/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c
>> index 604f189dbd70..e769af7e7cdb 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c
>> +++ b/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c
>> @@ -5,6 +5,7 @@
>>
>> #include "xe_gt_tlb_invalidation.h"
>>
>> +#include "xe_device.h"
>> #include "xe_gt.h"
>> #include "xe_guc.h"
>> #include "xe_guc_ct.h"
>> @@ -112,6 +113,8 @@ static int send_tlb_invalidation(struct xe_guc *guc,
>> * in order which they currently are, if that changes the algorithm will
>> * need to be updated.
>> */
>> +
>> + xe_device_mem_access_get(gt->xe);
>> mutex_lock(&guc->ct.lock);
>> seqno = gt->tlb_invalidation.seqno;
>> if (fence) {
>> @@ -140,6 +143,7 @@ static int send_tlb_invalidation(struct xe_guc *guc,
>> if (ret < 0 && fence)
>> invalidation_fence_signal(fence);
>> mutex_unlock(&guc->ct.lock);
>> + xe_device_mem_access_put(gt->xe);
>>
>> return ret;
>> }
>> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
>> index 9055ff133a7c..579d7f341f13 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_ct.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
>> @@ -494,26 +494,22 @@ static int __guc_ct_send_locked(struct xe_guc_ct *ct, const u32 *action,
>> }
>> }
>>
>> - xe_device_mem_access_get(ct_to_xe(ct));
>> retry:
>> ret = has_room(ct, len + GUC_CTB_HDR_LEN, g2h_len);
>> if (unlikely(ret))
>> - goto put_wa;
>> + goto out;
>>
>> ret = h2g_write(ct, action, len, g2h_fence ? g2h_fence->seqno : 0,
>> !!g2h_fence);
>> if (unlikely(ret)) {
>> if (ret == -EAGAIN)
>> goto retry;
>> - goto put_wa;
>> + goto out;
>> }
>>
>> g2h_reserve_space(ct, g2h_len, num_g2h);
>> xe_guc_notify(ct_to_guc(ct));
>> -put_wa:
>> - xe_device_mem_access_put(ct_to_xe(ct));
>> out:
>> -
>> return ret;
>> }
>>
>> @@ -535,6 +531,7 @@ static int guc_ct_send_locked(struct xe_guc_ct *ct, const u32 *action, u32 len,
>>
>> XE_BUG_ON(g2h_len && g2h_fence);
>> lockdep_assert_held(&ct->lock);
>> + xe_device_assert_mem_access(ct_to_xe(ct));
>>
>> try_again:
>> ret = __guc_ct_send_locked(ct, action, len, g2h_len, num_g2h,
>> @@ -602,10 +599,14 @@ static int guc_ct_send(struct xe_guc_ct *ct, const u32 *action, u32 len,
>>
>> XE_BUG_ON(g2h_len && g2h_fence);
>>
>> + xe_device_mem_access_get(ct_to_xe(ct));
>> +
>> mutex_lock(&ct->lock);
>> ret = guc_ct_send_locked(ct, action, len, g2h_len, num_g2h, g2h_fence);
>> mutex_unlock(&ct->lock);
>>
>> + xe_device_mem_access_put(ct_to_xe(ct));
>> +
>> return ret;
>> }
>>
>> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
>> index b7b57f10ba25..b2ffa001e6f7 100644
>> --- a/drivers/gpu/drm/xe/xe_pm.c
>> +++ b/drivers/gpu/drm/xe/xe_pm.c
>> @@ -210,14 +210,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..1b4c15b5e71a 100644
>> --- a/drivers/gpu/drm/xe/xe_pm.h
>> +++ b/drivers/gpu/drm/xe/xe_pm.h
>> @@ -19,7 +19,7 @@ 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);
>>
>> #endif
>> --
>> 2.40.0
>>
More information about the Intel-xe
mailing list