Messages in this thread |  | | Date | Mon, 26 Mar 2018 14:39:18 -0700 (PDT) | From | David Rientjes <> | Subject | Re: [PATCH v9 24/24] powerpc/mm: Add speculative page fault |
| |
On Tue, 13 Mar 2018, Laurent Dufour wrote:
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c > index 866446cf2d9a..104f3cc86b51 100644 > --- a/arch/powerpc/mm/fault.c > +++ b/arch/powerpc/mm/fault.c > @@ -392,6 +392,9 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address, > unsigned long error_code) > { > struct vm_area_struct * vma; > +#ifdef CONFIG_SPECULATIVE_PAGE_FAULT > + struct vm_area_struct *spf_vma = NULL; > +#endif > struct mm_struct *mm = current->mm; > unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE; > int is_exec = TRAP(regs) == 0x400; > @@ -459,6 +462,20 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address, > if (is_exec) > flags |= FAULT_FLAG_INSTRUCTION; > > +#ifdef CONFIG_SPECULATIVE_PAGE_FAULT > + if (is_user && (atomic_read(&mm->mm_users) > 1)) { > + /* let's try a speculative page fault without grabbing the > + * mmap_sem. > + */ > + fault = handle_speculative_fault(mm, address, flags, &spf_vma); > + if (!(fault & VM_FAULT_RETRY)) { > + perf_sw_event(PERF_COUNT_SW_SPF, 1, > + regs, address); > + goto done; > + } > + } > +#endif /* CONFIG_SPECULATIVE_PAGE_FAULT */ > +
Can't you elimiate all #ifdef's in this patch if handle_speculative_fault() can be passed is_user and return some error code that fallback is needed? Maybe reuse VM_FAULT_FALLBACK?
> /* When running in the kernel we expect faults to occur only to > * addresses in user space. All other faults represent errors in the > * kernel and should generate an OOPS. Unfortunately, in the case of an > @@ -489,7 +506,16 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address, > might_sleep(); > } > > - vma = find_vma(mm, address); > +#ifdef CONFIG_SPECULATIVE_PAGE_FAULT > + if (spf_vma) { > + if (can_reuse_spf_vma(spf_vma, address)) > + vma = spf_vma; > + else > + vma = find_vma(mm, address); > + spf_vma = NULL; > + } else > +#endif > + vma = find_vma(mm, address); > if (unlikely(!vma)) > return bad_area(regs, address); > if (likely(vma->vm_start <= address))
I think the code quality here could be improved such that you can pass mm, &spf_vma, and address and some helper function would return spf_vma if can_reuse_spf_vma() is true (and do *spf_vma to NULL) or otherwise return find_vma(mm, address).
Also, spf_vma is being set to NULL because of VM_FAULT_RETRY, but does it make sense to retry handle_speculative_fault() in this case since we've dropped mm->mmap_sem and there may have been a writer queued behind it?
> @@ -568,6 +594,9 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address, > > up_read(¤t->mm->mmap_sem); > > +#ifdef CONFIG_SPECULATIVE_PAGE_FAULT > +done: > +#endif > if (unlikely(fault & VM_FAULT_ERROR)) > return mm_fault_error(regs, address, fault); >
And things like this are trivially handled by doing
done: __maybe_unused
|  |