Message ID | 1520963994-28477-5-git-send-email-ldufour@linux.vnet.ibm.com (mailing list archive) |
---|---|
State | Not Applicable |
Headers | show |
Series | Speculative page faults | expand |
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; > } >
On 25/03/2018 23:50, David Rientjes wrote: > 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. I think you're right, I'll move down this define in the series. >> 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? Agreed. >> +{ >> + 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? You're right, now this label name is not correct, I'll rename it to "out_free_new" and rename also the label "oom" to "out" since it is generic too now. >> 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. I'll update it to : /* * Back out if the VMA has changed in our back during * a speculative page fault or if somebody else * faulted in this pte while we released the pte lock. */ > >> - 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. ok will do. > >> @@ -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. Ok changing 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. I think we can safely invert the call to mem_cgroup_cancel_charge() and to pte_unmap_unlock(), and then introduce a new label and jump in if pte_map_lock() failed. Something like this: @@ -3001,10 +3020,13 @@ int do_swap_page(struct vm_fault *vmf) } /* - * Back out if somebody else already faulted in this pte. + * Back out if the VMA has changed in our back during a speculative + * page fault or if somebody else already faulted in this pte. */ - vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, vmf->address, - &vmf->ptl); + if (!pte_map_lock(vmf)) { + ret = VM_FAULT_RETRY; + goto out_cancel_cgroup; + } if (unlikely(!pte_same(*vmf->pte, vmf->orig_pte))) goto out_nomap; @@ -3082,8 +3104,9 @@ int do_swap_page(struct vm_fault *vmf) out: return ret; out_nomap: - mem_cgroup_cancel_charge(page, memcg, false); pte_unmap_unlock(vmf->pte, vmf->ptl); +out_cancel_cgroup: + mem_cgroup_cancel_charge(page, memcg, false); out_page: unlock_page(page); out_release: >> @@ -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? I do agree, this was due to inverted calls to mem_cgroup_cancel_charge() and pte_unmap_unlock(). This will become: @@ -3170,14 +3193,16 @@ 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_none(*vmf->pte)) + if (!pte_map_lock(vmf)) { + ret = VM_FAULT_RETRY; goto release; + } + if (!pte_none(*vmf->pte)) + goto unlock_and_release; ret = check_stable_address_space(vma->vm_mm); if (ret) - goto release; + goto unlock_and_release; /* Deliver the page fault to userland, check inside PT lock */ if (userfaultfd_missing(vma)) { @@ -3199,10 +3224,12 @@ static int do_anonymous_page(struct vm_fault *vmf) unlock: pte_unmap_unlock(vmf->pte, vmf->ptl); return ret; +unlock_and_release: + pte_unmap_unlock(vmf->pte, vmf->ptl); release: mem_cgroup_cancel_charge(page, memcg, false); put_page(page); - goto unlock; + return ret; oom_free_page: put_page(page); oom: Thanks, Laurent. > >> @@ -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; >> } >> >
On Wed, 28 Mar 2018, Laurent Dufour wrote: > >> 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. > > I think you're right, I'll move down this define in the series. > > >> 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? > > Agreed. > Ignore this, the final form of the function after the full patchset shouldn't be 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? > > You're right, now this label name is not correct, I'll rename it to > "out_free_new" and rename also the label "oom" to "out" since it is generic too > now. > I think it would just be better to introduce a out_uncharge that handles the mem_cgroup_cancel_charge() in the exit path. diff --git a/mm/memory.c b/mm/memory.c --- a/mm/memory.c +++ b/mm/memory.c @@ -2645,9 +2645,8 @@ static int wp_page_copy(struct vm_fault *vmf) * Re-check the pte - we dropped the lock */ if (!pte_map_lock(vmf)) { - mem_cgroup_cancel_charge(new_page, memcg, false); ret = VM_FAULT_RETRY; - goto oom_free_new; + goto out_uncharge; } if (likely(pte_same(*vmf->pte, vmf->orig_pte))) { if (old_page) { @@ -2735,6 +2734,8 @@ static int wp_page_copy(struct vm_fault *vmf) put_page(old_page); } return page_copied ? VM_FAULT_WRITE : 0; +out_uncharge: + mem_cgroup_cancel_charge(new_page, memcg, false); oom_free_new: put_page(new_page); oom:
On 03/04/2018 23:57, David Rientjes wrote: > On Wed, 28 Mar 2018, Laurent Dufour wrote: > >>>> 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. >> >> I think you're right, I'll move down this define in the series. >> >>>> 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? >> >> Agreed. >> > > Ignore this, the final form of the function after the full patchset > shouldn't be inline. Indeed, I only kept as inlined the small pte_map_lock() and later pte_spinlock() defined when CONFIG_SPECULATIVE_PAGE_FAULT is not set. >>>> +{ >>>> + 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? >> >> You're right, now this label name is not correct, I'll rename it to >> "out_free_new" and rename also the label "oom" to "out" since it is generic too >> now. >> > > I think it would just be better to introduce a out_uncharge that handles > the mem_cgroup_cancel_charge() in the exit path. Yes adding an out_uncharge label sounds good too. I'll add it and also rename oom_* ones to out_*. > > diff --git a/mm/memory.c b/mm/memory.c > --- a/mm/memory.c > +++ b/mm/memory.c > @@ -2645,9 +2645,8 @@ static int wp_page_copy(struct vm_fault *vmf) > * Re-check the pte - we dropped the lock > */ > if (!pte_map_lock(vmf)) { > - mem_cgroup_cancel_charge(new_page, memcg, false); > ret = VM_FAULT_RETRY; > - goto oom_free_new; > + goto out_uncharge; > } > if (likely(pte_same(*vmf->pte, vmf->orig_pte))) { > if (old_page) { > @@ -2735,6 +2734,8 @@ static int wp_page_copy(struct vm_fault *vmf) > put_page(old_page); > } > return page_copied ? VM_FAULT_WRITE : 0; > +out_uncharge: > + mem_cgroup_cancel_charge(new_page, memcg, false); > oom_free_new: > put_page(new_page); > oom: >
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" }, \ 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) +{ + 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; + } 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. */ - 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); @@ -3003,8 +3020,11 @@ int do_swap_page(struct vm_fault *vmf) /* * Back out if somebody else already faulted in this pte. */ - 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; @@ -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; @@ -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; }