Re: [diagnostic TDR mode patches] unify our solution opinions/suggestions in one thread

[Andrey Grodzovsky <andrey.grodzovsky@amd.com>]

On 2021-09-01 12:25 a.m., Jingwen Chen wrote:
> On Wed Sep 01, 2021 at 12:04:47AM -0400, Andrey Grodzovsky wrote:
>> I will answer everything here -
>>
>> On 2021-08-31 9:58 p.m., Liu, Monk wrote:
>>
>>
>>      [AMD Official Use Only]
>>
>>       
>>
>>      In the previous discussion, you guys stated that we should drop the
>>      “kthread_should_park” in cleanup_job.
>>
>>       
>>
>>      @@ -676,15 +676,6 @@ drm_sched_get_cleanup_job(struct drm_gpu_scheduler
>>      *sched)
>>
>>      {
>>
>>              struct drm_sched_job *job, *next;
>>
>>       
>>
>>      -       /*
>>
>>      -        * Don't destroy jobs while the timeout worker is running  OR
>>      thread
>>
>>      -        * is being parked and hence assumed to not touch pending_list
>>
>>      -        */
>>
>>      -       if ((sched->timeout != MAX_SCHEDULE_TIMEOUT &&
>>
>>      -           !cancel_delayed_work(&sched->work_tdr)) ||
>>
>>      -           kthread_should_park())
>>
>>      -               return NULL;
>>
>>       
>>
>>      But I suddenly have a question here: if return the timedout job no matter
>>      kthread_should_park() or not, then we are backing to the original problem
>>      again: that the timedout_job is suddenly signaling and cleanup_job still
>>      returns it to sched_main and job is freed while it is still handling by
>>      vendor’s timeout callback
>>
>>       
>>
>>      If we return NULL when kthread_should_park() in cleanup_job, we can prevent
>>      above scenario from happening: once a job is processed by job_timedout we
>>      can stop its scheduler, and after that even this job suddenly signaled the
>>      cleanup_job won’t return it so sched_main won’t free it in parallel …
>>
>>       
>>
>>      What do you think ?
>>
>>
>> Is your analysis above takes into account that you also submit
>> '[PATCH 2/2] drm/sched: serialize job_timeout and scheduler' then I don't see a
>> problem -
> Hi Andrey,
> Monk has talked to me and we agreed that as there're multiple opinions about the
> '[PATCH 2/2] drm/sched: serialize job_timeout and scheduler' and patch
> 1 is an independent patch to fix some error. So we should not take the patch 2 into
> analysis.
>
>> I think that as long as you put kthread_park(sched->thread) BEFORE
>> fetching next bad job from pending list (under spinlock) there is no
>> such issue as in the case you describe because this potential bad job
>> that became signaled will be removed from pending list before you
>> even fetch the next job and by the time you fetch it the scheduler
>> thread is already stopped anyway
>>
>> If you don't submit and we keep the removal hack for now then also no problem
>> because
>> we temporary remove the job we fetch for TDR from pending list under spinlock
>> exactly to avoid this race
>>
> So can you help review [PATCH 1/2] drm/sched: fix the bug of time out calculation(v3)?
> patch v3 keeps this kthread_should_park check.


But since in both cases looks like there is no danger of use after free
then I see no reason to keep kthread_should_park check.

Andrey


>
> Best Regards,
> JingWen
>>
>>      Thanks
>>
>>       
>>
>>      ------------------------------------------
>>
>>      Monk Liu | Cloud-GPU Core team
>>
>>      ------------------------------------------
>>
>>       
>>
>>      From: Liu, Monk
>>      Sent: Wednesday, September 1, 2021 9:23 AM
>>      To: Koenig, Christian <Christian.Koenig@amd.com>; Grodzovsky, Andrey
>>      <Andrey.Grodzovsky@amd.com>; Daniel Vetter <daniel@ffwll.ch>; Chen, JingWen
>>      <JingWen.Chen2@amd.com>
>>      Cc: DRI Development <dri-devel@lists.freedesktop.org>;
>>      amd-gfx@lists.freedesktop.org
>>      Subject: [diagnostic TDR mode patches] unify our solution opinions/
>>      suggestions in one thread
>>
>>       
>>
>>      [AMD Official Use Only]
>>
>>       
>>
>>      Hi Daniel/Christian/Andrey
>>
>>       
>>
>>      It looks the voice from you three are spread over those email floods to me,
>>      the feature we are working on (diagnostic TDR scheme) is pending there for
>>      more than 6 month (we started it from feb 2021).
>>
>>       
>>
>>      Honestly speaking the email ways that we are using now is not friendly and
>>      quite painful to me ….
>>
>>      Can we try to put all our opinions, suggestions, or even objects here
>>      together, let’s go through them one by one, it’s too hard for us to reply
>>      each email on different questions .
>>
>>       
>>
>>      For [PATCH 1/2] drm/sched: fix the bug of time out calculation(v4)
>>
>>       
>>
>>      This is a fixing patch on the timeout timer in scheduler, can we complete
>>      this one first ? it should already resolved all the questions and
>>      suggestions.
>>
>>
>> I have no objections for this one besides getting rid of the
>> kthread_should_park()) return null part,
>> if my answer above is not wrong then it seems superfluous to me
>>
>>
>>       
>>
>>      For [PATCH 2/2] drm/sched: serialize job_timeout and scheduler
>>
>>       
>>
>>      I think I already explained the questions raised by Daniel in other thread
>>      , regarding why I use __kthread_should_park()
>>
>>
>> Is this race free ? Can't the other thread execute kthread_park after the check
>> ?
>>
>>
>>      For other aspects, can we put all our opinion synthesized here ?
>>
>>
>> So to summarize from previous threads I think that the best solution
>> to the problem being solved in this patch is if we do HW fence embedding
>> at the drm_sched_job level instead of doing it only for amdgpu, and modifying
>> all
>> the drivers to support this we can both remove this hack and solve the race
>> against concurrent drm_sched_cleanup_jobs job freeing just by taking reference
>> to the hw fence of the job at the beginning of drm_sched_job_timedout
>>
>> If doing this refactoring for all the drivers is not an option now and you need
>> a quick
>> solution such as the serialization you do here then take into account other
>> concurrent
>> TDR handlers that might run, as mentioned before, without serializing against
>> other TDR handlers from other
>> schedulers you just race here against them, e.g. you parked it now but another
>> one in progress will unpark it as part of calling  drm_sched_start for other
>> rings.
>> So you either need a global lock or dedicated single threaded queue as Daniel
>> suggested.
>> At minimum we should change cancel_delayed_work in drm_sched_stop to
>> cancel_delayed_work_sync
>> to cancel and flush all concurrent TDRs and probably move it to the begining of
>> the function, after kthread_park
>> and before we start playing with the pending list.
>>
>> P.S One comment I had regarding single threaded queue is that in case we have
>> multiple TDR
>> arising from same event we will proceed to resetting multiple times - something
>> that with trylock
>> we mostly avoid today within amdgpu (see amdgpu_device_lock_adev and
>> amdgpu_device_lock_hive_adev)
>> Daniel mentioned it's not a problem but I still haven't understood why not.
>>
>> Andrey
>>
>>
>>       
>>
>>      Thanks !
>>
>>       
>>
>>      ------------------------------------------
>>
>>      Monk Liu | Cloud-GPU Core team
>>
>>      ------------------------------------------
>>
>>       
>>