<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<div dir="auto">
<div><br>
<div class="gmail_extra"><br>
<div class="gmail_quote">Am 17.10.2019 18:26 schrieb "Yang, Philip" <Philip.Yang@amd.com>:<br type="attribution">
<blockquote class="quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><font size="2"><span style="font-size:11pt">
<div><br>
<br>
On 2019-10-17 4:54 a.m., Christian König wrote:<br>
> Am 16.10.19 um 18:04 schrieb Jason Gunthorpe:<br>
>> On Wed, Oct 16, 2019 at 10:58:02AM +0200, Christian König wrote:<br>
>>> Am 15.10.19 um 20:12 schrieb Jason Gunthorpe:<br>
>>>> From: Jason Gunthorpe <jgg@mellanox.com><br>
>>>><br>
>>>> 8 of the mmu_notifier using drivers (i915_gem, radeon_mn, umem_odp, <br>
>>>> hfi1,<br>
>>>> scif_dma, vhost, gntdev, hmm) drivers are using a common pattern where<br>
>>>> they only use invalidate_range_start/end and immediately check the<br>
>>>> invalidating range against some driver data structure to tell if the<br>
>>>> driver is interested. Half of them use an interval_tree, the others are<br>
>>>> simple linear search lists.<br>
>>>><br>
>>>> Of the ones I checked they largely seem to have various kinds of races,<br>
>>>> bugs and poor implementation. This is a result of the complexity in how<br>
>>>> the notifier interacts with get_user_pages(). It is extremely <br>
>>>> difficult to<br>
>>>> use it correctly.<br>
>>>><br>
>>>> Consolidate all of this code together into the core mmu_notifier and<br>
>>>> provide a locking scheme similar to hmm_mirror that allows the user to<br>
>>>> safely use get_user_pages() and reliably know if the page list still<br>
>>>> matches the mm.<br>
>>> That sounds really good, but could you outline for a moment how that is<br>
>>> archived?<br>
>> It uses the same basic scheme as hmm and rdma odp, outlined in the<br>
>> revisions to hmm.rst later on.<br>
>><br>
>> Basically,<br>
>><br>
>> seq = mmu_range_read_begin(&mrn);<br>
>><br>
>> // This is a speculative region<br>
>> .. get_user_pages()/hmm_range_fault() ..<br>
> <br>
> How do we enforce that this get_user_pages()/hmm_range_fault() doesn't <br>
> see outdated page table information?<br>
> <br>
> In other words how the the following race prevented:<br>
> <br>
> CPU A CPU B<br>
> invalidate_range_start()<br>
> mmu_range_read_begin()<br>
> get_user_pages()/hmm_range_fault()<br>
> Updating the ptes<br>
> invalidate_range_end()<br>
> <br>
> <br>
> I mean get_user_pages() tries to circumvent this issue by grabbing a <br>
> reference to the pages in question, but that isn't sufficient for the <br>
> SVM use case.<br>
> <br>
> That's the reason why we had this horrible solution with a r/w lock and <br>
> a linked list of BOs in an interval tree.<br>
> <br>
> Regards,<br>
> Christian.<br>
get_user_pages/hmm_range_fault() and invalidate_range_start() both are <br>
called while holding mm->map_sem, so they are always serialized.<br>
</div>
</span></font></div>
</blockquote>
</div>
</div>
</div>
<div dir="auto"><br>
</div>
<div dir="auto">Not even remotely. </div>
<div dir="auto"><br>
</div>
<div dir="auto">For calling get_user_pages()/hmm_range_fault() you only need to hold the mmap_sem in read mode.</div>
<div dir="auto"><br>
</div>
<div dir="auto">And IIRC invalidate_range_start() is sometimes called without holding the mmap_sem at all.</div>
<div dir="auto"><br>
</div>
<div dir="auto">So again how are they serialized?</div>
<div dir="auto"><br>
</div>
<div dir="auto">Regards,</div>
<div dir="auto">Christian.</div>
<div dir="auto"><br>
</div>
<div dir="auto">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><font size="2"><span style="font-size:11pt">
<div><br>
Philip<br>
> <br>
>> // Result cannot be derferenced<br>
>><br>
>> take_lock(driver->update);<br>
>> if (mmu_range_read_retry(&mrn, range.notifier_seq) {<!-- --><br>
>> // collision! The results are not correct<br>
>> goto again<br>
>> }<br>
>><br>
>> // no collision, and now under lock. Now we can de-reference the <br>
>> pages/etc<br>
>> // program HW<br>
>> // Now the invalidate callback is responsible to synchronize against <br>
>> changes<br>
>> unlock(driver->update)<br>
>><br>
>> Basically, anything that was using hmm_mirror correctly transisions<br>
>> over fairly trivially, just with the modification to store a sequence<br>
>> number to close that race described in the hmm commit.<br>
>><br>
>> For something like AMD gpu I expect it to transition to use dma_fence<br>
>> from the notifier for coherency right before it unlocks driver->update.<br>
>><br>
>> Jason<br>
>> _______________________________________________<br>
>> amd-gfx mailing list<br>
>> amd-gfx@lists.freedesktop.org<br>
>> <a href="https://lists.freedesktop.org/mailman/listinfo/amd-gfx">https://lists.freedesktop.org/mailman/listinfo/amd-gfx</a><br>
> <br>
> _______________________________________________<br>
> amd-gfx mailing list<br>
> amd-gfx@lists.freedesktop.org<br>
> <a href="https://lists.freedesktop.org/mailman/listinfo/amd-gfx">https://lists.freedesktop.org/mailman/listinfo/amd-gfx</a><br>
</div>
</span></font></div>
</blockquote>
</div>
<br>
</div>
</div>
</div>
</body>
</html>