[PATCH v3 1/2] drm/sched: Avoid job cleanup if sched thread is parked.

[PATCH v3 1/2] drm/sched: Avoid job cleanup if sched thread is parked.

[Andrey Grodzovsky]

When the sched thread is parked we assume ring_mirror_list is
not accessed from here.

Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/scheduler/sched_main.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index d4cc728..6774955 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -635,9 +635,13 @@ drm_sched_get_cleanup_job(struct drm_gpu_scheduler *sched)
 	struct drm_sched_job *job;
 	unsigned long flags;
 
-	/* Don't destroy jobs while the timeout worker is running */
-	if (sched->timeout != MAX_SCHEDULE_TIMEOUT &&
-	    !cancel_delayed_work(&sched->work_tdr))
+	/*
+	* Don't destroy jobs while the timeout worker is running  OR thread
+	* is being parked and hence assumed to not touch ring_mirror_list
+	*/
+	if ((sched->timeout != MAX_SCHEDULE_TIMEOUT &&
+	     !cancel_delayed_work(&sched->work_tdr)) ||
+	     __kthread_should_park(sched->thread))
 		return NULL;
 
 	spin_lock_irqsave(&sched->job_list_lock, flags);
-- 
2.7.4

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

[PATCH v3 1/2] drm/sched: Avoid job cleanup if sched thread is parked.

[Andrey Grodzovsky]

When the sched thread is parked we assume ring_mirror_list is
not accessed from here.

Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/scheduler/sched_main.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index d4cc728..6774955 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -635,9 +635,13 @@ drm_sched_get_cleanup_job(struct drm_gpu_scheduler *sched)
 	struct drm_sched_job *job;
 	unsigned long flags;
 
-	/* Don't destroy jobs while the timeout worker is running */
-	if (sched->timeout != MAX_SCHEDULE_TIMEOUT &&
-	    !cancel_delayed_work(&sched->work_tdr))
+	/*
+	* Don't destroy jobs while the timeout worker is running  OR thread
+	* is being parked and hence assumed to not touch ring_mirror_list
+	*/
+	if ((sched->timeout != MAX_SCHEDULE_TIMEOUT &&
+	     !cancel_delayed_work(&sched->work_tdr)) ||
+	     __kthread_should_park(sched->thread))
 		return NULL;
 
 	spin_lock_irqsave(&sched->job_list_lock, flags);
-- 
2.7.4

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

[PATCH v3 2/2] drm/scheduler: Avoid accessing freed bad job.

[Andrey Grodzovsky]

Problem:
Due to a race between drm_sched_cleanup_jobs in sched thread and
drm_sched_job_timedout in timeout work there is a possiblity that
bad job was already freed while still being accessed from the
timeout thread.

Fix:
Instead of just peeking at the bad job in the mirror list
remove it from the list under lock and then put it back later when
we are garanteed no race with main sched thread is possible which
is after the thread is parked.

v2: Lock around processing ring_mirror_list in drm_sched_cleanup_jobs.

v3: Rebase on top of drm-misc-next. v2 is not needed anymore as
drm_sched_cleanup_jobs already has a lock there.

Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
Tested-by: Emily Deng <Emily.Deng@amd.com>
---
 drivers/gpu/drm/scheduler/sched_main.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 6774955..a604dfa 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -284,10 +284,24 @@ static void drm_sched_job_timedout(struct work_struct *work)
 	unsigned long flags;
 
 	sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work);
