[PATCH] drm/amdgpu: Fix the null pointer issue for tdr
Andrey Grodzovsky
Andrey.Grodzovsky at amd.com
Thu Nov 14 15:53:39 UTC 2019
ok
Andrey
On 11/14/19 3:12 AM, Christian König wrote:
>> What about instead of peeking at the job to actually remove it from
>> ring_mirror_list right there,
> Also an interesting idea. We would need to protect the mirror list
> with a lock again, but that should be the lesser evil.
>
> Maybe prototype that and see if it works or not.
>
> Regards,
> Christian.
>
> Am 13.11.19 um 17:00 schrieb Andrey Grodzovsky:
>>
>>
>> On 11/13/19 9:20 AM, Christian König wrote:
>>> Another more fundamental question: Could we get rid of the timeout
>>> job at all?
>>
>>
>> There are other stuff there besides picking the first unfinished job
>> which is common for all the drivers - such as freeing guilty signaled
>> job and rearming the timeout work timer.
>>
>>
>>>
>>> I mean we used to give this as parameter to the scheduler callback
>>> because we had the timeout worker in the job, but that is no longer
>>> the case.
>>>
>>> E.g. in drm_sched_job_timedout() we do the following:
>>>> job = list_first_entry_or_null(&sched->ring_mirror_list,
>>>> struct drm_sched_job, node);
>>>
>>> Why don't we just remove that here and only get the first job after
>>> we have stopped the scheduler?
>>
>>
>> Should be ok since we have the extra check for __kthread_should_park
>> in drm_sched_cleanup_jobs which will protect us in this case from a
>> wakeup of sched thread and execution of in drm_sched_cleanup_jobs
>> after we already parked it. The problem here is we need the
>> drm_sched_job to access the private data for each client driver (see
>> amdgpu_job_timedout for example). What about instead of peeking at
>> the job to actually remove it from ring_mirror_list right there, go
>> ahead with it through the reset routine, if it's signaled in the
>> meanwhile that great - release it, otherwise put it back into
>> ring_mirror_list in drm_sched_resubmit_jobs.
>>
>> Andrey
>>
>>
>>>
>>> Regards,
>>> Christian.
>>>
>>> Am 13.11.19 um 15:12 schrieb Andrey Grodzovsky:
>>>>
>>>> This why I asked for a trace with timer enabled, but since there is
>>>> a finite number of places we touch the timer Emily can just put
>>>> prints there. Also, I wonder if this temp fix helps her with the
>>>> issue or not.
>>>>
>>>> Andrey
>>>>
>>>> On 11/13/19 2:36 AM, Christian König wrote:
>>>>> The question is where do we rearm the timer for this problem to
>>>>> occur?
>>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>> Am 12.11.19 um 20:21 schrieb Andrey Grodzovsky:
>>>>>>
>>>>>> I was able to reproduce the crash by using the attached
>>>>>> simulate_crash.patch - waiting on guilty job to signal in reset
>>>>>> work and artificially rearming the timeout timer just before the
>>>>>> check for !cancel_delayed_work(&sched->work_tdr) in
>>>>>> drm_sched_cleanup_jobs - crash log attached in crash.log. This I
>>>>>> think confirms my theory i described earlier in this thread.
>>>>>>
>>>>>> basic_fix.patch handles this by testing whether another timer
>>>>>> already armed ob this scheduler or is there a timeout work in
>>>>>> execution right now (see documentation for work_busy) -
>>>>>> obviously this is not a full solution as this will not protect
>>>>>> from races if for example there is immediate work scheduling such
>>>>>> as in drm_sched_fault - so we probably need to account for this
>>>>>> by making drm_sched_cleanup_jobs (at least in the part where it
>>>>>> iterates ring mirror list and frees jobs) and GPU reset really
>>>>>> mutually exclusive and not like now.
>>>>>>
>>>>>> Andrey
>>>>>>
>>>>>>
>>>>>> On 11/11/19 4:11 PM, Christian König wrote:
>>>>>>> Hi Emily,
>>>>>>>
>>>>>>> you need to print which scheduler instance is freeing the jobs
>>>>>>> and which one is triggering the reset. The TID and PID is
>>>>>>> completely meaningless here since we are called from different
>>>>>>> worker threads and the TID/PID can change on each call.
>>>>>>>
>>>>>>> Apart from that I will look into this a bit deeper when I have
>>>>>>> time.
>>>>>>>
>>>>>>> Regards,
>>>>>>> Christian.
>>>>>>>
>>>>>>> Am 12.11.19 um 07:02 schrieb Deng, Emily:
>>>>>>>> Hi Christian,
>>>>>>>> I add the follow print in function drm_sched_cleanup_jobs.
>>>>>>>> From the log it shows that only use cancel_delayed_work could
>>>>>>>> not avoid to free job when the sched is in reset. But don’t
>>>>>>>> know exactly where it is wrong about the driver. Do you have
>>>>>>>> any suggestion about this?
>>>>>>>> + printk("Emily:drm_sched_cleanup_jobs:begin,tid:%lu,
>>>>>>>> pid:%lu\n", current->tgid, current->pid);
>>>>>>>> /*
>>>>>>>> * Don't destroy jobs while the timeout worker is
>>>>>>>> running OR thread
>>>>>>>> * is being parked and hence assumed to not touch
>>>>>>>> ring_mirror_list
>>>>>>>> */
>>>>>>>> if ((sched->timeout != MAX_SCHEDULE_TIMEOUT &&
>>>>>>>> !cancel_delayed_work(&sched->work_tdr)))
>>>>>>>> return;
>>>>>>>> + printk("Emily:drm_sched_cleanup_jobs,tid:%lu, pid:%lu\n",
>>>>>>>> current->tgid, current->pid);
>>>>>>>> Best wishes
>>>>>>>> Emily Deng
>>>>>>>> Nov 12 12:58:20 ubuntu-drop-August-2018-rc2-gpu0-vf02 kernel:
>>>>>>>> [11380.695091] Emily:drm_sched_cleanup_jobs:begin,tid:2262,
>>>>>>>> pid:2262
>>>>>>>> Nov 12 12:58:20 ubuntu-drop-August-2018-rc2-gpu0-vf02 kernel:
>>>>>>>> [11380.695104] Emily:drm_sched_cleanup_jobs:begin,tid:2262,
>>>>>>>> pid:2262
>>>>>>>> Nov 12 12:58:20 ubuntu-drop-August-2018-rc2-gpu0-vf02 kernel:
>>>>>>>> [11380.695105] Emily:drm_sched_cleanup_jobs,tid:2262, pid:2262
>>>>>>>> Nov 12 12:58:20 ubuntu-drop-August-2018-rc2-gpu0-vf02 kernel:
>>>>>>>> [11380.695107] Emily:drm_sched_cleanup_jobs:begin,tid:2262,
>>>>>>>> pid:2262
>>>>>>>> Nov 12 12:58:20 ubuntu-drop-August-2018-rc2-gpu0-vf02 kernel:
>>>>>>>> [11380.695107] Emily:drm_sched_cleanup_jobs,tid:2262, pid:2262
>>>>>>>> Nov 12 12:58:20 ubuntu-drop-August-2018-rc2-gpu0-vf02 kernel:
>>>>>>>> [11381.222954] [drm:amdgpu_job_timedout [amdgpu]] *ERROR* ring
>>>>>>>> sdma0 timeout, signaled seq=78585, emitted seq=78587
>>>>>>>> Nov 12 12:58:20 ubuntu-drop-August-2018-rc2-gpu0-vf02 kernel:
>>>>>>>> [11381.224275] [drm:amdgpu_job_timedout [amdgpu]] *ERROR*
>>>>>>>> Process information: process pid 0 thread pid 0,
>>>>>>>> s_job:00000000fe75ab36,tid=15603, pid=15603
>>>>>>>> Nov 12 12:58:20 ubuntu-drop-August-2018-rc2-gpu0-vf02 kernel:
>>>>>>>> [11381.225413] amdgpu 0000:00:08.0: GPU reset begin!
>>>>>>>> Nov 12 12:58:20 ubuntu-drop-August-2018-rc2-gpu0-vf02 kernel:
>>>>>>>> [11381.225417] Emily:drm_sched_cleanup_jobs:begin,tid:2262,
>>>>>>>> pid:2262
>>>>>>>> Nov 12 12:58:20 ubuntu-drop-August-2018-rc2-gpu0-vf02 kernel:
>>>>>>>> [11381.225425] Emily:drm_sched_cleanup_jobs:begin,tid:2262,
>>>>>>>> pid:2262
>>>>>>>> Nov 12 12:58:20 ubuntu-drop-August-2018-rc2-gpu0-vf02 kernel:
>>>>>>>> [11381.225425] Emily:drm_sched_cleanup_jobs,tid:2262, pid:2262
>>>>>>>> Nov 12 12:58:20 ubuntu-drop-August-2018-rc2-gpu0-vf02 kernel:
>>>>>>>> [11381.225428] Emily:amdgpu_job_free_cb,Process information:
>>>>>>>> process pid 0 thread pid 0, s_job:00000000fe75ab36, tid:2262,
>>>>>>>> pid:2262
>>>>>>>> Nov 12 12:58:20 ubuntu-drop-August-2018-rc2-gpu0-vf02 kernel:
>>>>>>>> [11381.225429] Emily:drm_sched_cleanup_jobs:begin,tid:2262,
>>>>>>>> pid:2262
>>>>>>>> Nov 12 12:58:20 ubuntu-drop-August-2018-rc2-gpu0-vf02 kernel:
>>>>>>>> [11381.225430] Emily:drm_sched_cleanup_jobs,tid:2262, pid:2262
>>>>>>>> Nov 12 12:58:20 ubuntu-drop-August-2018-rc2-gpu0-vf02 kernel:
>>>>>>>> [11381.225473] Emily:drm_sched_cleanup_jobs:begin,tid:2253,
>>>>>>>> pid:2253
>>>>>>>> Nov 12 12:58:20 ubuntu-drop-August-2018-rc2-gpu0-vf02 kernel:
>>>>>>>> [11381.225486] Emily:drm_sched_cleanup_jobs:begin,tid:2262,
>>>>>>>> pid:2262
>>>>>>>> Nov 12 12:58:20 ubuntu-drop-August-2018-rc2-gpu0-vf02 kernel:
>>>>>>>> [11381.225489] Emily:drm_sched_cleanup_jobs,tid:2262, pid:2262
>>>>>>>> Nov 12 12:58:20 ubuntu-drop-August-2018-rc2-gpu0-vf02 kernel:
>>>>>>>> [11381.225494] Emily:amdgpu_job_free_cb,Process information:
>>>>>>>> process pid 0 thread pid 0, s_job:00000000f086ec84, tid:2262,
>>>>>>>> pid:2262
>>>>>>>> >-----Original Message-----
>>>>>>>> >From: Grodzovsky, Andrey <Andrey.Grodzovsky at amd.com>
>>>>>>>> >Sent: Tuesday, November 12, 2019 11:28 AM
>>>>>>>> >To: Koenig, Christian <Christian.Koenig at amd.com>; Deng, Emily
>>>>>>>> ><Emily.Deng at amd.com>; amd-gfx at lists.freedesktop.org
>>>>>>>> >Subject: Re: [PATCH] drm/amdgpu: Fix the null pointer issue
>>>>>>>> for tdr
>>>>>>>> >
>>>>>>>> >Thinking more about this claim - we assume here that if
>>>>>>>> cancel_delayed_work
>>>>>>>> >returned true it guarantees that timeout work is not running
>>>>>>>> but, it merely
>>>>>>>> >means there was a pending timeout work which was removed from the
>>>>>>>> >workqueue before it's timer elapsed and so it didn't have a
>>>>>>>> chance to be
>>>>>>>> >dequeued and executed, it doesn't cover already executing
>>>>>>>> work. So there is a
>>>>>>>> >possibility where while timeout work started executing another
>>>>>>>> timeout work
>>>>>>>> >already got enqueued (maybe through earlier cleanup jobs or
>>>>>>>> through
>>>>>>>> >drm_sched_fault) and if at this point another
>>>>>>>> drm_sched_cleanup_jobs runs
>>>>>>>> >cancel_delayed_work(&sched->work_tdr) will return true even
>>>>>>>> while there is a
>>>>>>>> >timeout job in progress.
>>>>>>>> >Unfortunately we cannot change cancel_delayed_work to
>>>>>>>> >cancel_delayed_work_sync to flush the timeout work as timeout
>>>>>>>> work itself
>>>>>>>> >waits for schedule thread to be parked again when calling
>>>>>>>> park_thread.
>>>>>>>> >
>>>>>>>> >Andrey
>>>>>>>> >
>>>>>>>> >________________________________________
>>>>>>>> >From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> on
>>>>>>>> behalf of
>>>>>>>> >Koenig, Christian <Christian.Koenig at amd.com>
>>>>>>>> >Sent: 08 November 2019 05:35:18
>>>>>>>> >To: Deng, Emily; amd-gfx at lists.freedesktop.org
>>>>>>>> >Subject: Re: [PATCH] drm/amdgpu: Fix the null pointer issue
>>>>>>>> for tdr
>>>>>>>> >
>>>>>>>> >Hi Emily,
>>>>>>>> >
>>>>>>>> >exactly that can't happen. See here:
>>>>>>>> >
>>>>>>>> >> /* Don't destroy jobs while the timeout worker is
>>>>>>>> running */
>>>>>>>> >> if (sched->timeout != MAX_SCHEDULE_TIMEOUT &&
>>>>>>>> >> !cancel_delayed_work(&sched->work_tdr))
>>>>>>>> >> return NULL;
>>>>>>>> >
>>>>>>>> >We never free jobs while the timeout working is running to
>>>>>>>> prevent exactly
>>>>>>>> >that issue.
>>>>>>>> >
>>>>>>>> >Regards,
>>>>>>>> >Christian.
>>>>>>>> >
>>>>>>>> >Am 08.11.19 um 11:32 schrieb Deng, Emily:
>>>>>>>> >> Hi Christian,
>>>>>>>> >> The drm_sched_job_timedout-> amdgpu_job_timedout call
>>>>>>>> >amdgpu_device_gpu_recover. I mean the main scheduler free the
>>>>>>>> jobs while
>>>>>>>> >in amdgpu_device_gpu_recover, and before calling drm_sched_stop.
>>>>>>>> >>
>>>>>>>> >> Best wishes
>>>>>>>> >> Emily Deng
>>>>>>>> >>
>>>>>>>> >>
>>>>>>>> >>
>>>>>>>> >>> -----Original Message-----
>>>>>>>> >>> From: Koenig, Christian <Christian.Koenig at amd.com>
>>>>>>>> >>> Sent: Friday, November 8, 2019 6:26 PM
>>>>>>>> >>> To: Deng, Emily <Emily.Deng at amd.com>;
>>>>>>>> amd-gfx at lists.freedesktop.org
>>>>>>>> >>> Subject: Re: [PATCH] drm/amdgpu: Fix the null pointer issue
>>>>>>>> for tdr
>>>>>>>> >>>
>>>>>>>> >>> Hi Emily,
>>>>>>>> >>>
>>>>>>>> >>> well who is calling amdgpu_device_gpu_recover() in this case?
>>>>>>>> >>>
>>>>>>>> >>> When it's not the scheduler we shouldn't have a guilty job
>>>>>>>> in the first place.
>>>>>>>> >>>
>>>>>>>> >>> Regards,
>>>>>>>> >>> Christian.
>>>>>>>> >>>
>>>>>>>> >>> Am 08.11.19 um 11:22 schrieb Deng, Emily:
>>>>>>>> >>>> Hi Chrisitan,
>>>>>>>> >>>> No, I am with the new branch and also has the
>>>>>>>> patch. Even it
>>>>>>>> >>>> are freed by
>>>>>>>> >>> main scheduler, how we could avoid main scheduler to free
>>>>>>>> jobs while
>>>>>>>> >>> enter to function amdgpu_device_gpu_recover?
>>>>>>>> >>>> Best wishes
>>>>>>>> >>>> Emily Deng
>>>>>>>> >>>>
>>>>>>>> >>>>
>>>>>>>> >>>>
>>>>>>>> >>>>> -----Original Message-----
>>>>>>>> >>>>> From: Koenig, Christian <Christian.Koenig at amd.com>
>>>>>>>> >>>>> Sent: Friday, November 8, 2019 6:15 PM
>>>>>>>> >>>>> To: Deng, Emily <Emily.Deng at amd.com>; amd-
>>>>>>>> >gfx at lists.freedesktop.org
>>>>>>>> >>>>> Subject: Re: [PATCH] drm/amdgpu: Fix the null pointer
>>>>>>>> issue for tdr
>>>>>>>> >>>>>
>>>>>>>> >>>>> Hi Emily,
>>>>>>>> >>>>>
>>>>>>>> >>>>> in this case you are on an old code branch.
>>>>>>>> >>>>>
>>>>>>>> >>>>> Jobs are freed now by the main scheduler thread and only
>>>>>>>> if no
>>>>>>>> >>>>> timeout handler is running.
>>>>>>>> >>>>>
>>>>>>>> >>>>> See this patch here:
>>>>>>>> >>>>>> commit 5918045c4ed492fb5813f980dcf89a90fefd0a4e
>>>>>>>> >>>>>> Author: Christian König <christian.koenig at amd.com>
>>>>>>>> >>>>>> Date: Thu Apr 18 11:00:21 2019 -0400
>>>>>>>> >>>>>>
>>>>>>>> >>>>>> drm/scheduler: rework job destruction
>>>>>>>> >>>>> Regards,
>>>>>>>> >>>>> Christian.
>>>>>>>> >>>>>
>>>>>>>> >>>>> Am 08.11.19 um 11:11 schrieb Deng, Emily:
>>>>>>>> >>>>>> Hi Christian,
>>>>>>>> >>>>>> Please refer to follow log, when it enter to
>>>>>>>> >>>>>> amdgpu_device_gpu_recover
>>>>>>>> >>>>> function, the bad job 000000005086879e is freeing in
>>>>>>>> function
>>>>>>>> >>>>> amdgpu_job_free_cb at the same time, because of the
>>>>>>>> hardware fence
>>>>>>>> >>> signal.
>>>>>>>> >>>>> But amdgpu_device_gpu_recover goes faster, at this case, the
>>>>>>>> >>>>> s_fence is already freed, but job is not freed in time.
>>>>>>>> Then this issue
>>>>>>>> >occurs.
>>>>>>>> >>>>>> [ 449.792189] [drm:amdgpu_job_timedout [amdgpu]]
>>>>>>>> *ERROR* ring
>>>>>>>> >>> sdma0
>>>>>>>> >>>>>> timeout, signaled seq=2481, emitted seq=2483 [ 449.793202]
>>>>>>>> >>>>>> [drm:amdgpu_job_timedout [amdgpu]] *ERROR* Process
>>>>>>>> information:
>>>>>>>> >>>>> process pid 0 thread pid 0, s_job:000000005086879e [
>>>>>>>> 449.794163]
>>>>>>>> >>>>> amdgpu
>>>>>>>> >>>>> 0000:00:08.0: GPU reset begin!
>>>>>>>> >>>>>> [ 449.794175] Emily:amdgpu_job_free_cb,Process
>>>>>>>> information:
>>>>>>>> >>>>>> process pid 0 thread pid 0, s_job:000000005086879e [
>>>>>>>> 449.794221]
>>>>>>>> >>>>>> Emily:amdgpu_job_free_cb,Process information: process
>>>>>>>> pid 0
>>>>>>>> >>>>>> thread pid 0, s_job:0000000066eb74ab [ 449.794222]
>>>>>>>> >>>>>> Emily:amdgpu_job_free_cb,Process information: process
>>>>>>>> pid 0
>>>>>>>> >>>>>> thread pid 0, s_job:00000000d4438ad9 [ 449.794255]
>>>>>>>> >>>>>> Emily:amdgpu_job_free_cb,Process information: process
>>>>>>>> pid 0
>>>>>>>> >>>>>> thread pid 0, s_job:00000000b6d69c65 [ 449.794257]
>>>>>>>> >>>>>> Emily:amdgpu_job_free_cb,Process information: process
>>>>>>>> pid 0
>>>>>>>> >>>>>> thread pid 0,
>>>>>>>> >>>>> s_job:00000000ea85e922 [ 449.794287]
>>>>>>>> >>>>> Emily:amdgpu_job_free_cb,Process
>>>>>>>> >>>>> information: process pid 0 thread pid 0,
>>>>>>>> s_job:00000000ed3a5ac6 [
>>>>>>>> >>>>> 449.794366] BUG: unable to handle kernel NULL pointer
>>>>>>>> dereference
>>>>>>>> >>>>> at
>>>>>>>> >>>>> 00000000000000c0 [ 449.800818] PGD 0 P4D 0 [ 449.801040]
>>>>>>>> Oops:
>>>>>>>> >>>>> 0000 [#1] SMP PTI
>>>>>>>> >>>>>> [ 449.801338] CPU: 3 PID: 55 Comm: kworker/3:1 Tainted:
>>>>>>>> G OE
>>>>>>>> >>>>> 4.18.0-15-generic #16~18.04.1-Ubuntu
>>>>>>>> >>>>>> [ 449.802157] Hardware name: QEMU Standard PC (i440FX +
>>>>>>>> PIIX,
>>>>>>>> >>>>>> 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014 [ 449.802944]
>>>>>>>> >>>>>> Workqueue: events drm_sched_job_timedout [amd_sched] [
>>>>>>>> >>>>>> 449.803488]
>>>>>>>> >>> RIP:
>>>>>>>> >>>>> 0010:amdgpu_device_gpu_recover+0x1da/0xb60 [amdgpu]
>>>>>>>> >>>>>> [ 449.804020] Code: dd ff ff 49 39 c5 48 89 55 a8 0f 85
>>>>>>>> 56 ff ff
>>>>>>>> >>>>>> ff
>>>>>>>> >>>>>> 45 85 e4 0f
>>>>>>>> >>>>> 85 a1 00 00 00 48 8b 45 b0 48 85 c0 0f 84 60 01 00 00 48
>>>>>>>> 8b 40 10
>>>>>>>> >>>>> <48> 8b
>>>>>>>> >>> 98
>>>>>>>> >>>>> c0 00 00 00 48 85 db 0f 84 4c 01 00 00 48 8b 43
>>>>>>>> 48 a8 01
>>>>>>>> >>>>>> [ 449.805593] RSP: 0018:ffffb4c7c08f7d68 EFLAGS:
>>>>>>>> 00010286 [
>>>>>>>> >>>>>> 449.806032] RAX: 0000000000000000 RBX: 0000000000000000
>>>>>>>> RCX:
>>>>>>>> >>>>>> 0000000000000000 [ 449.806625] RDX: ffffb4c7c08f5ac0 RSI:
>>>>>>>> >>>>>> 0000000fffffffe0 RDI: 0000000000000246 [ 449.807224] RBP:
>>>>>>>> >>>>>> ffffb4c7c08f7de0 R08: 00000068b9d54000 R09:
>>>>>>>> 0000000000000000 [
>>>>>>>> >>>>>> 449.807818] R10: 0000000000000000 R11: 0000000000000148
>>>>>>>> R12:
>>>>>>>> >>>>>> 0000000000000000 [ 449.808411] R13: ffffb4c7c08f7da0 R14:
>>>>>>>> >>>>>> ffff8d82b8525d40 R15: ffff8d82b8525d40 [ 449.809004] FS:
>>>>>>>> >>>>>> 0000000000000000(0000) GS:ffff8d82bfd80000(0000)
>>>>>>>> >>>>>> knlGS:0000000000000000 [ 449.809674] CS: 0010 DS: 0000
>>>>>>>> ES: 0000
>>>>>>>> >CR0:
>>>>>>>> >>>>>> 0000000080050033 [ 449.810153] CR2: 00000000000000c0 CR3:
>>>>>>>> >>>>>> 000000003cc0a001 CR4: 00000000003606e0 [ 449.810747] DR0:
>>>>>>>> >>>>> 0000000000000000 DR1: 0000000000000000 DR2:
>>>>>>>> 0000000000000000 [
>>>>>>>> >>>>> 449.811344] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
>>>>>>>> >>>>> 0000000000000400 [ 449.811937] Call Trace:
>>>>>>>> >>>>>> [ 449.812206] amdgpu_job_timedout+0x114/0x140 [amdgpu] [
>>>>>>>> >>>>>> 449.812635] drm_sched_job_timedout+0x44/0x90 [amd_sched] [
>>>>>>>> >>>>>> 449.813139] ? amdgpu_cgs_destroy_device+0x10/0x10
>>>>>>>> [amdgpu] [
>>>>>>>> >>>>>> 449.813609] ? drm_sched_job_timedout+0x44/0x90
>>>>>>>> [amd_sched] [
>>>>>>>> >>>>>> 449.814077] process_one_work+0x1fd/0x3f0 [ 449.814417]
>>>>>>>> >>>>>> worker_thread+0x34/0x410 [ 449.814728]
>>>>>>>> kthread+0x121/0x140 [
>>>>>>>> >>>>>> 449.815004] ? process_one_work+0x3f0/0x3f0 [
>>>>>>>> 449.815374] ?
>>>>>>>> >>>>>> kthread_create_worker_on_cpu+0x70/0x70
>>>>>>>> >>>>>> [ 449.815799] ret_from_fork+0x35/0x40
>>>>>>>> >>>>>>
>>>>>>>> >>>>>>> -----Original Message-----
>>>>>>>> >>>>>>> From: Koenig, Christian <Christian.Koenig at amd.com>
>>>>>>>> >>>>>>> Sent: Friday, November 8, 2019 5:43 PM
>>>>>>>> >>>>>>> To: Deng, Emily <Emily.Deng at amd.com>; amd-
>>>>>>>> >>> gfx at lists.freedesktop.org
>>>>>>>> >>>>>>> Subject: Re: [PATCH] drm/amdgpu: Fix the null pointer
>>>>>>>> issue for
>>>>>>>> >>>>>>> tdr
>>>>>>>> >>>>>>>
>>>>>>>> >>>>>>> Am 08.11.19 um 10:39 schrieb Deng, Emily:
>>>>>>>> >>>>>>>> Sorry, please take your time.
>>>>>>>> >>>>>>> Have you seen my other response a bit below?
>>>>>>>> >>>>>>>
>>>>>>>> >>>>>>> I can't follow how it would be possible for
>>>>>>>> job->s_fence to be
>>>>>>>> >>>>>>> NULL without the job also being freed.
>>>>>>>> >>>>>>>
>>>>>>>> >>>>>>> So it looks like this patch is just papering over some
>>>>>>>> bigger issues.
>>>>>>>> >>>>>>>
>>>>>>>> >>>>>>> Regards,
>>>>>>>> >>>>>>> Christian.
>>>>>>>> >>>>>>>
>>>>>>>> >>>>>>>> Best wishes
>>>>>>>> >>>>>>>> Emily Deng
>>>>>>>> >>>>>>>>
>>>>>>>> >>>>>>>>
>>>>>>>> >>>>>>>>
>>>>>>>> >>>>>>>>> -----Original Message-----
>>>>>>>> >>>>>>>>> From: Koenig, Christian <Christian.Koenig at amd.com>
>>>>>>>> >>>>>>>>> Sent: Friday, November 8, 2019 5:08 PM
>>>>>>>> >>>>>>>>> To: Deng, Emily <Emily.Deng at amd.com>; amd-
>>>>>>>> >>>>> gfx at lists.freedesktop.org
>>>>>>>> >>>>>>>>> Subject: Re: [PATCH] drm/amdgpu: Fix the null pointer
>>>>>>>> issue for
>>>>>>>> >>>>>>>>> tdr
>>>>>>>> >>>>>>>>>
>>>>>>>> >>>>>>>>> Am 08.11.19 um 09:52 schrieb Deng, Emily:
>>>>>>>> >>>>>>>>>> Ping.....
>>>>>>>> >>>>>>>>> You need to give me at least enough time to wake up :)
>>>>>>>> >>>>>>>>>
>>>>>>>> >>>>>>>>>> Best wishes
>>>>>>>> >>>>>>>>>> Emily Deng
>>>>>>>> >>>>>>>>>>
>>>>>>>> >>>>>>>>>>
>>>>>>>> >>>>>>>>>>
>>>>>>>> >>>>>>>>>>> -----Original Message-----
>>>>>>>> >>>>>>>>>>> From: amd-gfx
>>>>>>>> <amd-gfx-bounces at lists.freedesktop.org> On
>>>>>>>> >>> Behalf
>>>>>>>> >>>>>>>>>>> Of Deng, Emily
>>>>>>>> >>>>>>>>>>> Sent: Friday, November 8, 2019 10:56 AM
>>>>>>>> >>>>>>>>>>> To: Koenig, Christian <Christian.Koenig at amd.com>; amd-
>>>>>>>> >>>>>>>>>>> gfx at lists.freedesktop.org
>>>>>>>> >>>>>>>>>>> Subject: RE: [PATCH] drm/amdgpu: Fix the null
>>>>>>>> pointer issue
>>>>>>>> >>>>>>>>>>> for tdr
>>>>>>>> >>>>>>>>>>>
>>>>>>>> >>>>>>>>>>>> -----Original Message-----
>>>>>>>> >>>>>>>>>>>> From: Christian König
>>>>>>>> <ckoenig.leichtzumerken at gmail.com>
>>>>>>>> >>>>>>>>>>>> Sent: Thursday, November 7, 2019 7:28 PM
>>>>>>>> >>>>>>>>>>>> To: Deng, Emily <Emily.Deng at amd.com>;
>>>>>>>> >>>>>>>>>>>> amd-gfx at lists.freedesktop.org
>>>>>>>> >>>>>>>>>>>> Subject: Re: [PATCH] drm/amdgpu: Fix the null
>>>>>>>> pointer issue
>>>>>>>> >>>>>>>>>>>> for tdr
>>>>>>>> >>>>>>>>>>>>
>>>>>>>> >>>>>>>>>>>> Am 07.11.19 um 11:25 schrieb Emily Deng:
>>>>>>>> >>>>>>>>>>>>> When the job is already signaled, the s_fence is
>>>>>>>> freed.
>>>>>>>> >>>>>>>>>>>>> Then it will has null pointer in
>>>>>>>> amdgpu_device_gpu_recover.
>>>>>>>> >>>>>>>>>>>> NAK, the s_fence is only set to NULL when the job
>>>>>>>> is destroyed.
>>>>>>>> >>>>>>>>>>>> See drm_sched_job_cleanup().
>>>>>>>> >>>>>>>>>>> I know it is set to NULL in drm_sched_job_cleanup.
>>>>>>>> But in one
>>>>>>>> >>>>>>>>>>> case, when it enter into the
>>>>>>>> amdgpu_device_gpu_recover, it
>>>>>>>> >>>>>>>>>>> already in drm_sched_job_cleanup, and at this time,
>>>>>>>> it will
>>>>>>>> >>>>>>>>>>> go to free
>>>>>>>> >>>>> job.
>>>>>>>> >>>>>>>>>>> But the amdgpu_device_gpu_recover sometimes is
>>>>>>>> faster. At
>>>>>>>> >>>>>>>>>>> that time, job is not freed, but s_fence is already
>>>>>>>> NULL.
>>>>>>>> >>>>>>>>> No, that case can't happen. See here:
>>>>>>>> >>>>>>>>>
>>>>>>>> >>>>>>>>>> drm_sched_job_cleanup(s_job);
>>>>>>>> >>>>>>>>>>
>>>>>>>> >>>>>>>>>> amdgpu_ring_priority_put(ring, s_job->s_priority);
>>>>>>>> >>>>>>>>>> dma_fence_put(job->fence);
>>>>>>>> >>>>>>>>>> amdgpu_sync_free(&job->sync);
>>>>>>>> >>>>>>>>>> amdgpu_sync_free(&job->sched_sync);
>>>>>>>> >>>>>>>>>> kfree(job);
>>>>>>>> >>>>>>>>> The job itself is freed up directly after freeing the
>>>>>>>> reference
>>>>>>>> >>>>>>>>> to the
>>>>>>>> >>>>> s_fence.
>>>>>>>> >>>>>>>>> So you are just papering over a much bigger problem
>>>>>>>> here. This
>>>>>>>> >>>>>>>>> patch is a clear NAK.
>>>>>>>> >>>>>>>>>
>>>>>>>> >>>>>>>>> Regards,
>>>>>>>> >>>>>>>>> Christian.
>>>>>>>> >>>>>>>>>
>>>>>>>> >>>>>>>>>>>> When you see a job without an s_fence then that
>>>>>>>> means the
>>>>>>>> >>>>>>>>>>>> problem is somewhere else.
>>>>>>>> >>>>>>>>>>>>
>>>>>>>> >>>>>>>>>>>> Regards,
>>>>>>>> >>>>>>>>>>>> Christian.
>>>>>>>> >>>>>>>>>>>>
>>>>>>>> >>>>>>>>>>>>> Signed-off-by: Emily Deng <Emily.Deng at amd.com>
>>>>>>>> >>>>>>>>>>>>> ---
>>>>>>>> >>>>>>>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
>>>>>>>> >>>>>>>>>>>>> drivers/gpu/drm/scheduler/sched_main.c | 11
>>>>>>>> ++++++---
>>>>>>>> >--
>>>>>>>> >>>>>>>>>>>>> 2 files changed, 7 insertions(+), 6
>>>>>>>> deletions(-)
>>>>>>>> >>>>>>>>>>>>>
>>>>>>>> >>>>>>>>>>>>> diff --git
>>>>>>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>>>>> >>>>>>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>>>>> >>>>>>>>>>>>> index e6ce949..5a8f08e 100644
>>>>>>>> >>>>>>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>>>>> >>>>>>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>>>>> >>>>>>>>>>>>> @@ -4075,7 +4075,7 @@ int
>>>>>>>> >>> amdgpu_device_gpu_recover(struct
>>>>>>>> >>>>>>>>>>>> amdgpu_device *adev,
>>>>>>>> >>>>>>>>>>>>> *
>>>>>>>> >>>>>>>>>>>>> * job->base holds a reference to
>>>>>>>> parent fence
>>>>>>>> >>>>>>>>>>>>> */
>>>>>>>> >>>>>>>>>>>>> - if (job && job->base.s_fence->parent &&
>>>>>>>> >>>>>>>>>>>>> + if (job && job->base.s_fence &&
>>>>>>>> >>>>>>>>>>>>> + job->base.s_fence->parent
>>>>>>>> >>>>>>> &&
>>>>>>>> >>>>>>>>>>>>> dma_fence_is_signaled(job->base.s_fence->parent))
>>>>>>>> >>>>>>>>>>>>> job_signaled = true;
>>>>>>>> >>>>>>>>>>>>>
>>>>>>>> >>>>>>>>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>> >>>>>>>>>>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>> >>>>>>>>>>>>> index 31809ca..56cc10e 100644
>>>>>>>> >>>>>>>>>>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>> >>>>>>>>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>> >>>>>>>>>>>>> @@ -334,8 +334,8 @@ void
>>>>>>>> >drm_sched_increase_karma(struct
>>>>>>>> >>>>>>>>>>>> drm_sched_job
>>>>>>>> >>>>>>>>>>>>> *bad)
>>>>>>>> >>>>>>>>>>>>>
>>>>>>>> >>>>>>>>>>>>> spin_lock(&rq->lock);
>>>>>>>> >>>>>>>>>>>>> list_for_each_entry_safe(entity,
>>>>>>>> >>>>>>>>>>>>> tmp,
>>>>>>>> >>> &rq-
>>>>>>>> >>>>>>>> entities,
>>>>>>>> >>>>>>>>>>>> list) {
>>>>>>>> >>>>>>>>>>>>> - if
>>>>>>>> (bad->s_fence->scheduled.context
>>>>>>>> >>>>>>> ==
>>>>>>>> >>>>>>>>>>>>> - entity->fence_context) {
>>>>>>>> >>>>>>>>>>>>> + if (bad->s_fence &&
>>>>>>>> >>>>>>>>>>>>> + (bad->s_fence-
>>>>>>>> >>>>>>>>>>>>> scheduled.context ==
>>>>>>>> >>>>>>>>>>>>> + entity->fence_context)) {
>>>>>>>> >>>>>>>>>>>>> if
>>>>>>>> >>>>>>>>>>>>> (atomic_read(&bad-
>>>>>>>> >>>>>>>> karma) >
>>>>>>>> >>>>>>>>>>>>> bad->sched-
>>>>>>>> >>>> hang_limit)
>>>>>>>> >>>>>>>>>>>>> if
>>>>>>>> >>>>>>>>>>>>> (entity-
>>>>>>>> >>>> guilty) @@ -376,7 +376,7 @@ void
>>>>>>>> >>>>>>>>>>>>> drm_sched_stop(struct
>>>>>>>> >>>>>>> drm_gpu_scheduler
>>>>>>>> >>>>>>>>>>>> *sched, struct drm_sched_job *bad)
>>>>>>>> >>>>>>>>>>>>> * This iteration is thread safe as
>>>>>>>> sched thread
>>>>>>>> >>>>>>>>>>>>> is
>>>>>>>> >>> stopped.
>>>>>>>> >>>>>>>>>>>>> */
>>>>>>>> >>>>>>>>>>>>> list_for_each_entry_safe_reverse(s_job, tmp,
>>>>>>>> >>>>>>>>>>>>> &sched- ring_mirror_list, node) {
>>>>>>>> >>>>>>>>>>>>> - if (s_job->s_fence->parent &&
>>>>>>>> >>>>>>>>>>>>> + if (s_job->s_fence &&
>>>>>>>> s_job->s_fence->parent &&
>>>>>>>> >>>>>>>>>>>>> dma_fence_remove_callback(s_job-
>>>>>>>> >>>> s_fence-
>>>>>>>> >>>>>>>> parent,
>>>>>>>> >>>>>>>>>>>>> &s_job->cb)) {
>>>>>>>> >>>>>>>>>>>>> atomic_dec(&sched->hw_rq_count);
>>>>>>>> >>> @@ -
>>>>>>>> >>>>>>> 395,7
>>>>>>>> >>>>>>>>>>> +395,8 @@ void
>>>>>>>> >>>>>>>>>>>>> drm_sched_stop(struct drm_gpu_scheduler
>>>>>>>> >>>>>>>>>>>> *sched, struct drm_sched_job *bad)
>>>>>>>> >>>>>>>>>>>>> *
>>>>>>>> >>>>>>>>>>>>> * Job is still alive so fence
>>>>>>>> >>>>>>>>>>>>> refcount at
>>>>>>>> >>> least 1
>>>>>>>> >>>>>>>>>>>>> */
>>>>>>>> >>>>>>>>>>>>> - dma_fence_wait(&s_job->s_fence->finished,
>>>>>>>> >>>>>>> false);
>>>>>>>> >>>>>>>>>>>>> + if (s_job->s_fence)
>>>>>>>> >>>>>>>>>>>>> + dma_fence_wait(&s_job->s_fence-
>>>>>>>> >>>>>>>> finished,
>>>>>>>> >>>>>>>>>>>> false);
>>>>>>>> >>>>>>>>>>>>> /*
>>>>>>>> >>>>>>>>>>>>> * We must keep bad job alive
>>>>>>>> >>>>>>>>>>>>> for later
>>>>>>>> >>> use
>>>>>>>> >>>>>>> during @@
>>>>>>>> >>>>>>>>>>>> -438,7
>>>>>>>> >>>>>>>>>>>>> +439,7 @@ void drm_sched_start(struct
>>>>>>>> drm_gpu_scheduler
>>>>>>>> >>>>> *sched,
>>>>>>>> >>>>>>>>>>>>> +bool
>>>>>>>> >>>>>>>>>>>> full_recovery)
>>>>>>>> >>>>>>>>>>>>> * GPU recovers can't run in parallel.
>>>>>>>> >>>>>>>>>>>>> */
>>>>>>>> >>>>>>>>>>>>> list_for_each_entry_safe(s_job, tmp,
>>>>>>>> >>>>>>>>>>>>> &sched->ring_mirror_list,
>>>>>>>> >>>>>>>>>>>>> node)
>>>>>>>> >>>>>>>>>>>> {
>>>>>>>> >>>>>>>>>>>>> - struct dma_fence *fence =
>>>>>>>> s_job->s_fence->parent;
>>>>>>>> >>>>>>>>>>>>> + struct dma_fence *fence =
>>>>>>>> s_job->s_fence ?
>>>>>>>> >>>>>>>>>>>>> + s_job-
>>>>>>>> >>>>>>>> s_fence-
>>>>>>>> >>>>>>>>>>>>> parent :
>>>>>>>> >>>>>>>>>>>>> +NULL;
>>>>>>>> >>>>>>>>>>>>>
>>>>>>>> >>>>>>>>>>>>> atomic_inc(&sched->hw_rq_count);
>>>>>>>> >>>>>>>>>>>>>
>>>>>>>> >>>>>>>>>>> _______________________________________________
>>>>>>>> >>>>>>>>>>> amd-gfx mailing list
>>>>>>>> >>>>>>>>>>> amd-gfx at lists.freedesktop.org
>>>>>>>> >>>>>>>>>>>
>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>>>>>> <https://lists.freedesktop.org/mailman/listinfo/amd-gfx>
>>>>>>>> >
>>>>>>>> >_______________________________________________
>>>>>>>> >amd-gfx mailing list
>>>>>>>> >amd-gfx at lists.freedesktop.org
>>>>>>>> >https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> amd-gfx mailing list
>>>>>> amd-gfx at lists.freedesktop.org
>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>>>
>>>
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
More information about the amd-gfx
mailing list