lkml.org 
[lkml]   [2018]   [Mar]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v9 04/24] mm: Prepare for FAULT_FLAG_SPECULATIVE
    On Tue, 13 Mar 2018, Laurent Dufour wrote:

    > From: Peter Zijlstra <peterz@infradead.org>
    >
    > When speculating faults (without holding mmap_sem) we need to validate
    > that the vma against which we loaded pages is still valid when we're
    > ready to install the new PTE.
    >
    > Therefore, replace the pte_offset_map_lock() calls that (re)take the
    > PTL with pte_map_lock() which can fail in case we find the VMA changed
    > since we started the fault.
    >

    Based on how its used, I would have suspected this to be named
    pte_map_trylock().

    > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
    >
    > [Port to 4.12 kernel]
    > [Remove the comment about the fault_env structure which has been
    > implemented as the vm_fault structure in the kernel]
    > [move pte_map_lock()'s definition upper in the file]
    > Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
    > ---
    > include/linux/mm.h | 1 +
    > mm/memory.c | 56 ++++++++++++++++++++++++++++++++++++++----------------
    > 2 files changed, 41 insertions(+), 16 deletions(-)
    >
    > diff --git a/include/linux/mm.h b/include/linux/mm.h
    > index 4d02524a7998..2f3e98edc94a 100644
    > --- a/include/linux/mm.h
    > +++ b/include/linux/mm.h
    > @@ -300,6 +300,7 @@ extern pgprot_t protection_map[16];
    > #define FAULT_FLAG_USER 0x40 /* The fault originated in userspace */
    > #define FAULT_FLAG_REMOTE 0x80 /* faulting for non current tsk/mm */
    > #define FAULT_FLAG_INSTRUCTION 0x100 /* The fault was during an instruction fetch */
    > +#define FAULT_FLAG_SPECULATIVE 0x200 /* Speculative fault, not holding mmap_sem */
    >
    > #define FAULT_FLAG_TRACE \
    > { FAULT_FLAG_WRITE, "WRITE" }, \

    I think FAULT_FLAG_SPECULATIVE should be introduced in the patch that
    actually uses it.

    > diff --git a/mm/memory.c b/mm/memory.c
    > index e0ae4999c824..8ac241b9f370 100644
    > --- a/mm/memory.c
    > +++ b/mm/memory.c
    > @@ -2288,6 +2288,13 @@ int apply_to_page_range(struct mm_struct *mm, unsigned long addr,
    > }
    > EXPORT_SYMBOL_GPL(apply_to_page_range);
    >
    > +static bool pte_map_lock(struct vm_fault *vmf)

    inline?

    > +{
    > + vmf->pte = pte_offset_map_lock(vmf->vma->vm_mm, vmf->pmd,
    > + vmf->address, &vmf->ptl);
    > + return true;
    > +}
    > +
    > /*
    > * handle_pte_fault chooses page fault handler according to an entry which was
    > * read non-atomically. Before making any commitment, on those architectures
    > @@ -2477,6 +2484,7 @@ static int wp_page_copy(struct vm_fault *vmf)
    > const unsigned long mmun_start = vmf->address & PAGE_MASK;
    > const unsigned long mmun_end = mmun_start + PAGE_SIZE;
    > struct mem_cgroup *memcg;
    > + int ret = VM_FAULT_OOM;
    >
    > if (unlikely(anon_vma_prepare(vma)))
    > goto oom;
    > @@ -2504,7 +2512,11 @@ static int wp_page_copy(struct vm_fault *vmf)
    > /*
    > * Re-check the pte - we dropped the lock
    > */
    > - vmf->pte = pte_offset_map_lock(mm, vmf->pmd, vmf->address, &vmf->ptl);
    > + if (!pte_map_lock(vmf)) {
    > + mem_cgroup_cancel_charge(new_page, memcg, false);
    > + ret = VM_FAULT_RETRY;
    > + goto oom_free_new;
    > + }

    Ugh, but we aren't oom here, so maybe rename oom_free_new so that it makes
    sense for return values other than VM_FAULT_OOM?

    > if (likely(pte_same(*vmf->pte, vmf->orig_pte))) {
    > if (old_page) {
    > if (!PageAnon(old_page)) {
    > @@ -2596,7 +2608,7 @@ static int wp_page_copy(struct vm_fault *vmf)
    > oom:
    > if (old_page)
    > put_page(old_page);
    > - return VM_FAULT_OOM;
    > + return ret;
    > }
    >
    > /**
    > @@ -2617,8 +2629,8 @@ static int wp_page_copy(struct vm_fault *vmf)
    > int finish_mkwrite_fault(struct vm_fault *vmf)
    > {
    > WARN_ON_ONCE(!(vmf->vma->vm_flags & VM_SHARED));
    > - vmf->pte = pte_offset_map_lock(vmf->vma->vm_mm, vmf->pmd, vmf->address,
    > - &vmf->ptl);
    > + if (!pte_map_lock(vmf))
    > + return VM_FAULT_RETRY;
    > /*
    > * We might have raced with another page fault while we released the
    > * pte_offset_map_lock.
    > @@ -2736,8 +2748,11 @@ static int do_wp_page(struct vm_fault *vmf)
    > get_page(vmf->page);
    > pte_unmap_unlock(vmf->pte, vmf->ptl);
    > lock_page(vmf->page);
    > - vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
    > - vmf->address, &vmf->ptl);
    > + if (!pte_map_lock(vmf)) {
    > + unlock_page(vmf->page);
    > + put_page(vmf->page);
    > + return VM_FAULT_RETRY;
    > + }
    > if (!pte_same(*vmf->pte, vmf->orig_pte)) {
    > unlock_page(vmf->page);
    > pte_unmap_unlock(vmf->pte, vmf->ptl);
    > @@ -2947,8 +2962,10 @@ int do_swap_page(struct vm_fault *vmf)
    > * Back out if somebody else faulted in this pte
    > * while we released the pte lock.
    > */

    Comment needs updating, pte_same() isn't the only reason to bail out here.

    > - vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
    > - vmf->address, &vmf->ptl);
    > + if (!pte_map_lock(vmf)) {
    > + delayacct_clear_flag(DELAYACCT_PF_SWAPIN);
    > + return VM_FAULT_RETRY;
    > + }
    > if (likely(pte_same(*vmf->pte, vmf->orig_pte)))
    > ret = VM_FAULT_OOM;
    > delayacct_clear_flag(DELAYACCT_PF_SWAPIN);

    Not crucial, but it would be nice if this could do goto out instead,
    otherwise this is the first mid function return.

    > @@ -3003,8 +3020,11 @@ int do_swap_page(struct vm_fault *vmf)
    > /*
    > * Back out if somebody else already faulted in this pte.
    > */

    Same as above.

    > - vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, vmf->address,
    > - &vmf->ptl);
    > + if (!pte_map_lock(vmf)) {
    > + ret = VM_FAULT_RETRY;
    > + mem_cgroup_cancel_charge(page, memcg, false);
    > + goto out_page;
    > + }
    > if (unlikely(!pte_same(*vmf->pte, vmf->orig_pte)))
    > goto out_nomap;
    >

    mem_cgroup_try_charge() is done before grabbing pte_offset_map_lock(), why
    does the out_nomap exit path do mem_cgroup_cancel_charge();
    pte_unmap_unlock()? If the pte lock can be droppde first, there's no need
    to embed the mem_cgroup_cancel_charge() here.

    > @@ -3133,8 +3153,8 @@ static int do_anonymous_page(struct vm_fault *vmf)
    > !mm_forbids_zeropage(vma->vm_mm)) {
    > entry = pte_mkspecial(pfn_pte(my_zero_pfn(vmf->address),
    > vma->vm_page_prot));
    > - vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
    > - vmf->address, &vmf->ptl);
    > + if (!pte_map_lock(vmf))
    > + return VM_FAULT_RETRY;
    > if (!pte_none(*vmf->pte))
    > goto unlock;
    > ret = check_stable_address_space(vma->vm_mm);
    > @@ -3169,8 +3189,11 @@ static int do_anonymous_page(struct vm_fault *vmf)
    > if (vma->vm_flags & VM_WRITE)
    > entry = pte_mkwrite(pte_mkdirty(entry));
    >
    > - vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, vmf->address,
    > - &vmf->ptl);
    > + if (!pte_map_lock(vmf)) {
    > + mem_cgroup_cancel_charge(page, memcg, false);
    > + put_page(page);
    > + return VM_FAULT_RETRY;
    > + }
    > if (!pte_none(*vmf->pte))
    > goto release;
    >

    This is more spaghetti, can the exit path be fixed up so we order things
    consistently for all gotos?

    > @@ -3294,8 +3317,9 @@ static int pte_alloc_one_map(struct vm_fault *vmf)
    > * pte_none() under vmf->ptl protection when we return to
    > * alloc_set_pte().
    > */
    > - vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, vmf->address,
    > - &vmf->ptl);
    > + if (!pte_map_lock(vmf))
    > + return VM_FAULT_RETRY;
    > +
    > return 0;
    > }
    >

    \
     
     \ /
      Last update: 2018-03-25 23:52    [W:9.103 / U:1.532 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site