+
+	/*
+	 * Protects against concurrent deletion in drm_sched_cleanup_jobs that
+	 * is already in progress.
+	 */
+	spin_lock_irqsave(&sched->job_list_lock, flags);
 	job = list_first_entry_or_null(&sched->ring_mirror_list,
 				       struct drm_sched_job, node);
 
 	if (job) {
+		/*
+		 * Remove the bad job so it cannot be freed by already in progress
+		 * drm_sched_cleanup_jobs. It will be reinsrted back after sched->thread
+		 * is parked at which point it's safe.
+		 */
+		list_del_init(&job->node);
+		spin_unlock_irqrestore(&sched->job_list_lock, flags);
+
 		job->sched->ops->timedout_job(job);
 
 		/*
@@ -298,6 +312,8 @@ static void drm_sched_job_timedout(struct work_struct *work)
 			job->sched->ops->free_job(job);
 			sched->free_guilty = false;
 		}
+	} else {
+		spin_unlock_irqrestore(&sched->job_list_lock, flags);
 	}
 
 	spin_lock_irqsave(&sched->job_list_lock, flags);
@@ -370,6 +386,19 @@ void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad)
 	kthread_park(sched->thread);
 
 	/*
+	 * Reinsert back the bad job here - now it's safe as drm_sched_cleanup_jobs
+	 * cannot race against us and release the bad job at this point - we parked
+	 * (waited for) any in progress (earlier) cleanups and any later ones will
+	 * bail out due to sched->thread being parked.
+	 */
+	if (bad && bad->sched == sched)
+		/*
+		 * Add at the head of the queue to reflect it was the earliest
+		 * job extracted.
+		 */
+		list_add(&bad->node, &sched->ring_mirror_list);
+
+	/*
 	 * Iterate the job list from later to  earlier one and either deactive
 	 * their HW callbacks or remove them from mirror list if they already
 	 * signaled.
-- 
2.7.4

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

[PATCH v3 2/2] drm/scheduler: Avoid accessing freed bad job.

[Andrey Grodzovsky]

Problem:
Due to a race between drm_sched_cleanup_jobs in sched thread and
drm_sched_job_timedout in timeout work there is a possiblity that
bad job was already freed while still being accessed from the
timeout thread.

Fix:
Instead of just peeking at the bad job in the mirror list
remove it from the list under lock and then put it back later when
we are garanteed no race with main sched thread is possible which
is after the thread is parked.

v2: Lock around processing ring_mirror_list in drm_sched_cleanup_jobs.

v3: Rebase on top of drm-misc-next. v2 is not needed anymore as
drm_sched_cleanup_jobs already has a lock there.

Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
Tested-by: Emily Deng <Emily.Deng@amd.com>
---
 drivers/gpu/drm/scheduler/sched_main.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 6774955..a604dfa 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -284,10 +284,24 @@ static void drm_sched_job_timedout(struct work_struct *work)
 	unsigned long flags;
 
 	sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work);
+
+	/*
+	 * Protects against concurrent deletion in drm_sched_cleanup_jobs that
+	 * is already in progress.
+	 */
+	spin_lock_irqsave(&sched->job_list_lock, flags);
 	job = list_first_entry_or_null(&sched->ring_mirror_list,
 				       struct drm_sched_job, node);
 
 	if (job) {
+		/*
+		 * Remove the bad job so it cannot be freed by already in progress
+		 * drm_sched_cleanup_jobs. It will be reinsrted back after sched->thread
+		 * is parked at which point it's safe.
+		 */
+		list_del_init(&job->node);
+		spin_unlock_irqrestore(&sched->job_list_lock, flags);
+
 		job->sched->ops->timedout_job(job);
 
 		/*
@@ -298,6 +312,8 @@ static void drm_sched_job_timedout(struct work_struct *work)
 			job->sched->ops->free_job(job);
 			sched->free_guilty = false;
 		}
+	} else {
+		spin_unlock_irqrestore(&sched->job_list_lock, flags);
 	}
 
 	spin_lock_irqsave(&sched->job_list_lock, flags);
@@ -370,6 +386,19 @@ void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad)
 	kthread_park(sched->thread);
 
 	/*
+	 * Reinsert back the bad job here - now it's safe as drm_sched_cleanup_jobs
+	 * cannot race against us and release the bad job at this point - we parked
+	 * (waited for) any in progress (earlier) cleanups and any later ones will
+	 * bail out due to sched->thread being parked.
+	 */
+	if (bad && bad->sched == sched)
+		/*
+		 * Add at the head of the queue to reflect it was the earliest
+		 * job extracted.
+		 */
+		list_add(&bad->node, &sched->ring_mirror_list);
+
+	/*
 	 * Iterate the job list from later to  earlier one and either deactive
 	 * their HW callbacks or remove them from mirror list if they already
 	 * signaled.
-- 
2.7.4

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

Re: [PATCH v3 1/2] drm/sched: Avoid job cleanup if sched thread is parked.

[Steven Price]

On 25/11/2019 14:10, Andrey Grodzovsky wrote:
> When the sched thread is parked we assume ring_mirror_list is
> not accessed from here.

FWIW I don't think this is necessary. kthread_park() will wait until the
thread is parked, at which point the thread is stuck in kthread_parkme()
until unparked.

So all this does is avoid waiting for any cleanup jobs before parking -
which might be a reasonable goal in itself, but if so lets at least
document that.

Steve

> 
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> Reviewed-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/scheduler/sched_main.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index d4cc728..6774955 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -635,9 +635,13 @@ drm_sched_get_cleanup_job(struct drm_gpu_scheduler *sched)
>  	struct drm_sched_job *job;
>  	unsigned long flags;
>  
> -	/* Don't destroy jobs while the timeout worker is running */
> -	if (sched->timeout != MAX_SCHEDULE_TIMEOUT &&
> -	    !cancel_delayed_work(&sched->work_tdr))
> +	/*
> +	* Don't destroy jobs while the timeout worker is running  OR thread
> +	* is being parked and hence assumed to not touch ring_mirror_list
> +	*/
> +	if ((sched->timeout != MAX_SCHEDULE_TIMEOUT &&
> +	     !cancel_delayed_work(&sched->work_tdr)) ||
> +	     __kthread_should_park(sched->thread))
>  		return NULL;
>  
>  	spin_lock_irqsave(&sched->job_list_lock, flags);
> 

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

Re: [PATCH v3 2/2] drm/scheduler: Avoid accessing freed bad job.

[Steven Price]

On 25/11/2019 14:10, Andrey Grodzovsky wrote:
> Problem:
> Due to a race between drm_sched_cleanup_jobs in sched thread and
> drm_sched_job_timedout in timeout work there is a possiblity that
> bad job was already freed while still being accessed from the
> timeout thread.
> 
> Fix:
> Instead of just peeking at the bad job in the mirror list
> remove it from the list under lock and then put it back later when
> we are garanteed no race with main sched thread is possible which
> is after the thread is parked.
> 
> v2: Lock around processing ring_mirror_list in drm_sched_cleanup_jobs.
> 
> v3: Rebase on top of drm-misc-next. v2 is not needed anymore as
> drm_sched_cleanup_jobs already has a lock there.
> 
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> Reviewed-by: Christian König <christian.koenig@amd.com>
> Tested-by: Emily Deng <Emily.Deng@amd.com>
> ---
>  drivers/gpu/drm/scheduler/sched_main.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index 6774955..a604dfa 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -284,10 +284,24 @@ static void drm_sched_job_timedout(struct work_struct *work)
>  	unsigned long flags;
>  
>  	sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work);
> +
> +	/*
> +	 * Protects against concurrent deletion in drm_sched_cleanup_jobs that
                                                   ^^^^^^^^^^^^^^^^^^^^^

drm_sched_cleanup_jobs() is gone - replaced with
drm_sched_get_cleanup_job() and code in drm_sched_main(). See:

588b9828f074 ("drm: Don't free jobs in wait_event_interruptible()")

> +	 * is already in progress.
> +	 */
> +	spin_lock_irqsave(&sched->job_list_lock, flags);
>  	job = list_first_entry_or_null(&sched->ring_mirror_list,
>  				       struct drm_sched_job, node);
>  
>  	if (job) {
> +		/*
> +		 * Remove the bad job so it cannot be freed by already in progress
> +		 * drm_sched_cleanup_jobs. It will be reinsrted back after sched->thread
                                                      ^^^^^^^^^
Typo: s/reinsrted/reinserted/

> +		 * is parked at which point it's safe.
> +		 */
> +		list_del_init(&job->node);
> +		spin_unlock_irqrestore(&sched->job_list_lock, flags);
> +
>  		job->sched->ops->timedout_job(job);
>  
>  		/*
> @@ -298,6 +312,8 @@ static void drm_sched_job_timedout(struct work_struct *work)
>  			job->sched->ops->free_job(job);
>  			sched->free_guilty = false;
>  		}
> +	} else {
> +		spin_unlock_irqrestore(&sched->job_list_lock, flags);
>  	}
>  
>  	spin_lock_irqsave(&sched->job_list_lock, flags);
> @@ -370,6 +386,19 @@ void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad)
>  	kthread_park(sched->thread);
>  
>  	/*
> +	 * Reinsert back the bad job here - now it's safe as drm_sched_cleanup_jobs

Another reference to drm_sched_cleanup_jobs.

> +	 * cannot race against us and release the bad job at this point - we parked
> +	 * (waited for) any in progress (earlier) cleanups and any later ones will
> +	 * bail out due to sched->thread being parked.

As explained in the other patch - after the kthread_park() has returned
there will be no more calls to drm_sched_get_cleanup_job() until the
thread is unparked.

Steve

> +	 */
> +	if (bad && bad->sched == sched)
> +		/*
> +		 * Add at the head of the queue to reflect it was the earliest
> +		 * job extracted.
> +		 */
> +		list_add(&bad->node, &sched->ring_mirror_list);
> +
> +	/*
>  	 * Iterate the job list from later to  earlier one and either deactive
>  	 * their HW callbacks or remove them from mirror list if they already
>  	 * signaled.
> 

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

Re: [PATCH v3 2/2] drm/scheduler: Avoid accessing freed bad job.

[Steven Price]

On 25/11/2019 14:10, Andrey Grodzovsky wrote:
> Problem:
> Due to a race between drm_sched_cleanup_jobs in sched thread and
> drm_sched_job_timedout in timeout work there is a possiblity that
> bad job was already freed while still being accessed from the
> timeout thread.
> 
> Fix:
> Instead of just peeking at the bad job in the mirror list
> remove it from the list under lock and then put it back later when
> we are garanteed no race with main sched thread is possible which
> is after the thread is parked.
> 
> v2: Lock around processing ring_mirror_list in drm_sched_cleanup_jobs.
> 
> v3: Rebase on top of drm-misc-next. v2 is not needed anymore as
> drm_sched_cleanup_jobs already has a lock there.
> 
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> Reviewed-by: Christian König <christian.koenig@amd.com>
> Tested-by: Emily Deng <Emily.Deng@amd.com>
> ---
>  drivers/gpu/drm/scheduler/sched_main.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index 6774955..a604dfa 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -284,10 +284,24 @@ static void drm_sched_job_timedout(struct work_struct *work)
>  	unsigned long flags;
>  
>  	sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work);
> +
> +	/*
> +	 * Protects against concurrent deletion in drm_sched_cleanup_jobs that
                                                   ^^^^^^^^^^^^^^^^^^^^^

drm_sched_cleanup_jobs() is gone - replaced with
drm_sched_get_cleanup_job() and code in drm_sched_main(). See:

588b9828f074 ("drm: Don't free jobs in wait_event_interruptible()")

> +	 * is already in progress.
> +	 */
> +	spin_lock_irqsave(&sched->job_list_lock, flags);
>  	job = list_first_entry_or_null(&sched->ring_mirror_list,
>  				       struct drm_sched_job, node);
>  
>  	if (job) {
> +		/*
> +		 * Remove the bad job so it cannot be freed by already in progress
> +		 * drm_sched_cleanup_jobs. It will be reinsrted back after sched->thread
                                                      ^^^^^^^^^
Typo: s/reinsrted/reinserted/

> +		 * is parked at which point it's safe.
> +		 */
> +		list_del_init(&job->node);
> +		spin_unlock_irqrestore(&sched->job_list_lock, flags);
> +
>  		job->sched->ops->timedout_job(job);
>  
>  		/*
> @@ -298,6 +312,8 @@ static void drm_sched_job_timedout(struct work_struct *work)
>  			job->sched->ops->free_job(job);
>  			sched->free_guilty = false;
>  		}
> +	} else {
> +		spin_unlock_irqrestore(&sched->job_list_lock, flags);
>  	}
>  
>  	spin_lock_irqsave(&sched->job_list_lock, flags);
> @@ -370,6 +386,19 @@ void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad)
>  	kthread_park(sched->thread);
>  
>  	/*
> +	 * Reinsert back the bad job here - now it's safe as drm_sched_cleanup_jobs

Another reference to drm_sched_cleanup_jobs.

> +	 * cannot race against us and release the bad job at this point - we parked
> +	 * (waited for) any in progress (earlier) cleanups and any later ones will
> +	 * bail out due to sched->thread being parked.

As explained in the other patch - after the kthread_park() has returned
there will be no more calls to drm_sched_get_cleanup_job() until the
thread is unparked.

Steve

> +	 */
> +	if (bad && bad->sched == sched)
> +		/*
> +		 * Add at the head of the queue to reflect it was the earliest
> +		 * job extracted.
> +		 */
> +		list_add(&bad->node, &sched->ring_mirror_list);
> +
> +	/*
>  	 * Iterate the job list from later to  earlier one and either deactive
>  	 * their HW callbacks or remove them from mirror list if they already
>  	 * signaled.
> 

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

Re: [PATCH v3 1/2] drm/sched: Avoid job cleanup if sched thread is parked.

[Christian König]

Am 25.11.19 um 17:51 schrieb Steven Price:
> On 25/11/2019 14:10, Andrey Grodzovsky wrote:
>> When the sched thread is parked we assume ring_mirror_list is
>> not accessed from here.
> FWIW I don't think this is necessary. kthread_park() will wait until the
> thread is parked, at which point the thread is stuck in kthread_parkme()
> until unparked.
>
> So all this does is avoid waiting for any cleanup jobs before parking -
> which might be a reasonable goal in itself, but if so lets at least
> document that.

Now that you mention it that is indeed wrong.

The real problem is that in the main thread we mangled the call to 
kthread_parkme() into drm_sched_blocked() which can be called in atomic 
context.

I suggest to rework this so that the kthread_should_park() and 
kthread_should_stop() test in wait_event_interruptible() come first and 
then call kthread_parkme() outside of the wait_event_interruptible().

Regards,
Christian.

>
> Steve
>
>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>> Reviewed-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/scheduler/sched_main.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
>> index d4cc728..6774955 100644
>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>> @@ -635,9 +635,13 @@ drm_sched_get_cleanup_job(struct drm_gpu_scheduler *sched)
>>   	struct drm_sched_job *job;
>>   	unsigned long flags;
>>   
>> -	/* Don't destroy jobs while the timeout worker is running */
>> -	if (sched->timeout != MAX_SCHEDULE_TIMEOUT &&
>> -	    !cancel_delayed_work(&sched->work_tdr))
>> +	/*
>> +	* Don't destroy jobs while the timeout worker is running  OR thread
>> +	* is being parked and hence assumed to not touch ring_mirror_list
>> +	*/
>> +	if ((sched->timeout != MAX_SCHEDULE_TIMEOUT &&
>> +	     !cancel_delayed_work(&sched->work_tdr)) ||
>> +	     __kthread_should_park(sched->thread))
>>   		return NULL;
>>   
>>   	spin_lock_irqsave(&sched->job_list_lock, flags);
>>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

Re: [PATCH v3 1/2] drm/sched: Avoid job cleanup if sched thread is parked.

[Andrey Grodzovsky]

On 11/26/19 4:08 AM, Christian König wrote:
> Am 25.11.19 um 17:51 schrieb Steven Price:
>> On 25/11/2019 14:10, Andrey Grodzovsky wrote:
>>> When the sched thread is parked we assume ring_mirror_list is
>>> not accessed from here.
>> FWIW I don't think this is necessary. kthread_park() will wait until the
>> thread is parked, at which point the thread is stuck in kthread_parkme()
>> until unparked.
>>
>> So all this does is avoid waiting for any cleanup jobs before parking -
>> which might be a reasonable goal in itself, but if so lets at least
>> document that.
>
> Now that you mention it that is indeed wrong.


I wouldn't s call it wrong but superfluous in current code as indeed 
once the thread is parked there will be no subsequent calls to 
drm_sched_get_cleanup_job until the thread is unpacked back, if for 
example we decide to call drm_sched_get_cleanup_job from a work item 
which keeps scheduled it would be needed.


>
> The real problem is that in the main thread we mangled the call to 
> kthread_parkme() into drm_sched_blocked() which can be called in 
> atomic context.


Where is the atomic context in wait_event_interruptible ? I seem no to 
see any.

Andrey


>
> I suggest to rework this so that the kthread_should_park() and 
> kthread_should_stop() test in wait_event_interruptible() come first 
> and then call kthread_parkme() outside of the wait_event_interruptible().
>
> Regards,
> Christian.
>
>>
>> Steve
>>
>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>   drivers/gpu/drm/scheduler/sched_main.c | 10 +++++++---
>>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>> index d4cc728..6774955 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>> @@ -635,9 +635,13 @@ drm_sched_get_cleanup_job(struct 
>>> drm_gpu_scheduler *sched)
>>>       struct drm_sched_job *job;
>>>       unsigned long flags;
>>>   -    /* Don't destroy jobs while the timeout worker is running */
>>> -    if (sched->timeout != MAX_SCHEDULE_TIMEOUT &&
>>> -        !cancel_delayed_work(&sched->work_tdr))
>>> +    /*
>>> +    * Don't destroy jobs while the timeout worker is running OR thread
>>> +    * is being parked and hence assumed to not touch ring_mirror_list
>>> +    */
>>> +    if ((sched->timeout != MAX_SCHEDULE_TIMEOUT &&
>>> +         !cancel_delayed_work(&sched->work_tdr)) ||
>>> +         __kthread_should_park(sched->thread))
>>>           return NULL;
>>>         spin_lock_irqsave(&sched->job_list_lock, flags);
>>>
>> _______________________________________________
>> 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=02%7C01%7Candrey.grodzovsky%40amd.com%7Ce81a824a5f984f51bc1908d772503a25%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637103561221298691&amp;sdata=tW9Xupt7ascVPOlHxH0pHGcbUArVyTa5VTle016AcGg%3D&amp;reserved=0 
>>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/sched: Avoid job cleanup if sched thread is parked.

[Andrey Grodzovsky]

Ping...

Andrey

On 11/26/19 10:36 AM, Andrey Grodzovsky wrote:
>
> On 11/26/19 4:08 AM, Christian König wrote:
>> Am 25.11.19 um 17:51 schrieb Steven Price:
>>> On 25/11/2019 14:10, Andrey Grodzovsky wrote:
>>>> When the sched thread is parked we assume ring_mirror_list is
>>>> not accessed from here.
>>> FWIW I don't think this is necessary. kthread_park() will wait until 
>>> the
>>> thread is parked, at which point the thread is stuck in 
>>> kthread_parkme()
>>> until unparked.
>>>
>>> So all this does is avoid waiting for any cleanup jobs before parking -
>>> which might be a reasonable goal in itself, but if so lets at least
>>> document that.
>>
>> Now that you mention it that is indeed wrong.
>
>
> I wouldn't s call it wrong but superfluous in current code as indeed 
> once the thread is parked there will be no subsequent calls to 
> drm_sched_get_cleanup_job until the thread is unpacked back, if for 
> example we decide to call drm_sched_get_cleanup_job from a work item 
> which keeps scheduled it would be needed.
>
>
>>
>> The real problem is that in the main thread we mangled the call to 
>> kthread_parkme() into drm_sched_blocked() which can be called in 
>> atomic context.
>
>
> Where is the atomic context in wait_event_interruptible ? I seem no to 
> see any.
>
> Andrey
>
>
>>
>> I suggest to rework this so that the kthread_should_park() and 
>> kthread_should_stop() test in wait_event_interruptible() come first 
>> and then call kthread_parkme() outside of the 
>> wait_event_interruptible().
>>
>> Regards,
>> Christian.
>>
>>>
>>> Steve
>>>
>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/scheduler/sched_main.c | 10 +++++++---
>>>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>> index d4cc728..6774955 100644
>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>> @@ -635,9 +635,13 @@ drm_sched_get_cleanup_job(struct 
>>>> drm_gpu_scheduler *sched)
>>>>       struct drm_sched_job *job;
>>>>       unsigned long flags;
>>>>   -    /* Don't destroy jobs while the timeout worker is running */
>>>> -    if (sched->timeout != MAX_SCHEDULE_TIMEOUT &&
>>>> -        !cancel_delayed_work(&sched->work_tdr))
>>>> +    /*
>>>> +    * Don't destroy jobs while the timeout worker is running OR 
>>>> thread
>>>> +    * is being parked and hence assumed to not touch ring_mirror_list
>>>> +    */
>>>> +    if ((sched->timeout != MAX_SCHEDULE_TIMEOUT &&
>>>> +         !cancel_delayed_work(&sched->work_tdr)) ||
>>>> +         __kthread_should_park(sched->thread))
>>>>           return NULL;
>>>>         spin_lock_irqsave(&sched->job_list_lock, flags);
>>>>
>>> _______________________________________________
>>> 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=02%7C01%7Candrey.grodzovsky%40amd.com%7Ce81a824a5f984f51bc1908d772503a25%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637103561221298691&amp;sdata=tW9Xupt7ascVPOlHxH0pHGcbUArVyTa5VTle016AcGg%3D&amp;reserved=0 
>>>
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/sched: Avoid job cleanup if sched thread is parked.

[Christian König]

Am 27.11.19 um 16:32 schrieb Andrey Grodzovsky:
> Ping...
>
> Andrey
>
> On 11/26/19 10:36 AM, Andrey Grodzovsky wrote:
>>
>> On 11/26/19 4:08 AM, Christian König wrote:
>>> Am 25.11.19 um 17:51 schrieb Steven Price:
>>>> On 25/11/2019 14:10, Andrey Grodzovsky wrote:
>>>>> When the sched thread is parked we assume ring_mirror_list is
>>>>> not accessed from here.
>>>> FWIW I don't think this is necessary. kthread_park() will wait 
>>>> until the
>>>> thread is parked, at which point the thread is stuck in 
>>>> kthread_parkme()
>>>> until unparked.
>>>>
>>>> So all this does is avoid waiting for any cleanup jobs before 
>>>> parking -
>>>> which might be a reasonable goal in itself, but if so lets at least
>>>> document that.
>>>
>>> Now that you mention it that is indeed wrong.
>>
>>
>> I wouldn't s call it wrong but superfluous in current code as indeed 
>> once the thread is parked there will be no subsequent calls to 
>> drm_sched_get_cleanup_job until the thread is unpacked back, if for 
>> example we decide to call drm_sched_get_cleanup_job from a work item 
>> which keeps scheduled it would be needed.
>>
>>
>>>
>>> The real problem is that in the main thread we mangled the call to 
>>> kthread_parkme() into drm_sched_blocked() which can be called in 
>>> atomic context.
>>
>>
>> Where is the atomic context in wait_event_interruptible ? I seem no 
>> to see any.

It's a rare event, but the check code in a wait_event_* macro might be 
called in atomic context.

That's also the reason why Steven Price came up with the following patch:
> commit 588b9828f0744ca13555c4a35cd0251ac8ad8ad2
> Author: Steven Price <steven.price@arm.com>
> Date:   Fri Oct 25 11:51:56 2019 +0100
>
>     drm: Don't free jobs in wait_event_interruptible()

BTW: I've just pushed the v4 of that patch to drm-misc-next.

Christian.

>>
>> Andrey
>>
>>
>>>
>>> I suggest to rework this so that the kthread_should_park() and 
>>> kthread_should_stop() test in wait_event_interruptible() come first 
>>> and then call kthread_parkme() outside of the 
>>> wait_event_interruptible().
>>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>> Steve
>>>>
>>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>>>> ---
>>>>>   drivers/gpu/drm/scheduler/sched_main.c | 10 +++++++---
>>>>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>>> index d4cc728..6774955 100644
>>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>>> @@ -635,9 +635,13 @@ drm_sched_get_cleanup_job(struct 
>>>>> drm_gpu_scheduler *sched)
>>>>>       struct drm_sched_job *job;
>>>>>       unsigned long flags;
>>>>>   -    /* Don't destroy jobs while the timeout worker is running */
>>>>> -    if (sched->timeout != MAX_SCHEDULE_TIMEOUT &&
>>>>> -        !cancel_delayed_work(&sched->work_tdr))
>>>>> +    /*
>>>>> +    * Don't destroy jobs while the timeout worker is running OR 
>>>>> thread
>>>>> +    * is being parked and hence assumed to not touch 
>>>>> ring_mirror_list
>>>>> +    */
>>>>> +    if ((sched->timeout != MAX_SCHEDULE_TIMEOUT &&
>>>>> +         !cancel_delayed_work(&sched->work_tdr)) ||
>>>>> +         __kthread_should_park(sched->thread))
>>>>>           return NULL;
>>>>>         spin_lock_irqsave(&sched->job_list_lock, flags);
>>>>>
>>>> _______________________________________________
>>>> 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=02%7C01%7Candrey.grodzovsky%40amd.com%7Ce81a824a5f984f51bc1908d772503a25%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637103561221298691&amp;sdata=tW9Xupt7ascVPOlHxH0pHGcbUArVyTa5VTle016AcGg%3D&amp;reserved=0 
>>>>
>>>

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

Re: [PATCH v3 1/2] drm/sched: Avoid job cleanup if sched thread is parked.

[Christian König]

Am 27.11.19 um 16:32 schrieb Andrey Grodzovsky:
> Ping...
>
> Andrey
>
> On 11/26/19 10:36 AM, Andrey Grodzovsky wrote:
>>
>> On 11/26/19 4:08 AM, Christian König wrote:
>>> Am 25.11.19 um 17:51 schrieb Steven Price:
>>>> On 25/11/2019 14:10, Andrey Grodzovsky wrote:
>>>>> When the sched thread is parked we assume ring_mirror_list is
>>>>> not accessed from here.
>>>> FWIW I don't think this is necessary. kthread_park() will wait 
>>>> until the
>>>> thread is parked, at which point the thread is stuck in 
>>>> kthread_parkme()
>>>> until unparked.
>>>>
>>>> So all this does is avoid waiting for any cleanup jobs before 
>>>> parking -
>>>> which might be a reasonable goal in itself, but if so lets at least
>>>> document that.
>>>
>>> Now that you mention it that is indeed wrong.
>>
>>
>> I wouldn't s call it wrong but superfluous in current code as indeed 
>> once the thread is parked there will be no subsequent calls to 
>> drm_sched_get_cleanup_job until the thread is unpacked back, if for 
>> example we decide to call drm_sched_get_cleanup_job from a work item 
>> which keeps scheduled it would be needed.
>>
>>
>>>
>>> The real problem is that in the main thread we mangled the call to 
>>> kthread_parkme() into drm_sched_blocked() which can be called in 
>>> atomic context.
>>
>>
>> Where is the atomic context in wait_event_interruptible ? I seem no 
>> to see any.

It's a rare event, but the check code in a wait_event_* macro might be 
called in atomic context.

That's also the reason why Steven Price came up with the following patch:
> commit 588b9828f0744ca13555c4a35cd0251ac8ad8ad2
> Author: Steven Price <steven.price@arm.com>
> Date:   Fri Oct 25 11:51:56 2019 +0100
>
>     drm: Don't free jobs in wait_event_interruptible()

BTW: I've just pushed the v4 of that patch to drm-misc-next.

Christian.

>>
>> Andrey
>>
>>
>>>
>>> I suggest to rework this so that the kthread_should_park() and 
>>> kthread_should_stop() test in wait_event_interruptible() come first 
>>> and then call kthread_parkme() outside of the 
>>> wait_event_interruptible().
>>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>> Steve
>>>>
>>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>>>> ---
>>>>>   drivers/gpu/drm/scheduler/sched_main.c | 10 +++++++---
>>>>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>>> index d4cc728..6774955 100644
>>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>>> @@ -635,9 +635,13 @@ drm_sched_get_cleanup_job(struct 
>>>>> drm_gpu_scheduler *sched)
>>>>>       struct drm_sched_job *job;
>>>>>       unsigned long flags;
>>>>>   -    /* Don't destroy jobs while the timeout worker is running */
>>>>> -    if (sched->timeout != MAX_SCHEDULE_TIMEOUT &&
>>>>> -        !cancel_delayed_work(&sched->work_tdr))
>>>>> +    /*
>>>>> +    * Don't destroy jobs while the timeout worker is running OR 
>>>>> thread
>>>>> +    * is being parked and hence assumed to not touch 
>>>>> ring_mirror_list
>>>>> +    */
>>>>> +    if ((sched->timeout != MAX_SCHEDULE_TIMEOUT &&
>>>>> +         !cancel_delayed_work(&sched->work_tdr)) ||
>>>>> +         __kthread_should_park(sched->thread))
>>>>>           return NULL;
>>>>>         spin_lock_irqsave(&sched->job_list_lock, flags);
>>>>>
>>>> _______________________________________________
>>>> 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=02%7C01%7Candrey.grodzovsky%40amd.com%7Ce81a824a5f984f51bc1908d772503a25%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637103561221298691&amp;sdata=tW9Xupt7ascVPOlHxH0pHGcbUArVyTa5VTle016AcGg%3D&amp;reserved=0 
>>>>
>>>

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