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. 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@xxxxxxx>; Grodzovsky, Andrey > <Andrey.Grodzovsky@xxxxxxx>; Daniel Vetter <daniel@xxxxxxxx>; Chen, JingWen > <JingWen.Chen2@xxxxxxx> > Cc: DRI Development <dri-devel@xxxxxxxxxxxxxxxxxxxxx>; > amd-gfx@xxxxxxxxxxxxxxxxxxxxx > 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 > > ------------------------------------------ > > >