[PATCH v2] drm/scheduler: Fix hang when sched_entity released

[PATCH v2] drm/scheduler: Fix hang when sched_entity released

[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@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;
 	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);
+
+			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);
 
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2] drm/scheduler: Fix hang when sched_entity released

[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@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;
 	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);
+
+			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);
 
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2] drm/scheduler: Fix hang when sched_entity released

[Christian König]

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@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.

> +
> +			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);
>   

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/scheduler: Fix hang when sched_entity released

[Christian König]

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@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.

> +
> +			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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2] drm/scheduler: Fix hang when sched_entity released

[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@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.

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);
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/scheduler: Fix hang when sched_entity released

[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@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.

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2] drm/scheduler: Fix hang when sched_entity released

[Christian König]

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@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.

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/scheduler: Fix hang when sched_entity released

[Christian König]

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@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.

>
> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2] drm/scheduler: Fix hang when sched_entity released

[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@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.

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);
>>>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/scheduler: Fix hang when sched_entity released

[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@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.

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2] drm/scheduler: Fix hang when sched_entity released

[Andrey Grodzovsky]

Ping

Andrey

On 2/18/21 11:41 AM, Andrey Grodzovsky wrote:
>
> 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@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.
>
> 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);
>>>>
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/scheduler: Fix hang when sched_entity released

[Andrey Grodzovsky]

Ping

Andrey

On 2/18/21 11:41 AM, Andrey Grodzovsky wrote:
>
> 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@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.
>
> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2] drm/scheduler: Fix hang when sched_entity released

[Christian König]

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@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.

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/scheduler: Fix hang when sched_entity released

[Christian König]

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@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.

>
> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2] drm/scheduler: Fix hang when sched_entity released

[Andrey Grodzovsky]

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@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);
>>>>>
>>>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/scheduler: Fix hang when sched_entity released

[Andrey Grodzovsky]

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@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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2] drm/scheduler: Fix hang when sched_entity released

[Andrey Grodzovsky]

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@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@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Candrey.grodzovsky%40amd.com%7Cd948a126eeb84abf3e1308d8d598ca2f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637494199501121764%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=SAWjfsFZ%2BTi6jKVcyJtIC9qM8EgGthmt3MnSFlxbut8%3D&amp;reserved=0 
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/scheduler: Fix hang when sched_entity released

[Andrey Grodzovsky]

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@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@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Candrey.grodzovsky%40amd.com%7Cd948a126eeb84abf3e1308d8d598ca2f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637494199501121764%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=SAWjfsFZ%2BTi6jKVcyJtIC9qM8EgGthmt3MnSFlxbut8%3D&amp;reserved=0 
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2] drm/scheduler: Fix hang when sched_entity released

[Andrey Grodzovsky]

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@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);
>>>>>>
>>>>
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/scheduler: Fix hang when sched_entity released

[Andrey Grodzovsky]

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@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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2] drm/scheduler: Fix hang when sched_entity released

[Christian König]

Am 24.02.21 um 16:13 schrieb Andrey Grodzovsky:
> Ping

Sorry, I've been on vacation this week.

>
> 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@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 ?

Yes, I think that should work.

Christian.


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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/scheduler: Fix hang when sched_entity released

[Christian König]

Am 24.02.21 um 16:13 schrieb Andrey Grodzovsky:
> Ping

Sorry, I've been on vacation this week.

>
> 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@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 ?

Yes, I think that should work.

Christian.


>>
>> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2] drm/scheduler: Fix hang when sched_entity released

[Andrey Grodzovsky]

On 2021-02-25 2:53 a.m., Christian König wrote:
> Am 24.02.21 um 16:13 schrieb Andrey Grodzovsky:
>> Ping
>
> Sorry, I've been on vacation this week.
>
>>
>> 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@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 ?
>
> Yes, I think that should work.


