[PATCH v2] drm/scheduler: Fix hang when sched_entity released
Andrey Grodzovsky
andrey.grodzovsky at amd.com
Mon Feb 22 13:35:07 UTC 2021
Ping
Andrey
On 2021-02-20 7:12 a.m., Andrey Grodzovsky wrote:
>
> On 2/20/21 3:38 AM, Christian König wrote:
>>
>>
>> Am 18.02.21 um 17:41 schrieb Andrey Grodzovsky:
>>>
>>> On 2/18/21 10:15 AM, Christian König wrote:
>>>> Am 18.02.21 um 16:05 schrieb Andrey Grodzovsky:
>>>>>
>>>>> On 2/18/21 3:07 AM, Christian König wrote:
>>>>>>
>>>>>>
>>>>>> Am 17.02.21 um 22:59 schrieb Andrey Grodzovsky:
>>>>>>> Problem: If scheduler is already stopped by the time sched_entity
>>>>>>> is released and entity's job_queue not empty I encountred
>>>>>>> a hang in drm_sched_entity_flush. This is because
>>>>>>> drm_sched_entity_is_idle
>>>>>>> never becomes false.
>>>>>>>
>>>>>>> Fix: In drm_sched_fini detach all sched_entities from the
>>>>>>> scheduler's run queues. This will satisfy drm_sched_entity_is_idle.
>>>>>>> Also wakeup all those processes stuck in sched_entity flushing
>>>>>>> as the scheduler main thread which wakes them up is stopped by now.
>>>>>>>
>>>>>>> v2:
>>>>>>> Reverse order of drm_sched_rq_remove_entity and marking
>>>>>>> s_entity as stopped to prevent reinserion back to rq due
>>>>>>> to race.
>>>>>>>
>>>>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky at amd.com>
>>>>>>> ---
>>>>>>> drivers/gpu/drm/scheduler/sched_main.c | 31
>>>>>>> +++++++++++++++++++++++++++++++
>>>>>>> 1 file changed, 31 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>> index 908b0b5..c6b7947 100644
>>>>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>> @@ -897,9 +897,40 @@ EXPORT_SYMBOL(drm_sched_init);
>>>>>>> */
>>>>>>> void drm_sched_fini(struct drm_gpu_scheduler *sched)
>>>>>>> {
>>>>>>> + int i;
>>>>>>> + struct drm_sched_entity *s_entity;
>>>>>>
>>>>>> BTW: Please order that so that i is declared last.
>>>>>>
>>>>>>> if (sched->thread)
>>>>>>> kthread_stop(sched->thread);
>>>>>>> + /* Detach all sched_entites from this scheduler once it's
>>>>>>> stopped */
>>>>>>> + for (i = DRM_SCHED_PRIORITY_COUNT - 1; i >=
>>>>>>> DRM_SCHED_PRIORITY_MIN; i--) {
>>>>>>> + struct drm_sched_rq *rq = &sched->sched_rq[i];
>>>>>>> +
>>>>>>> + if (!rq)
>>>>>>> + continue;
>>>>>>> +
>>>>>>> + /* Loop this way because rq->lock is taken in
>>>>>>> drm_sched_rq_remove_entity */
>>>>>>> + spin_lock(&rq->lock);
>>>>>>> + while ((s_entity = list_first_entry_or_null(&rq->entities,
>>>>>>> + struct drm_sched_entity,
>>>>>>> + list))) {
>>>>>>> + spin_unlock(&rq->lock);
>>>>>>> +
>>>>>>> + /* Prevent reinsertion and remove */
>>>>>>> + spin_lock(&s_entity->rq_lock);
>>>>>>> + s_entity->stopped = true;
>>>>>>> + drm_sched_rq_remove_entity(rq, s_entity);
>>>>>>> + spin_unlock(&s_entity->rq_lock);
>>>>>>
>>>>>> Well this spin_unlock/lock dance here doesn't look correct at all
>>>>>> now.
>>>>>>
>>>>>> Christian.
>>>>>
>>>>>
>>>>> In what way ? It's in the same same order as in other call sites
>>>>> (see drm_sched_entity_push_job and drm_sched_entity_flush).
>>>>> If i just locked rq->lock and did list_for_each_entry_safe while
>>>>> manually deleting entity->list instead of calling
>>>>> drm_sched_rq_remove_entity this still would not be possible as the
>>>>> order of lock acquisition between s_entity->rq_lock
>>>>> and rq->lock would be reverse compared to the call sites mentioned
>>>>> above.
>>>>
>>>> Ah, now I understand. You need this because
>>>> drm_sched_rq_remove_entity() will grab the rq lock again!
>>>>
>>>> Problem is now what prevents the entity from being destroyed while
>>>> you remove it?
>>>>
>>>> Christian.
>>>
>>> Right, well, since (unfortunately) sched_entity is part of
>>> amdgpu_ctx_entity and amdgpu_ctx_entity is refcounted
>>> there is a problem here that we don't increment amdgpu_ctx.refcount
>>> when assigning sched_entity
>>> to new rq (e.g. before drm_sched_rq_add_entity) and not decrement
>>> before removing. We do it for
>>> amdgpu_cs_parser.entity for example (in amdgpu_cs_parser_init and
>>> amdgpu_cs_parser_fini by
>>> calling amdgpu_ctx_get and amdgpu_ctx_put). But this seems a bit
>>> tricky due to all the drm_sched_entity_select_rq
>>> logic.
>>>
>>> Another, kind of a band aid fix, would probably be just locking
>>> amdgpu_ctx_mgr.lock around drm_sched_fini
>>> when finalizing the fence driver and around idr iteration in
>>> amdgpu_ctx_mgr_fini (which should be lock protected
>>> anyway as I see from other idr usages in the code) ... This should
>>> prevent this use after free.
>>
>> Puh, that's rather complicated as well. Ok let's look at it from the
>> other side for a moment.
>>
>> Why do we have to remove the entities from the rq in the first place?
>>
>> Wouldn't it be sufficient to just set all of them to stopped?
>>
>> Christian.
>
>
> And adding it as another condition in drm_sched_entity_is_idle ?
>
> Andrey
>
>
>>
>>>
>>> Andrey
>>>
>>>
>>>>
>>>>>
>>>>> Andrey
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>> +
>>>>>>> + spin_lock(&rq->lock);
>>>>>>> + }
>>>>>>> + spin_unlock(&rq->lock);
>>>>>>> +
>>>>>>> + }
>>>>>>> +
>>>>>>> + /* Wakeup everyone stuck in drm_sched_entity_flush for this
>>>>>>> scheduler */
>>>>>>> + wake_up_all(&sched->job_scheduled);
>>>>>>> +
>>>>>>> /* Confirm no work left behind accessing device structures */
>>>>>>> cancel_delayed_work_sync(&sched->work_tdr);
>>>>>>
>>>>
>>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7Candrey.grodzovsky%40amd.com%7Cd948a126eeb84abf3e1308d8d598ca2f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637494199501121764%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=SAWjfsFZ%2BTi6jKVcyJtIC9qM8EgGthmt3MnSFlxbut8%3D&reserved=0
>
More information about the amd-gfx
mailing list