Re: [PATCH] drm/scheduler: Add drm_sched_suspend/resume timeout functions

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Am 14.11.18 um 18:29 schrieb Sharat Masetty:
>
>
> On 11/8/2018 8:11 PM, Koenig, Christian wrote:
>> Am 08.11.18 um 14:42 schrieb Sharat Masetty:
>>> Hi Christian,
>>>
>>> Can you please review this patch? It is a continuation of the 
>>> discussion at [1].
>>> At first I was thinking of using a cancel for suspend instead of a 
>>> mod(to an
>>> arbitrarily large value), but I couldn't get it to fit in as I have 
>>> an additional
>>> constraint of being able to call these functions from an IRQ context.
>>>
>>> These new functions race with other contexts, primarily finish_job(),
>>> timedout_job() and recovery(), but I did go through the possible 
>>> races between
>>> these(I think). Please let me know what you think of this? I have 
>>> not tested
>>> this yet and if this is something in the right direction, I will put 
>>> this
>>> through my testing drill and polish it.
>>>
>>> IMO I think I prefer the callback approach as it appears to be 
>>> simple, less
>>> error prone for both the scheduler and the drivers.
>>
>> Well I agree that this is way to complicated and looks racy to me as
>> well. But this is because you moved away from my initial suggestion.
>>
>> So here is once more how to do it without any additional locks or races:
>>
>> /**
>>    * drm_sched_suspend_timeout - suspend timeout for reset worker
>>    *
>>    * @sched: scheduler instance for which to suspend the timeout
>>    *
>>    * Suspend the delayed work timeout for the scheduler. Note that
>>    * this function can be called from an IRQ context. It returns the
>> timeout remaining.
>>    */
>> unsigned long drm_sched_suspend_timeout(struct drm_gpu_scheduler *sched)
>> {
>>
>>     unsigned long timeout, current = jiffies
>>
>>     timeout = sched->work_tdr.timer.expires;
>>
>>     /*
>>      * Set timeout to an arbitrarily large value, this also prevents 
>> the timer to be
>>      * started when new submissions arrive.
>>      */
>>     if (mod_delayed_work(system_wq, &sched->work_tdr, sched->timeout 
>> * 10) &&
>>         time_after(timeout, current))
>>         return timeout - current;
>>     else
>>         return sched->timeout;
>> }
>>
>> /**
>>    * drm_sched_resume_timeout - resume timeout for reset worker
>>    *
>>    * @sched: scheduler instance for which to resume the timeout
>>    * @remaining: remaining timeout
>>    *
>>    * Resume the delayed work timeout for the scheduler. Note that
>>    * this function can be called from an IRQ context.
>>    */
>> void drm_sched_resume_timeout(struct drm_gpu_scheduler *sched, 
>> unsigned long remaining)
>> {
>>     if (list_empty(&sched->ring_mirror_list))
>>         cancel_delayed_work(&sched->work_tdr);
>>     else
>>         mod_delayed_work(system_wq, &sched->work_tdr, remaining);
>> }
> Hi Christian,
>
> Thank you for the suggestions - I was able to try this out this week. 
> It works for the most part, but there are a couple of races which need 
> further considerations.
>
> 1) The drm_sched_resume_timeout() can race with both the 
> drm_sched_job_finish() and also new job submissions. In the driver the 
> job submission which triggered the preemption can be complete as soon 
> as the switch happens and it is quite possible that I get the 
> preemption complete and the job done interrupt at the same time. So 
> this means that drm_sched_resume_timeout() in IRQ context can race 
> with drm_sched_job_finish() in worker thread context on another CPU. 
> Also in parallel new jobs can be submitted, which will also update the 
> ring mirror list . These races can be addressed however with locking 
> the job_list_lock inside the drm_sched_resume_timeout().

Yeah I know, but I considered this race harmless. Ok, thinking more 
about that I realized that this probably means that we could timeout a 
job way to early.

How about canceling the timer first and then using mod_delayed_work to 
set it to the remaining jiffies if there is a job running?

Otherwise as you noted as well the alternative is really to make the 
job_list_lock irq safe.