In this case reverse locking order is created, In 
drm_sched_entity_push_job and drm_sched_entity_flush lock 
entity->rq_lock locked first and rq->lock locked second. In 
drm_sched_fini on the other hand, I need to lock rq->lock first to 
iterate rq->entities and then lock s_entity->rq_lock for each entity to 
modify s_entity->stopped. I guess we could change s_entity->stopped to 
atomic ?

Andrey


>
> Christian.
>
>
>>>
>>> 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);
>>>>>>>>
>>>>>>
>>>>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/scheduler: Fix hang when sched_entity released

[Andrey Grodzovsky]

On 2021-02-25 2:53 a.m., Christian König wrote:
> Am 24.02.21 um 16:13 schrieb Andrey Grodzovsky:
>> Ping
>
> Sorry, I've been on vacation this week.
>
>>
>> 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@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 ?
>
> Yes, I think that should work.


In this case reverse locking order is created, In 
drm_sched_entity_push_job and drm_sched_entity_flush lock 
entity->rq_lock locked first and rq->lock locked second. In 
drm_sched_fini on the other hand, I need to lock rq->lock first to 
iterate rq->entities and then lock s_entity->rq_lock for each entity to 
modify s_entity->stopped. I guess we could change s_entity->stopped to 
atomic ?

Andrey


>
> Christian.
>
>
>>>
>>> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2] drm/scheduler: Fix hang when sched_entity released

[Christian König]

Am 25.02.21 um 17:03 schrieb Andrey Grodzovsky:
>
> On 2021-02-25 2:53 a.m., Christian König wrote:
>> Am 24.02.21 um 16:13 schrieb Andrey Grodzovsky:
>>> Ping
>>
>> Sorry, I've been on vacation this week.
>>
>>>
>>> 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@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 ?
>>
>> Yes, I think that should work.
>
>
> In this case reverse locking order is created, In 
> drm_sched_entity_push_job and drm_sched_entity_flush lock 
> entity->rq_lock locked first and rq->lock locked second. In 
> drm_sched_fini on the other hand, I need to lock rq->lock first to 
> iterate rq->entities and then lock s_entity->rq_lock for each entity 
> to modify s_entity->stopped. I guess we could change s_entity->stopped 
> to atomic ?

Good point. But I think the memory barrier inserted by 
wake_up_all(&sched->job_scheduled); should be sufficient.

If I see this correctly we have the entity->rq_lock mainly to protect 
concurrent changes of entity->rq.

But when two CPUs both set entity->stopped to true at the same time we 
don't really care about it as long drm_sched_entity_is_idle() sees it.

Regards,
Christian.

>
> Andrey
>
>
>>
>> Christian.
>>
>>
>>>>
>>>> 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);
>>>>>>>>>
>>>>>>>
>>>>>
>>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/scheduler: Fix hang when sched_entity released

[Christian König]

Am 25.02.21 um 17:03 schrieb Andrey Grodzovsky:
>
> On 2021-02-25 2:53 a.m., Christian König wrote:
>> Am 24.02.21 um 16:13 schrieb Andrey Grodzovsky:
>>> Ping
>>
>> Sorry, I've been on vacation this week.
>>
>>>
>>> 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@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 ?
>>
>> Yes, I think that should work.
>
>
> In this case reverse locking order is created, In 
> drm_sched_entity_push_job and drm_sched_entity_flush lock 
> entity->rq_lock locked first and rq->lock locked second. In 
> drm_sched_fini on the other hand, I need to lock rq->lock first to 
> iterate rq->entities and then lock s_entity->rq_lock for each entity 
> to modify s_entity->stopped. I guess we could change s_entity->stopped 
> to atomic ?

Good point. But I think the memory barrier inserted by 
wake_up_all(&sched->job_scheduled); should be sufficient.

If I see this correctly we have the entity->rq_lock mainly to protect 
concurrent changes of entity->rq.

But when two CPUs both set entity->stopped to true at the same time we 
don't really care about it as long drm_sched_entity_is_idle() sees it.

Regards,
Christian.

>
> Andrey
>
>
>>
>> Christian.
>>
>>
>>>>
>>>> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2] drm/scheduler: Fix hang when sched_entity released

[Andrey Grodzovsky]

On 2021-02-25 1:42 p.m., Christian König wrote:
>
>
> Am 25.02.21 um 17:03 schrieb Andrey Grodzovsky:
>>
>> On 2021-02-25 2:53 a.m., Christian König wrote:
>>> Am 24.02.21 um 16:13 schrieb Andrey Grodzovsky:
>>>> Ping
>>>
>>> Sorry, I've been on vacation this week.
>>>
>>>>
>>>> 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@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 ?
>>>
>>> Yes, I think that should work.
>>
>>
>> In this case reverse locking order is created, In 
>> drm_sched_entity_push_job and drm_sched_entity_flush lock 
>> entity->rq_lock locked first and rq->lock locked second. In 
>> drm_sched_fini on the other hand, I need to lock rq->lock first to 
>> iterate rq->entities and then lock s_entity->rq_lock for each entity 
>> to modify s_entity->stopped. I guess we could change 
>> s_entity->stopped to atomic ?
>
> Good point. But I think the memory barrier inserted by 
> wake_up_all(&sched->job_scheduled); should be sufficient.
>
> If I see this correctly we have the entity->rq_lock mainly to protect 
> concurrent changes of entity->rq.
>
> But when two CPUs both set entity->stopped to true at the same time we 
> don't really care about it as long drm_sched_entity_is_idle() sees it.
>
> Regards,
> Christian.


I was more thinking about integrity of reading/writing entity->stopped 
from different threads. I guess since it's bool it's guaranteed to be 
atomic from HW perspective anyway ?
Will send V3 soon.

Andrey


>
>>
>> Andrey
>>
>>
>>>
>>> Christian.
>>>
>>>
>>>>>
>>>>> 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);
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/scheduler: Fix hang when sched_entity released

[Andrey Grodzovsky]

On 2021-02-25 1:42 p.m., Christian König wrote:
>
>
> Am 25.02.21 um 17:03 schrieb Andrey Grodzovsky:
>>
>> On 2021-02-25 2:53 a.m., Christian König wrote:
>>> Am 24.02.21 um 16:13 schrieb Andrey Grodzovsky:
>>>> Ping
>>>
>>> Sorry, I've been on vacation this week.
>>>
>>>>
>>>> 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@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 ?
>>>
>>> Yes, I think that should work.
>>
>>
>> In this case reverse locking order is created, In 
>> drm_sched_entity_push_job and drm_sched_entity_flush lock 
>> entity->rq_lock locked first and rq->lock locked second. In 
>> drm_sched_fini on the other hand, I need to lock rq->lock first to 
>> iterate rq->entities and then lock s_entity->rq_lock for each entity 
>> to modify s_entity->stopped. I guess we could change 
>> s_entity->stopped to atomic ?
>
> Good point. But I think the memory barrier inserted by 
> wake_up_all(&sched->job_scheduled); should be sufficient.
>
> If I see this correctly we have the entity->rq_lock mainly to protect 
> concurrent changes of entity->rq.
>
> But when two CPUs both set entity->stopped to true at the same time we 
> don't really care about it as long drm_sched_entity_is_idle() sees it.
>
> Regards,
> Christian.


I was more thinking about integrity of reading/writing entity->stopped 
from different threads. I guess since it's bool it's guaranteed to be 
atomic from HW perspective anyway ?
Will send V3 soon.

Andrey


>
>>
>> Andrey
>>
>>
>>>
>>> Christian.
>>>
>>>
>>>>>
>>>>> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2] drm/scheduler: Fix hang when sched_entity released

[Christian König]

