Messages in this thread | | | Date | Tue, 22 Sep 2020 16:11:16 -0300 | From | Jason Gunthorpe <> | Subject | Re: [PATCH 1/5] mm: Introduce mm_struct.has_pinned |
| |
On Tue, Sep 22, 2020 at 01:54:15PM -0400, Peter Xu wrote: > diff --git a/mm/memory.c b/mm/memory.c > index 8f3521be80ca..6591f3f33299 100644 > +++ b/mm/memory.c > @@ -888,8 +888,8 @@ copy_one_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm, > * Because we'll need to release the locks before doing cow, > * pass this work to upper layer. > */ > - if (READ_ONCE(src_mm->has_pinned) && wp && > - page_maybe_dma_pinned(page)) { > + if (wp && page_maybe_dma_pinned(page) && > + READ_ONCE(src_mm->has_pinned)) { > /* We've got the page already; we're safe */ > data->cow_old_page = page; > data->cow_oldpte = *src_pte; > > I can also add some more comment to emphasize this.
It is not just that, but the ptep_set_wrprotect() has to be done earlier.
Otherwise it races like:
pin_user_pages_fast() fork() atomic_set(has_pinned, 1); [..] atomic_read(page->_refcount) //false // skipped atomic_read(has_pinned) atomic_add(page->_refcount) ordered check write protect() ordered set write protect()
And now have a write protect on a DMA pinned page, which is the invarient we are trying to create.
The best algorithm I've thought of is something like:
pte_map_lock() if (page) { if (wp) { ptep_set_wrprotect() /* Order with try_grab_compound_head(), either we see * page_maybe_dma_pinned(), or they see the wrprotect */ get_page();
if (page_maybe_dma_pinned() && READ_ONCE(src_mm->has_pinned)) { put_page(); ptep_clear_wrprotect()
// do copy return } } else { get_page(); } page_dup_rmap() pte_unmap_lock()
Then the do_wp_page() path would have to detect that the page is not write protected under the pte lock inside the fault handler and just do nothing. Ie the set/clear could be visible to the CPU and trigger a spurious fault, but never trigger a COW.
Thus 'wp' becomes a 'lock' that prevents GUP from returning this page.
Very tricky, deserves a huge comment near the ptep_clear_wrprotect()
Consider the above algorithm beside the gup_fast() algorithm:
if (!pte_access_permitted(pte, flags & FOLL_WRITE)) goto pte_unmap; [..] head = try_grab_compound_head(page, 1, flags); if (!head) goto pte_unmap; if (unlikely(pte_val(pte) != pte_val(*ptep))) { put_compound_head(head, 1, flags); goto pte_unmap;
That last *ptep will check that the WP is not set after making page_maybe_dma_pinned() true.
It still looks reasonable, the extra work is still just the additional atomic in page_maybe_dma_pinned(), just everything else has to be very carefully sequenced due to unlocked page table accessors.
> I think the WRITE_ONCE/READ_ONCE can actually be kept, because atomic ops > should contain proper memory barriers already so the memory access orders > should be guaranteed
I always have to carefully check ORDERING in Documentation/atomic_t.txt when asking those questions..
It seems very subtle to me, but yes, try_grab_compound_head() and page_maybe_dma_pinned() are already paired ordering barriers, so both the pte_val() on the GUP side and the READ_ONCE(has_pinned) look OK.
Jason
| |