<!DOCTYPE html><html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<p><br>
</p>
<div class="moz-cite-prefix">On 2025-03-09 20:51, Deng, Emily wrote:<br>
</div>
<blockquote type="cite" cite="mid:CO6PR12MB541112593034D35359E030238FD62@CO6PR12MB5411.namprd12.prod.outlook.com">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style>@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}@font-face
{font-family:DengXian;
panose-1:2 1 6 0 3 1 1 1 1 1;}@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}@font-face
{font-family:Aptos;}@font-face
{font-family:"\@DengXian";
panose-1:2 1 6 0 3 1 1 1 1 1;}@font-face
{font-family:Consolas;
panose-1:2 11 6 9 2 2 4 3 2 4;}p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
font-size:12.0pt;
font-family:"Aptos",sans-serif;}a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}pre
{mso-style-priority:99;
mso-style-link:"HTML Preformatted Char";
margin:0in;
font-size:10.0pt;
font-family:"Courier New";}span.HTMLPreformattedChar
{mso-style-name:"HTML Preformatted Char";
mso-style-priority:99;
mso-style-link:"HTML Preformatted";
font-family:Consolas;}span.EmailStyle22
{mso-style-type:personal-reply;
font-family:"Arial",sans-serif;
color:windowtext;}.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;
mso-ligatures:none;}div.WordSection1
{page:WordSection1;}</style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
<p style="font-family:Calibri;font-size:10pt;color:#0000FF;margin:5pt;font-style:normal;font-weight:normal;text-decoration:none;" align="Left">
[AMD Official Use Only - AMD Internal Distribution Only]<br>
</p>
<br>
<div>
<p style="font-family:Calibri;font-size:10pt;color:#0000FF;margin:5pt;font-style:normal;font-weight:normal;text-decoration:none;" align="Left">
[AMD Official Use Only - AMD Internal Distribution Only]<br>
</p>
<br>
<div>
<div class="WordSection1">
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Arial",sans-serif"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Arial",sans-serif"><o:p> </o:p></span></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt">
<div>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">From:</span></b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">
Chen, Xiaogang <a class="moz-txt-link-rfc2396E" href="mailto:Xiaogang.Chen@amd.com"><Xiaogang.Chen@amd.com></a>
<br>
<b>Sent:</b> Saturday, March 8, 2025 8:38 AM<br>
<b>To:</b> Deng, Emily <a class="moz-txt-link-rfc2396E" href="mailto:Emily.Deng@amd.com"><Emily.Deng@amd.com></a>;
<a class="moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a><br>
<b>Subject:</b> Re: [PATCH v4] drm/amdgpu: Fix the
race condition for draining retry fault<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<p><o:p> </o:p></p>
<div>
<p class="MsoNormal">On 3/6/2025 7:27 PM, Deng, Emily
wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p style="margin:5.0pt"><span style="font-size:10.0pt;font-family:"Calibri",sans-serif;color:blue">[AMD
Official Use Only - AMD Internal Distribution Only]<o:p></o:p></span></p>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Arial",sans-serif"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Arial",sans-serif"> </span><o:p></o:p></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt">
<div>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">From:</span></b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">
Chen, Xiaogang
<a href="mailto:Xiaogang.Chen@amd.com" moz-do-not-send="true"><Xiaogang.Chen@amd.com></a>
<br>
<b>Sent:</b> Friday, March 7, 2025 1:01 AM<br>
<b>To:</b> Deng, Emily <a href="mailto:Emily.Deng@amd.com" moz-do-not-send="true"><Emily.Deng@amd.com></a>;
<a href="mailto:amd-gfx@lists.freedesktop.org" moz-do-not-send="true" class="moz-txt-link-freetext">amd-gfx@lists.freedesktop.org</a><br>
<b>Subject:</b> Re: [PATCH v4] drm/amdgpu:
Fix the race condition for draining retry
fault</span><o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"> <o:p></o:p></p>
<p>Thanks for catch up and fix this race condition.
It looks good to me. One minor thing below:<o:p></o:p></p>
<div>
<p class="MsoNormal">On 3/6/2025 12:03 AM, Emily
Deng wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre>Issue:<o:p></o:p></pre>
<pre>In the scenario where svm_range_restore_pages is called, but svm->checkpoint_ts<o:p></o:p></pre>
<pre> has not been set and the retry fault has not been drained, svm_range_unmap_from_cpu<o:p></o:p></pre>
<pre>is triggered and calls svm_range_free. Meanwhile, svm_range_restore_pages<o:p></o:p></pre>
<pre>continues execution and reaches svm_range_from_addr. This results in<o:p></o:p></pre>
<pre>a "failed to find prange..." error, causing the page recovery to fail.<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>How to fix:<o:p></o:p></pre>
<pre>Move the timestamp check code under the protection of svm->lock.<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>v2:<o:p></o:p></pre>
<pre>Make sure all right locks are released before go out.<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>v3:<o:p></o:p></pre>
<pre>Directly goto out_unlock_svms, and return -EAGAIN.<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>v4:<o:p></o:p></pre>
<pre>Refine code.<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>Signed-off-by: Emily Deng <a href="mailto:Emily.Deng@amd.com" moz-do-not-send="true"><Emily.Deng@amd.com></a><o:p></o:p></pre>
<pre>---<o:p></o:p></pre>
<pre> drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 30 +++++++++++++++-------------<o:p></o:p></pre>
<pre> 1 file changed, 16 insertions(+), 14 deletions(-)<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c<o:p></o:p></pre>
<pre>index d04725583f19..83ac14bf7a7a 100644<o:p></o:p></pre>
<pre>--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c<o:p></o:p></pre>
<pre>+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c<o:p></o:p></pre>
<pre>@@ -3008,19 +3008,6 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,<o:p></o:p></pre>
<pre> goto out;<o:p></o:p></pre>
<pre> }<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>- /* check if this page fault time stamp is before svms->checkpoint_ts */<o:p></o:p></pre>
<pre>- if (svms->checkpoint_ts[gpuidx] != 0) {<o:p></o:p></pre>
<pre>- if (amdgpu_ih_ts_after_or_equal(ts, svms->checkpoint_ts[gpuidx])) {<o:p></o:p></pre>
<pre>- pr_debug("draining retry fault, drop fault 0x%llx\n", addr);<o:p></o:p></pre>
<pre>- r = 0;<o:p></o:p></pre>
<pre>- goto out;<o:p></o:p></pre>
<pre>- } else<o:p></o:p></pre>
<pre>- /* ts is after svms->checkpoint_ts now, reset svms->checkpoint_ts<o:p></o:p></pre>
<pre>- * to zero to avoid following ts wrap around give wrong comparing<o:p></o:p></pre>
<pre>- */<o:p></o:p></pre>
<pre>- svms->checkpoint_ts[gpuidx] = 0;<o:p></o:p></pre>
<pre>- }<o:p></o:p></pre>
<pre>-<o:p></o:p></pre>
<pre> if (!p->xnack_enabled) {<o:p></o:p></pre>
<pre> pr_debug("XNACK not enabled for pasid 0x%x\n", pasid);<o:p></o:p></pre>
<pre> r = -EFAULT;<o:p></o:p></pre>
<pre>@@ -3040,6 +3027,20 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,<o:p></o:p></pre>
<pre> mmap_read_lock(mm);<o:p></o:p></pre>
<pre> retry_write_locked:<o:p></o:p></pre>
<pre> mutex_lock(&svms->lock);<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+ /* check if this page fault time stamp is before svms->checkpoint_ts */<o:p></o:p></pre>
<pre>+ if (svms->checkpoint_ts[gpuidx] != 0) {<o:p></o:p></pre>
<pre>+ if (amdgpu_ih_ts_after_or_equal(ts, svms->checkpoint_ts[gpuidx])) {<o:p></o:p></pre>
<pre>+ pr_debug("draining retry fault, drop fault 0x%llx\n", addr);<o:p></o:p></pre>
<pre>+ r = -EAGAIN;<o:p></o:p></pre>
</blockquote>
<p>We drop page fault because it is stale, not mean
to handle it again. if return -EAGAIN we do
amdgpu_gmc_filter_faults_remove. If after unmap,
user map same range again we should treat page
fault happened at same range as new one. <o:p></o:p></p>
<p>Regards<o:p></o:p></p>
<p>Xiaogang<o:p></o:p></p>
<p><span style="font-size:11.0pt;font-family:"Arial",sans-serif">Sorry,
I didn't quite catch that. So, you think we
shouldn't remove the fault from
amdgpu_gmc_filter_faults_remove?</span><o:p></o:p></p>
</div>
</div>
</blockquote>
<p>I think return "-EAGAIN" means a page fault with an
addr and a pasid is not handled this time. Same page
fault will come back again and kfd will handle it, so
remove it from filter to make sure it will be handled
next time.
<o:p></o:p></p>
<p><span style="font-size:11.0pt;font-family:"Arial",sans-serif">Yes, I
also think we should remove it to make sure it will be
handled next time. From this point of view, it should
be removed from filter.</span></p>
</div>
</div>
</div>
</div>
</blockquote>
<p>I agree. Without removing it from the filter, we would ignore
later faults on the same address, which would result in stalling
the application indefinitely.</p>
<p>Regards,<br>
Felix</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:CO6PR12MB541112593034D35359E030238FD62@CO6PR12MB5411.namprd12.prod.outlook.com">
<div>
<div>
<div class="WordSection1">
<div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt">
<p><span style="font-size:11.0pt;font-family:"Arial",sans-serif"><o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Arial",sans-serif"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;mso-ligatures:standardcontextual">Emily Deng<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;mso-ligatures:standardcontextual">Best Wishes<o:p></o:p></span></p>
</div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Arial",sans-serif"><o:p> </o:p></span></p>
<p><span style="font-size:11.0pt;font-family:"Arial",sans-serif"><o:p> </o:p></span></p>
<p>In this case this page fault is stale and we do not need
or cannot handle it. There is no indication it will come
back again and we need handle it. I am not sure if should
remove it from filter. I just concern if should return
"-EAGAIN" in this case.<o:p></o:p></p>
<p>Regards<o:p></o:p></p>
<p>Xiaogang<o:p></o:p></p>
<p><span style="font-size:11.0pt;font-family:"Arial",sans-serif"><o:p> </o:p></span></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<div>
<div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt">
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Arial",sans-serif"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;mso-ligatures:standardcontextual">Emily Deng</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;mso-ligatures:standardcontextual">Best Wishes</span><o:p></o:p></p>
</div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Arial",sans-serif"> </span><o:p></o:p></p>
<p><span style="font-size:11.0pt;font-family:"Arial",sans-serif"> </span><o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre> <o:p></o:p></pre>
<pre>+ goto out_unlock_svms;<o:p></o:p></pre>
<pre>+ } else<o:p></o:p></pre>
<pre>+ /* ts is after svms->checkpoint_ts now, reset svms->checkpoint_ts<o:p></o:p></pre>
<pre>+ * to zero to avoid following ts wrap around give wrong comparing<o:p></o:p></pre>
<pre>+ */<o:p></o:p></pre>
<pre>+ svms->checkpoint_ts[gpuidx] = 0;<o:p></o:p></pre>
<pre>+ }<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre> prange = svm_range_from_addr(svms, addr, NULL);<o:p></o:p></pre>
<pre> if (!prange) {<o:p></o:p></pre>
<pre> pr_debug("failed to find prange svms 0x%p address [0x%llx]\n",<o:p></o:p></pre>
<pre>@@ -3165,7 +3166,8 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,<o:p></o:p></pre>
<pre> mutex_unlock(&svms->lock);<o:p></o:p></pre>
<pre> mmap_read_unlock(mm);<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>- svm_range_count_fault(node, p, gpuidx);<o:p></o:p></pre>
<pre>+ if (r != -EAGAIN)<o:p></o:p></pre>
<pre>+ svm_range_count_fault(node, p, gpuidx);<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> mmput(mm);<o:p></o:p></pre>
<pre> out:<o:p></o:p></pre>
</blockquote>
</div>
</blockquote>
</div>
</div>
</div>
</blockquote>
</body>
</html>