Am 25.02.21 um 22:27 schrieb Andrey Grodzovsky:
>
> On 2021-02-25 1:42 p.m., Christian König wrote:
>>
>>
>> Am 25.02.21 um 17:03 schrieb Andrey Grodzovsky:
>>>
>>> On 2021-02-25 2:53 a.m., Christian König wrote:
>>>> Am 24.02.21 um 16:13 schrieb Andrey Grodzovsky:
>>>>> Ping
>>>>
>>>> Sorry, I've been on vacation this week.
>>>>
>>>>>
>>>>> 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@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 ?
>>>>
>>>> Yes, I think that should work.
>>>
>>>
>>> In this case reverse locking order is created, In 
>>> drm_sched_entity_push_job and drm_sched_entity_flush lock 
>>> entity->rq_lock locked first and rq->lock locked second. In 
>>> drm_sched_fini on the other hand, I need to lock rq->lock first to 
>>> iterate rq->entities and then lock s_entity->rq_lock for each entity 
>>> to modify s_entity->stopped. I guess we could change 
>>> s_entity->stopped to atomic ?
>>
>> Good point. But I think the memory barrier inserted by 
>> wake_up_all(&sched->job_scheduled); should be sufficient.
>>
>> If I see this correctly we have the entity->rq_lock mainly to protect 
>> concurrent changes of entity->rq.
>>
>> But when two CPUs both set entity->stopped to true at the same time 
>> we don't really care about it as long drm_sched_entity_is_idle() sees 
>> it.
>>
>> Regards,
>> Christian.
>
>
> I was more thinking about integrity of reading/writing entity->stopped 
> from different threads. I guess since it's bool it's guaranteed to be 
> atomic from HW perspective anyway ?

More or less yes. The key point is that we only change it from false -> 
true and never the other way around. All that you need then for other 
CPUs to see the value is a barrier.

Christian.

> Will send V3 soon.
>
> Andrey
>
>
>>
>>>
>>> Andrey
>>>
>>>
>>>>
>>>> Christian.
>>>>
>>>>
>>>>>>
>>>>>> 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);
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>
>>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/scheduler: Fix hang when sched_entity released

[Christian König]

Am 25.02.21 um 22:27 schrieb Andrey Grodzovsky:
>
> On 2021-02-25 1:42 p.m., Christian König wrote:
>>
>>
>> Am 25.02.21 um 17:03 schrieb Andrey Grodzovsky:
>>>
>>> On 2021-02-25 2:53 a.m., Christian König wrote:
>>>> Am 24.02.21 um 16:13 schrieb Andrey Grodzovsky:
>>>>> Ping
>>>>
>>>> Sorry, I've been on vacation this week.
>>>>
>>>>>
>>>>> 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@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 ?
>>>>
>>>> Yes, I think that should work.
>>>
>>>
>>> In this case reverse locking order is created, In 
>>> drm_sched_entity_push_job and drm_sched_entity_flush lock 
>>> entity->rq_lock locked first and rq->lock locked second. In 
>>> drm_sched_fini on the other hand, I need to lock rq->lock first to 
>>> iterate rq->entities and then lock s_entity->rq_lock for each entity 
>>> to modify s_entity->stopped. I guess we could change 
>>> s_entity->stopped to atomic ?
>>
>> Good point. But I think the memory barrier inserted by 
>> wake_up_all(&sched->job_scheduled); should be sufficient.
>>
>> If I see this correctly we have the entity->rq_lock mainly to protect 
>> concurrent changes of entity->rq.
>>
>> But when two CPUs both set entity->stopped to true at the same time 
>> we don't really care about it as long drm_sched_entity_is_idle() sees 
>> it.
>>
>> Regards,
>> Christian.
>
>
> I was more thinking about integrity of reading/writing entity->stopped 
> from different threads. I guess since it's bool it's guaranteed to be 
> atomic from HW perspective anyway ?

More or less yes. The key point is that we only change it from false -> 
true and never the other way around. All that you need then for other 
CPUs to see the value is a barrier.

Christian.

> Will send V3 soon.
>
> Andrey
>
>
>>
>>>
>>> Andrey
>>>
>>>
>>>>
>>>> Christian.
>>>>
>>>>
>>>>>>
>>>>>> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx