Date:   Thu, 26 Sep 2019 17:13:12 +0000
From:   "Grodzovsky, Andrey" <Andrey.Grodzovsky@....com>
To:     Steven Price <steven.price@....com>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...ux.ie>,
        "Koenig, Christian" <Christian.Koenig@....com>
CC:     "Deucher, Alexander" <Alexander.Deucher@....com>,
        Nayan Deshmukh <nayan26deshmukh@...il.com>,
        Sharat Masetty <smasetty@...eaurora.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH v4] drm: Don't free jobs in wait_event_interruptible()


On 9/26/19 11:59 AM, Steven Price wrote:
> On 26/09/2019 16:48, Grodzovsky, Andrey wrote:
>> On 9/26/19 11:23 AM, Steven Price wrote:
>>> On 26/09/2019 16:14, Grodzovsky, Andrey wrote:
>>>> On 9/26/19 10:16 AM, Steven Price wrote:
>>>>> drm_sched_cleanup_jobs() attempts to free finished jobs, however because
>>>>> it is called as the condition of wait_event_interruptible() it must not
>>>>> sleep. Unfortuantly some free callbacks (notibly for Panfrost) do sleep.
>>>>>
>>>>> Instead let's rename drm_sched_cleanup_jobs() to
>>>>> drm_sched_get_cleanup_job() and simply return a job for processing if
>>>>> there is one. The caller can then call the free_job() callback outside
>>>>> the wait_event_interruptible() where sleeping is possible before
>>>>> re-checking and returning to sleep if necessary.
>>>>>
>>>>> Signed-off-by: Steven Price <steven.price@....com>
>>>>> ---
>>>>> Changes from v3:
>>>>>     * drm_sched_main() re-arms the timeout for the next job after calling
>>>>>       free_job()
>>>>>
>>>>>     drivers/gpu/drm/scheduler/sched_main.c | 45 +++++++++++++++-----------
>>>>>     1 file changed, 26 insertions(+), 19 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
>>>>> index 9a0ee74d82dc..148468447ba9 100644
>>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>>> @@ -622,43 +622,41 @@ static void drm_sched_process_job(struct dma_fence *f, struct dma_fence_cb *cb)
>>>>>     }
>>>>>     
>>>>>     /**
>>>>> - * drm_sched_cleanup_jobs - destroy finished jobs
>>>>> + * drm_sched_get_cleanup_job - fetch the next finished job to be destroyed
>>>>>      *
>>>>>      * @sched: scheduler instance
>>>>>      *
>>>>> - * Remove all finished jobs from the mirror list and destroy them.
>>>>> + * Returns the next finished job from the mirror list (if there is one)
>>>>> + * ready for it to be destroyed.
>>>>>      */
>>>>> -static void drm_sched_cleanup_jobs(struct drm_gpu_scheduler *sched)
>>>>> +static struct drm_sched_job *
>>>>> +drm_sched_get_cleanup_job(struct drm_gpu_scheduler *sched)
>>>>>     {
>>>>> +	struct drm_sched_job *job = NULL;
>>>>>     	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))
>>>>> -		return;
>>>>> -
>>>>> +		return NULL;
>>>>>     
>>>>> -	while (!list_empty(&sched->ring_mirror_list)) {
>>>>> -		struct drm_sched_job *job;
>>>>> +	spin_lock_irqsave(&sched->job_list_lock, flags);
>>>>>     
>>>>> -		job = list_first_entry(&sched->ring_mirror_list,
>>>>> +	job = list_first_entry_or_null(&sched->ring_mirror_list,
>>>>>     				       struct drm_sched_job, node);
>>>>> -		if (!dma_fence_is_signaled(&job->s_fence->finished))
>>>>> -			break;
>>>>>     
>>>>> -		spin_lock_irqsave(&sched->job_list_lock, flags);
>>>>> +	if (job && dma_fence_is_signaled(&job->s_fence->finished)) {
>>>>>     		/* remove job from ring_mirror_list */
>>>>>     		list_del_init(&job->node);
>>>>> -		spin_unlock_irqrestore(&sched->job_list_lock, flags);
>>>>> -
>>>>> -		sched->ops->free_job(job);
>>>>> +	} else {
>>>>> +		job = NULL;
>>>>> +		/* queue timeout for next job */
>>>>> +		drm_sched_start_timeout(sched);
>>>>>     	}
>>>>>     
>>>>> -	/* queue timeout for next job */
>>>>> -	spin_lock_irqsave(&sched->job_list_lock, flags);
>>>>> -	drm_sched_start_timeout(sched);
>>>>>     	spin_unlock_irqrestore(&sched->job_list_lock, flags);
>>>>>     
>>>>> +	return job;
>>>>>     }
>>>>>     
>>>>>     /**
>>>>> @@ -698,12 +696,21 @@ static int drm_sched_main(void *param)
>>>>>     		struct drm_sched_fence *s_fence;
>>>>>     		struct drm_sched_job *sched_job;
>>>>>     		struct dma_fence *fence;
>>>>> +		struct drm_sched_job *cleanup_job = NULL;
>>>>>     
>>>>>     		wait_event_interruptible(sched->wake_up_worker,
>>>>> -					 (drm_sched_cleanup_jobs(sched),
>>>>> +					 (cleanup_job = drm_sched_get_cleanup_job(sched)) ||
>>>>>     					 (!drm_sched_blocked(sched) &&
>>>>>     					  (entity = drm_sched_select_entity(sched))) ||
>>>>> -					 kthread_should_stop()));
>>>>> +					 kthread_should_stop());
>>>>> +
>>>>> +		while (cleanup_job) {
>>>>> +			sched->ops->free_job(cleanup_job);
>>>>> +			/* queue timeout for next job */
>>>>> +			drm_sched_start_timeout(sched);
>>>>> +
>>>>> +			cleanup_job = drm_sched_get_cleanup_job(sched);
>>>>> +		}
>>>> Why drm_sched_start_timeout is called both here and inside
>>>> drm_sched_get_cleanup_job ? And also why call it multiple times in the
>>>> loop instead of only once after the loop is done  ?
>>> Christian pointed out to be that the first thing
>>> drm_sched_get_cleanup_job does is call cancel_delayed_work(), and if
>>> that returns false then it bails out with a NULL return. So to actually
>>> get another job (if one exists) the timeout has to be restarted.
>>
>> For this case where timeout work already in progress note that
>> drm_sched_job_timedout restarts the timeout in it's end so it should be
>> ok to restart the timeout unconditionally inside
>> drm_sched_get_cleanup_job as it was done before.
> I may have misinterpreted Christian[1], but I interpreted the below as
> meaning that he'd prefer the caller (drm_sched_main()) to handle the
> drm_sched_start_timeout() in this case:
>
> On 26/09/2019 14:50, Koenig, Christian wrote:
>>> Alternatively the caller could manually re-arm the timeout after
>>> handling the job free.
>> I don't see anything that could go wrong immediately, but that is
>> probably the cleaner approach.
> [1]
> https://lore.kernel.org/lkml/d9b23ef8-f784-0bce-c34a-fa02002db1ea@amd.com/
>
> I wasn't entirely sure if it was safe to leave the timeout running while
> the job free was going on which would be the case if
> drm_sched_get_cleanup_job() re-armed unconditionally.
>
>>> It's also necessary to restart the timeout in the case where the return
>>> is NULL which is handled in the function itself.
>>>
>>> TBH I'm not sure whether this while loop is worth it - it may be better
>>> to replace it with simply:
>>>
>>> 	if (cleanup_job) {
>>> 		sched->ops->free_job(cleanup_job);
>>> 		/* queue timeout for next job */
>>> 		drm_sched_start_timeout(sched);
>>> 	}
>>>
>>> The outer loop would then handle the next call to
>>> drm_sched_get_cleanup_job() as necessary.
>>
>> What outer loop ?
> There's a loop round the code in drm_sched_main():
>
> 	while (!kthread_should_stop()) {
> 		[...]
> 		wait_event_interruptible(...)
>
> 		while (cleanup_job) {...}
>
> 		if (!entity)
> 			continue;
>
> 		[...]
> 	}
>
> So after handling the cleanup_job case, the outer loop will loop back
> round to the wait_event_interruptible() call. One other concern is
> whether it's possible for the thread to be signalled to stop before the
> clean up has been completed. The while loop I added ensures that all
> jobs are actually cleaned up before kthread_should_stop() is checked.
>
> Steve


Yes, seems to me only the inner while loop will guarantee proper free of 
all signaled jobs in mirorr list if the thread is stopped.

Andrey

Powered by blists - more mailing lists