Re: [PATCH v2 14/20] mm: Provide speculative fault infrastructure
From: Laurent Dufour <hidden>
Date: 2017-08-30 09:53:55
Also in:
linux-mm, lkml
On 30/08/2017 07:03, Anshuman Khandual wrote:
On 08/29/2017 07:15 PM, Peter Zijlstra wrote:quoted
On Tue, Aug 29, 2017 at 03:18:25PM +0200, Laurent Dufour wrote:quoted
On 29/08/2017 14:04, Peter Zijlstra wrote:quoted
On Tue, Aug 29, 2017 at 09:59:30AM +0200, Laurent Dufour wrote:quoted
On 27/08/2017 02:18, Kirill A. Shutemov wrote:quoted
quoted
+ + if (unlikely(!vma->anon_vma)) + goto unlock;It deserves a comment.You're right I'll add it in the next version. For the record, the root cause is that __anon_vma_prepare() requires the mmap_sem to be held because vm_next and vm_prev must be safe.But should that test not be: if (unlikely(vma_is_anonymous(vma) && !vma->anon_vma)) goto unlock; Because !anon vmas will never have ->anon_vma set and you don't want to exclude those.Yes in the case we later allow non anonymous vmas to be handled. Currently only anonymous vmas are supported so the check is good enough, isn't it ?That wasn't at all clear from reading the code. This makes it clear ->anon_vma is only ever looked at for anonymous. And like Kirill says, we _really_ should start allowing some (if not all) vm_ops. Large file based mappings aren't particularly rare. I'm not sure we want to introduce a white-list or just bite the bullet and audit all ->fault() implementations. But either works and isn't terribly difficult, auditing all is more work though.filemap_fault() is used as vma-vm_ops->fault() for most of the file systems. Changing it can enable speculative fault support for all of them. It will still exclude other driver based vma-vm_ops->fault() implementation. AFAICS, __lock_page_or_retry() function can drop mm->mmap_sem if the page could not be locked right away. As suggested by Peterz, making it understand FAULT_FLAG_SPECULATIVE should be good enough. The patch is lightly tested for file mappings on top of this series.
Hi Anshuman, This sounds pretty good, except for the FAULT_FLAG_RETRY_NOWAIT's case I mentioned in another mail. The next step would be to find a way to discriminate between the vm_fault() functions. Any idea ? Thanks, Laurent.
quoted hunk ↗ jump to hunk
diff --git a/mm/filemap.c b/mm/filemap.c index a497024..08f3042 100644 --- a/mm/filemap.c +++ b/mm/filemap.c@@ -1181,6 +1181,18 @@ int __lock_page_killable(struct page *__page) int __lock_page_or_retry(struct page *page, struct mm_struct *mm, unsigned int flags) { + if (flags & FAULT_FLAG_SPECULATIVE) { + if (flags & FAULT_FLAG_KILLABLE) { + int ret; + + ret = __lock_page_killable(page); + if (ret) + return 0; + } else + __lock_page(page); + return 1; + } + if (flags & FAULT_FLAG_ALLOW_RETRY) { /* * CAUTION! In this case, mmap_sem is not releaseddiff --git a/mm/memory.c b/mm/memory.c index 549d235..02347f3 100644 --- a/mm/memory.c +++ b/mm/memory.c@@ -3836,8 +3836,6 @@ static int handle_pte_fault(struct vm_fault *vmf) if (!vmf->pte) { if (vma_is_anonymous(vmf->vma)) return do_anonymous_page(vmf); - else if (vmf->flags & FAULT_FLAG_SPECULATIVE) - return VM_FAULT_RETRY; else return do_fault(vmf); }@@ -4012,17 +4010,7 @@ int handle_speculative_fault(struct mm_struct *mm, unsigned long address, goto unlock; } - /* - * Can't call vm_ops service has we don't know what they would do - * with the VMA. - * This include huge page from hugetlbfs. - */ - if (vma->vm_ops) { - trace_spf_vma_notsup(_RET_IP_, vma, address); - goto unlock; - } - - if (unlikely(!vma->anon_vma)) { + if (unlikely(vma_is_anonymous(vma) && !vma->anon_vma)) { trace_spf_vma_notsup(_RET_IP_, vma, address); goto unlock; }