<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=Windows-1252">
</head>
<body>
<div>I see now. In that case the change seems good to me.</div>
<div><br>
</div>
<div>Andrey</div>
<hr style="display:inline-block;width:98%" tabindex="-1">
<div id="divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" style="font-size:11pt" color="#000000"><b>From:</b> Koenig, Christian <Christian.Koenig@amd.com><br>
<b>Sent:</b> 24 March 2020 13:58<br>
<b>To:</b> Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; Tao, Yintian <Yintian.Tao@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com><br>
<b>Cc:</b> amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org><br>
<b>Subject:</b> Re: [PATCH] drm/amdgpu: hold the reference of finished fence</font>
<div> </div>
</div>
<div style="background-color:#FFFFFF">
<div class="x_moz-cite-prefix">There is a misunderstanding here:<br>
<br>
<blockquote type="cite"><font size="2"><span style="font-size:11pt">Did you find out why the zero refcount on the finished fence happens
<br>
before the fence was signaled ?</span></font></blockquote>
<br>
The refcount on the finished fence doesn't become zero before it is signaled, it becomes zero while it is signaled.<br>
<br>
CPU 1 calls dma_fence_signal(fence) without holding a reference to the fence. CPU 2 at the same time checks if the fence is signaled and frees the last reference because it find the signaled flag to be set.<br>
<br>
The problem is now that dma_fence_signal() wants to set the timestamp after setting the signaled flag and now races with freeing the memory.<br>
<br>
That's a really hard to hit problem, but it indeed seems to be possible.<br>
<br>
Christian.<br>
<br>
Am 24.03.20 um 15:52 schrieb Andrey Grodzovsky:<br>
</div>
<blockquote type="cite">
<p>This is only for the guilty job which was removed from the ring_mirror_list due to completion and hence will not be resubmitted by recovery and will not be freed by the usual flow in drm_sched_get_cleanup_job (see drm_sched_stop)</p>
<p>Andrey<br>
</p>
<div class="x_moz-cite-prefix">On 3/24/20 10:45 AM, Pan, Xinhui wrote:<br>
</div>
<blockquote type="cite">
<p align="Left" style="font-family:Arial; font-size:10pt; color:#0078D7; margin:15pt">
[AMD Official Use Only - Internal Distribution Only]<br>
</p>
<br>
<div>
<div dir="auto" style="direction:ltr; margin:0; padding:0; font-family:sans-serif; font-size:11pt; color:black">
Does this issue occur when gpu recovery?<br>
</div>
<div dir="auto" style="direction:ltr; margin:0; padding:0; font-family:sans-serif; font-size:11pt; color:black">
I just check the code, fence timedout will free job and put its fence. but gpu recovery might resubmit job.
<br>
</div>
<div dir="auto" style="direction:ltr; margin:0; padding:0; font-family:sans-serif; font-size:11pt; color:black">
Correct me if I am wrong.</div>
<hr tabindex="-1" style="display:inline-block; width:98%">
<div id="x_divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" color="#000000" style="font-size:11pt"><b>From:</b> amd-gfx
<a class="x_moz-txt-link-rfc2396E" href="mailto:amd-gfx-bounces@lists.freedesktop.org">
<amd-gfx-bounces@lists.freedesktop.org></a> on behalf of Andrey Grodzovsky <a class="x_moz-txt-link-rfc2396E" href="mailto:Andrey.Grodzovsky@amd.com">
<Andrey.Grodzovsky@amd.com></a><br>
<b>Sent:</b> Tuesday, March 24, 2020 11:40:06 AM<br>
<b>To:</b> Tao, Yintian <a class="x_moz-txt-link-rfc2396E" href="mailto:Yintian.Tao@amd.com">
<Yintian.Tao@amd.com></a>; Koenig, Christian <a class="x_moz-txt-link-rfc2396E" href="mailto:Christian.Koenig@amd.com">
<Christian.Koenig@amd.com></a>; Deucher, Alexander <a class="x_moz-txt-link-rfc2396E" href="mailto:Alexander.Deucher@amd.com">
<Alexander.Deucher@amd.com></a><br>
<b>Cc:</b> <a class="x_moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org">
amd-gfx@lists.freedesktop.org</a> <a class="x_moz-txt-link-rfc2396E" href="mailto:amd-gfx@lists.freedesktop.org">
<amd-gfx@lists.freedesktop.org></a><br>
<b>Subject:</b> Re: [PATCH] drm/amdgpu: hold the reference of finished fence</font>
<div> </div>
</div>
<div class="x_BodyFragment"><font size="2"><span style="font-size:11pt">
<div class="x_PlainText"><br>
On 3/23/20 10:22 AM, Yintian Tao wrote:<br>
> There is one one corner case at dma_fence_signal_locked<br>
> which will raise the NULL pointer problem just like below.<br>
> ->dma_fence_signal<br>
> ->dma_fence_signal_locked<br>
> ->test_and_set_bit<br>
> here trigger dma_fence_release happen due to the zero of fence refcount.<br>
<br>
<br>
Did you find out why the zero refcount on the finished fence happens <br>
before the fence was signaled ? The finished fence is created with <br>
refcount set to 1 in drm_sched_fence_create->dma_fence_init and then the <br>
refcount is decremented in <br>
drm_sched_main->amdgpu_job_free_cb->drm_sched_job_cleanup. This should <br>
only happen after fence is already signaled (see <br>
drm_sched_get_cleanup_job). On top of that the finished fence is <br>
referenced from other places (e.g. entity->last_scheduled e.t.c)...<br>
<br>
<br>
><br>
> ->dma_fence_put<br>
> ->dma_fence_release<br>
> ->drm_sched_fence_release_scheduled<br>
> ->call_rcu<br>
> here make the union fled “cb_list” at finished fence<br>
> to NULL because struct rcu_head contains two pointer<br>
> which is same as struct list_head cb_list<br>
><br>
> Therefore, to hold the reference of finished fence at drm_sched_process_job<br>
> to prevent the null pointer during finished fence dma_fence_signal<br>
><br>
> [ 732.912867] BUG: kernel NULL pointer dereference, address: 0000000000000008<br>
> [ 732.914815] #PF: supervisor write access in kernel mode<br>
> [ 732.915731] #PF: error_code(0x0002) - not-present page<br>
> [ 732.916621] PGD 0 P4D 0<br>
> [ 732.917072] Oops: 0002 [#1] SMP PTI<br>
> [ 732.917682] CPU: 7 PID: 0 Comm: swapper/7 Tainted: G OE 5.4.0-rc7 #1<br>
> [ 732.918980] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.8.2-0-g33fbe13 by qemu-project.org 04/01/2014<br>
> [ 732.920906] RIP: 0010:dma_fence_signal_locked+0x3e/0x100<br>
> [ 732.938569] Call Trace:<br>
> [ 732.939003] <IRQ><br>
> [ 732.939364] dma_fence_signal+0x29/0x50<br>
> [ 732.940036] drm_sched_fence_finished+0x12/0x20 [gpu_sched]<br>
> [ 732.940996] drm_sched_process_job+0x34/0xa0 [gpu_sched]<br>
> [ 732.941910] dma_fence_signal_locked+0x85/0x100<br>
> [ 732.942692] dma_fence_signal+0x29/0x50<br>
> [ 732.943457] amdgpu_fence_process+0x99/0x120 [amdgpu]<br>
> [ 732.944393] sdma_v4_0_process_trap_irq+0x81/0xa0 [amdgpu]<br>
><br>
> v2: hold the finished fence at drm_sched_process_job instead of<br>
> amdgpu_fence_process<br>
> v3: resume the blank line<br>
><br>
> Signed-off-by: Yintian Tao <a class="x_moz-txt-link-rfc2396E" href="mailto:yttao@amd.com">
<yttao@amd.com></a><br>
> ---<br>
> drivers/gpu/drm/scheduler/sched_main.c | 2 ++<br>
> 1 file changed, 2 insertions(+)<br>
><br>
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c<br>
> index a18eabf692e4..8e731ed0d9d9 100644<br>
> --- a/drivers/gpu/drm/scheduler/sched_main.c<br>
> +++ b/drivers/gpu/drm/scheduler/sched_main.c<br>
> @@ -651,7 +651,9 @@ static void drm_sched_process_job(struct dma_fence *f, struct dma_fence_cb *cb)<br>
> <br>
> trace_drm_sched_process_job(s_fence);<br>
> <br>
> + dma_fence_get(&s_fence->finished);<br>
> drm_sched_fence_finished(s_fence);<br>
<br>
<br>
If the fence was already released during call to <br>
drm_sched_fence_finished->dma_fence_signal->... why is it safe to <br>
reference the s_fence just before that call ? Can't it already be <br>
released by this time ?<br>
<br>
Andrey<br>
<br>
<br>
<br>
> + dma_fence_put(&s_fence->finished);<br>
> wake_up_interruptible(&sched->wake_up_worker);<br>
> }<br>
> <br>
_______________________________________________<br>
amd-gfx mailing list<br>
<a class="x_moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a><br>
<a href="https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cxinhui.pan%40amd.com%7C65933fca0b414d12aab408d7cfa51165%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637206180230440562&sdata=z6ec%2BcWkwjaDgZvkpL3jOMYkBtDjbNOxlXiAk4Ri5Ck%3D&reserved=0">https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cxinhui.pan%40amd.com%7C65933fca0b414d12aab408d7cfa51165%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637206180230440562&sdata=z6ec%2BcWkwjaDgZvkpL3jOMYkBtDjbNOxlXiAk4Ri5Ck%3D&reserved=0</a><br>
</div>
</span></font></div>
</div>
</blockquote>
</blockquote>
<br>
</div>
</body>
</html>