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
