Hi guys,
I have modified the patch as V2 in other thread, would you help to review?
-----Original Message-----
From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Koenig, Christian
Sent: Monday, November 12, 2018 6:12 PM
To: Liu, Monk <Monk.Liu@xxxxxxx>; Huang, Trigger <Trigger.Huang@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
Subject: Re: [PATCH] drm/scheduler: Fix bad job be re-processed in TDR
Hi guys,
yeah that is the same problem I was trying to fix with drm/sched: fix timeout handling v2.
But this case is even more complicated because here it is the hypervisor or KFD which triggers the second reset.
Going to take another look at this today, Christian.
Am 12.11.18 um 10:19 schrieb Liu, Monk:
> Hi Christian
>
> Trigger's patch intends to fix the double signaling on dma_fence from below scenario:
>
> 1) Gpu_recovery(job = some job) invoked by guest TDR ==> the hang
> job's fence is set as -ECANCELD and fake signaled 2)GPU_recovery(job =
> NULL) again invoked by hypervisor or KFD, but the job of above step
> hasn't finished its "drm_sched_job_finish" routine which is kicked by
> queu_work(), due to the cause that the second gpu_recovery() is called
> prior to "drm_sched_job_finish" (so the job is not took out from the
> mirror_list, so there will be duplicated dma_fence_set_error on that
> job and give you warning messeage)
>
> /Monk
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of
> Huang, Trigger
> Sent: Monday, November 12, 2018 4:36 PM
> To: Koenig, Christian <Christian.Koenig@xxxxxxx>;
> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: RE: [PATCH] drm/scheduler: Fix bad job be re-processed in TDR
>
> Hi Christian
>
> Thanks for the correction before, I gave my explanation as below, would you help to check again, thanks in advance.
>
> The key thing here is, if a job’s fence is signaled already, then call dma_fence_set_error to its fence will lead to a kernel warning call trace static inline void dma_fence_set_error(struct dma_fence *fence,
> int error) {
> WARN_ON(test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)); # here is warning source
> WARN_ON(error >= 0 || error < -MAX_ERRNO);
>
> fence->error = error; }
>
>
> Then, let’s see a guilty job’s process flow in TDR.
> amdgpu_device_gpu_recover -> drm_sched_job_recovery -> for each job in ring_mirror_list :
> 1) dma_fence_set_error(&s_fence->finished, -ECANCELED) if this job is guilty
> 2) amdgpu_job_run: the guilty job will be skipped, and not submitted into to ring
> 3) drm_sched_process_job(guilty_job) -> drm_sched_fence_finished -> dma_fence_signal(&fence->finished) -> drm_sched_job_finish_cb -> schedule_work(&job->finish_work);
> Later, finish_work’s callback function, drm_sched_job_finish, will be called by work queue, and guilty job will finally be deleted from ring_mirror_list.
> But sometimes, amdgpu_device_gpu_recover will be called(from host FLR interrupt or KFD) again immediately, and at this moment, finish_work is not scheduled by work queue, and drm_sched_job_finish for the guilty job is not called yet, so this guilty job is still in the ring_mirror_list. But remember, the guilty job’s fence was signaled before (by drm_sched_fence_finished), followed by the TDR process, dma_fence_set_error will be called again to this guilty, then cause the kernel warning call trace.
>
> So the root cause is, the finish_work for each job is not scheduled quickly enough for GPU TDR scenario.
> Three ideas:
> 1), Instead of using the system global work queue, maybe we can create a dedicate work queue to manage the job finish things, but still can’t guarantee its finish_work’s execution in GPU TDR scenario
> 2), Wait for all the guilty jobs’ finish_work to be finished execution before return from drm_sched_job_recovery , but I think it will waste too much time.
> 3), to delete the guilty job from the ring_mirror_list directly after this job is processed in amdgpu_job_run(). amdgpu_job_run is the final entrance for a job to be submitted into the ring, if something wrong with this job:(finished->error < 0), then this job should be never be submitted again, and so should be deleted from the recovery list.
> I choose 3).
>
>
> I am going to make a new patch as below instead of the original one
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> index e0af44f..aaf697f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> @@ -208,6 +208,7 @@ static struct dma_fence *amdgpu_job_dependency(struct drm_sched_job *sched_job, static struct dma_fence *amdgpu_job_run(struct drm_sched_job *sched_job) {
> struct amdgpu_ring *ring = to_amdgpu_ring(sched_job->sched);
> + struct drm_gpu_scheduler *sched = sched_job->sched;
> struct dma_fence *fence = NULL, *finished;
> struct amdgpu_job *job;
> int r;
> @@ -223,7 +224,10 @@ static struct dma_fence *amdgpu_job_run(struct drm_sched_job *sched_job)
> dma_fence_set_error(finished, -ECANCELED);/* skip IB
> as well if VRAM lost */
>
> if (finished->error < 0) {
> - DRM_INFO("Skip scheduling IBs!\n");
> + DRM_INFO("Skip scheduling IBs and delete job from recovery list!\n");
> + spin_lock(&sched->job_list_lock);
> + list_del_init(&sched_job->node);
> + spin_unlock(&sched->job_list_lock);
> } else {
> r = amdgpu_ib_schedule(ring, job->num_ibs, job->ibs, job,
> &fence);
>
>
> Thanks,
> Trigger
>
> -----Original Message-----
> From: Christian König <ckoenig.leichtzumerken@xxxxxxxxx>
> Sent: Friday, November 09, 2018 8:18 PM
> To: Huang, Trigger <Trigger.Huang@xxxxxxx>;
> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH] drm/scheduler: Fix bad job be re-processed in TDR
>
> Am 09.11.18 um 13:10 schrieb Trigger Huang:
>> A bad job is the one triggered TDR. In the recovery process, its
>> fence will be signaled and as a result, the work behind will be
>> scheduled to delete it from the mirror list, but if the TDR process
>> is invoked before the work's execution, then the call
>> dma_fence_set_error to its fence in TDR process will lead to kernel warning trace:
>>
>> [ 143.033605] WARNING: CPU: 2 PID: 53 at
>> ./include/linux/dma-fence.h:437 amddrm_sched_job_recovery+0x1af/0x1c0
>> [amd_sched]
>> kernel: [ 143.033606] Modules linked in: amdgpu(OE) amdchash(OE) amdttm(OE) amd_sched(OE) amdkcl(OE) amd_iommu_v2 drm_kms_helper drm i2c_algo_bit fb_sys_fops syscopyarea sysfillrect sysimgblt kvm_intel kvm irqbypass crct10dif_pclmul crc32_pclmul ghash_clmulni_intel pcbc aesni_intel aes_x86_64 snd_hda_codec_generic crypto_simd glue_helper cryptd snd_hda_intel snd_hda_codec snd_hda_core snd_hwdep snd_pcm snd_seq_midi snd_seq_midi_event snd_rawmidi snd_seq joydev snd_seq_device snd_timer snd soundcore binfmt_misc input_leds mac_hid serio_raw nfsd auth_rpcgss nfs_acl lockd grace sunrpc sch_fq_codel parport_pc ppdev lp parport ip_tables x_tables autofs4 8139too floppy psmouse 8139cp mii i2c_piix4 pata_acpi
>> [ 143.033649] CPU: 2 PID: 53 Comm: kworker/2:1 Tainted: G OE 4.15.0-20-generic #21-Ubuntu
>> [ 143.033650] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
>> BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014 [ 143.033653] Workqueue:
>> events drm_sched_job_timedout [amd_sched] [ 143.033656] RIP:
>> 0010:amddrm_sched_job_recovery+0x1af/0x1c0 [amd_sched] [ 143.033657]
>> RSP: 0018:ffffa9f880fe7d48 EFLAGS: 00010202 [ 143.033659] RAX:
>> 0000000000000007 RBX: ffff9b98f2b24c00 RCX: ffff9b98efef4f08 [
>> 143.033660] RDX: ffff9b98f2b27400 RSI: ffff9b98f2b24c50 RDI:
>> ffff9b98efef4f18 [ 143.033660] RBP: ffffa9f880fe7d98 R08:
>> 0000000000000001 R09: 00000000000002b6 [ 143.033661] R10:
>> 0000000000000000 R11: 0000000000000000 R12: ffff9b98efef3430 [
>> 143.033662] R13: ffff9b98efef4d80 R14: ffff9b98efef4e98 R15:
>> ffff9b98eaf91c00 [ 143.033663] FS: 0000000000000000(0000)
>> GS:ffff9b98ffd00000(0000) knlGS:0000000000000000 [ 143.033664] CS:
>> 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 143.033665] CR2:
>> 00007fc49c96d470 CR3: 000000001400a005 CR4: 00000000003606e0 [ 143.033669] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 143.033669] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 [ 143.033670] Call Trace:
>> [ 143.033744] amdgpu_device_gpu_recover+0x144/0x820 [amdgpu] [
>> 143.033788] amdgpu_job_timedout+0x9b/0xa0 [amdgpu] [ 143.033791]
>> drm_sched_job_timedout+0xcc/0x150 [amd_sched] [ 143.033795]
>> process_one_work+0x1de/0x410 [ 143.033797] worker_thread+0x32/0x410
>> [ 143.033799] kthread+0x121/0x140 [ 143.033801] ?
>> process_one_work+0x410/0x410 [ 143.033803] ?
>> kthread_create_worker_on_cpu+0x70/0x70
>> [ 143.033806] ret_from_fork+0x35/0x40
>>
>> So just delete the bad job from mirror list directly after signal its
>> fence
>>
>> Signed-off-by: Trigger Huang <Trigger.Huang@xxxxxxx>
>> ---
>> drivers/gpu/drm/scheduler/sched_main.c | 18 +++++++++++++++++-
>> 1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>> b/drivers/gpu/drm/scheduler/sched_main.c
>> index 18ebbb0..b2832fb 100644
>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>> @@ -228,7 +228,10 @@ static void drm_sched_job_finish(struct
>> work_struct *work)
>>
>> spin_lock(&sched->job_list_lock);
>> /* remove job from ring_mirror_list */
>> - list_del(&s_job->node);
>> + /* This node may already has been deleted in job recovery */
>> + if (s_job->node.next != &(s_job->node))
>> + list_del_init(&s_job->node);
>> +
> Well that doesn't make any sense at all. If the list_head is already empty (pointing to itself) list_del is a no-op.
>
>> /* queue TDR for next job */
>> drm_sched_start_timeout(sched);
>> spin_unlock(&sched->job_list_lock);
>> @@ -394,6 +397,19 @@ void drm_sched_job_recovery(struct drm_gpu_scheduler *sched)
>> drm_sched_process_job(NULL, &s_fence->cb);
>> }
>> spin_lock(&sched->job_list_lock);
>> + /**
>> + * Normally the 'bad' job will be deleted by its finish_work
>> + * after signal its fence, but sometimes if a GPU recovery is
>> + * invoked before this work finished execution (for example,
>> + * KFD/Host triggered the GPU reset when the current one is
>> + * on-going), then the 'bad' job may will be processed again,
>> + * which is definitely no necessary, and also will cause a lot
>> + * of warning call traces when this job is set by
>> + * 'dma_fence_set_error' because it has already been signaled
>> + */
>> + if ((s_fence->finished.error < 0)
>> + && (s_job->node.next != &(s_job->node)))
>> + list_del_init(&s_job->node);
> That strongly looks incorrect as well.
>
> Can you describe more specific what you are trying to do here?
>
> Thanks,
> Christian.
>
>> }
>> drm_sched_start_timeout(sched);
>> spin_unlock(&sched->job_list_lock);
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