>
> 2) This one is a little more tricky - In the driver I start off with 
> all the timeouts suspended(except the one for the default ring), then 
> on every preemption interrupt I suspend the outgoing ring and resume 
> the incoming ring. I can only resume if it was previously suspended. 
> This is how it is set up. The problem that becomes apparent with this 
> approach is that for the suspended rings this arbitrarily large 
> timeout can fire at some point(because of no work) and just before 
> drm_sched_timedout_job() runs a new job can be inserted into the 
> mirror list. So in this case we get an incorrect timeout.
>
> What are your thoughts on using cancel_delayed_work() instead of mod 
> in suspend_timeout. Yes we will lose the benefits of mod, but we 
> should be able to synchronize drm_sched_suspend_timeout() and 
> drm_sched_start_timeout() with some basic state. I have not thought 
> this completely through so I may be missing something here.

Sounds simply like your arbitrary large timeout is not large enough or 
do I miss the problem here?

I just suggested regular timeout*10 because at least on our hardware we 
still want to have some timeout even if the ring is preempted, but you 
could also use MAX_SCHEDULE_TIMEOUT as well.

Christian.

>
> Please share your thoughts on this
>
> Thank you
>
> Sharat
>>
>>
>> Regards,
>> Christian.
>>
>>>
>>> [1]  https://patchwork.freedesktop.org/patch/259914/
>>>
>>> Signed-off-by: Sharat Masetty <smasetty@xxxxxxxxxxxxxx>
>>> ---
>>>    drivers/gpu/drm/scheduler/sched_main.c | 81 
>>> +++++++++++++++++++++++++++++++++-
>>>    include/drm/gpu_scheduler.h            |  5 +++
>>>    2 files changed, 85 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>> index c993d10..9645789 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>> @@ -191,11 +191,84 @@ bool drm_sched_dependency_optimized(struct 
>>> dma_fence* fence,
>>>     */
>>>    static void drm_sched_start_timeout(struct drm_gpu_scheduler *sched)
>>>    {
>>> +    unsigned long flags;
>>> +
>>> +    spin_lock_irqsave(&sched->tdr_suspend_lock, flags);
>>> +
>>> +    sched->timeout_remaining = sched->timeout;
>>> +
>>>        if (sched->timeout != MAX_SCHEDULE_TIMEOUT &&
>>> -        !list_empty(&sched->ring_mirror_list))
>>> +        !list_empty(&sched->ring_mirror_list) && 
>>> !sched->work_tdr_suspended)
>>>            schedule_delayed_work(&sched->work_tdr, sched->timeout);
>>> +
>>> +    spin_unlock_irqrestore(&sched->tdr_suspend_lock, flags);
>>>    }
>>>
>>> +/**
>>> + * drm_sched_suspend_timeout - suspend timeout for reset worker
>>> + *
>>> + * @sched: scheduler instance for which to suspend the timeout
>>> + *
>>> + * Suspend the delayed work timeout for the scheduler. Note that
>>> + * this function can be called from an IRQ context.
>>> + */
>>> +void drm_sched_suspend_timeout(struct drm_gpu_scheduler *sched)
>>> +{
>>> +    unsigned long flags, timeout;
>>> +
>>> +    spin_lock_irqsave(&sched->tdr_suspend_lock, flags);
>>> +
>>> +    if (sched->work_tdr_suspended ||
>>> +            sched->timeout == MAX_SCHEDULE_TIMEOUT ||
>>> +            list_empty(&sched->ring_mirror_list))
>>> +        goto done;
>>> +
>>> +    timeout = sched->work_tdr.timer.expires;
>>> +
>>> +    /*
>>> +     * Reset timeout to an arbitrarily large value
>>> +     */
>>> +    mod_delayed_work(system_wq, &sched->work_tdr, sched->timeout * 
>>> 10);
>>> +
>>> +    timeout -= jiffies;
>>> +
>>> +    /* FIXME: Can jiffies be after timeout? */
>>> +    sched->timeout_remaining = time_after(jiffies, timeout)? 0: 
>>> timeout;
>>> +    sched->work_tdr_suspended = true;
>>> +
>>> +done:
>>> +    spin_unlock_irqrestore(&sched->tdr_suspend_lock, flags);
>>> +}
>>> +EXPORT_SYMBOL(drm_sched_suspend_timeout);
>>> +
>>> +/**
>>> + * drm_sched_resume_timeout - resume timeout for reset worker
>>> + *
>>> + * @sched: scheduler instance for which to resume the timeout
>>> + *
>>> + * Resume the delayed work timeout for the scheduler. Note that
>>> + * this function can be called from an IRQ context.
>>> + */
>>> +void drm_sched_resume_timeout(struct drm_gpu_scheduler *sched)
>>> +{
>>> +    unsigned long flags;
>>> +
>>> +    spin_lock_irqsave(&sched->tdr_suspend_lock, flags);
>>> +
>>> +    if (!sched->work_tdr_suspended ||
>>> +            sched->timeout == MAX_SCHEDULE_TIMEOUT) {
>>> + spin_unlock_irqrestore(&sched->tdr_suspend_lock, flags);
>>> +        return;
>>> +    }
>>> +
>>> +    mod_delayed_work(system_wq, &sched->work_tdr, 
>>> sched->timeout_remaining);
>>> +
>>> +    sched->work_tdr_suspended = false;
>>> +
>>> +    spin_unlock_irqrestore(&sched->tdr_suspend_lock, flags);
>>> +}
>>> +EXPORT_SYMBOL(drm_sched_resume_timeout);
>>> +
>>>    /* job_finish is called after hw fence signaled
>>>     */
>>>    static void drm_sched_job_finish(struct work_struct *work)
>>> @@ -348,6 +421,11 @@ void drm_sched_job_recovery(struct 
>>> drm_gpu_scheduler *sched)
>>>        struct drm_sched_job *s_job, *tmp;
>>>        bool found_guilty = false;
>>>        int r;
>>> +    unsigned long flags;
>>> +
>>> +    spin_lock_irqsave(&sched->tdr_suspend_lock, flags);
>>> +    sched->work_tdr_suspended = false;
>>> +    spin_unlock_irqrestore(&sched->tdr_suspend_lock, flags);
>>>
>>>        spin_lock(&sched->job_list_lock);
>>>        list_for_each_entry_safe(s_job, tmp, 
>>> &sched->ring_mirror_list, node) {
>>> @@ -607,6 +685,7 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
>>>        init_waitqueue_head(&sched->job_scheduled);
>>>        INIT_LIST_HEAD(&sched->ring_mirror_list);
>>>        spin_lock_init(&sched->job_list_lock);
>>> +    spin_lock_init(&sched->tdr_suspend_lock);
>>>        atomic_set(&sched->hw_rq_count, 0);
>>>        INIT_DELAYED_WORK(&sched->work_tdr, drm_sched_job_timedout);
>>>        atomic_set(&sched->num_jobs, 0);
>>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>>> index d87b268..5d39572 100644
>>> --- a/include/drm/gpu_scheduler.h
>>> +++ b/include/drm/gpu_scheduler.h
>>> @@ -278,6 +278,9 @@ struct drm_gpu_scheduler {
>>>        atomic_t            hw_rq_count;
>>>        atomic64_t            job_id_count;
>>>        struct delayed_work        work_tdr;
>>> +    unsigned long            timeout_remaining;
>>> +    bool                work_tdr_suspended;
>>> +    spinlock_t            tdr_suspend_lock;
>>>        struct task_struct        *thread;
>>>        struct list_head        ring_mirror_list;
>>>        spinlock_t            job_list_lock;
>>> @@ -300,6 +303,8 @@ void drm_sched_hw_job_reset(struct 
>>> drm_gpu_scheduler *sched,
>>>    bool drm_sched_dependency_optimized(struct dma_fence* fence,
>>>                        struct drm_sched_entity *entity);
>>>    void drm_sched_job_kickout(struct drm_sched_job *s_job);
>>> +void drm_sched_suspend_timeout(struct drm_gpu_scheduler *sched);
>>> +void drm_sched_resume_timeout(struct drm_gpu_scheduler *sched);
>>>
>>>    void drm_sched_rq_add_entity(struct drm_sched_rq *rq,
>>>                     struct drm_sched_entity *entity);
>>> -- 
>>> 1.9.1
>>>
>>
>> _______________________________________________
>> Freedreno mailing list
>> Freedreno@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/freedreno
>>
>





[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux