| Message ID | 20201208172901.17384-11-joao.m.martins@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm, sparse-vmemmap: Introduce compound pagemaps | expand |
On Tue, Dec 08, 2020 at 05:29:01PM +0000, Joao Martins wrote: > Similar to follow_hugetlb_page() add a follow_devmap_page which rather > than calling follow_page() per 4K page in a PMD/PUD it does so for the > entire PMD, where we lock the pmd/pud, get all pages , unlock. > > While doing so, we only change the refcount once when PGMAP_COMPOUND is > passed in. > > This let us improve {pin,get}_user_pages{,_longterm}() considerably: > > $ gup_benchmark -f /dev/dax0.2 -m 16384 -r 10 -S [-U,-b,-L] -n 512 -w > > (<test>) [before] -> [after] > (get_user_pages 2M pages) ~150k us -> ~8.9k us > (pin_user_pages 2M pages) ~192k us -> ~9k us > (pin_user_pages_longterm 2M pages) ~200k us -> ~19k us > > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > --- > I've special-cased this to device-dax vmas given its similar page size > guarantees as hugetlbfs, but I feel this is a bit wrong. I am > replicating follow_hugetlb_page() as RFC ought to seek feedback whether > this should be generalized if no fundamental issues exist. In such case, > should I be changing follow_page_mask() to take either an array of pages > or a function pointer and opaque arguments which would let caller pick > its structure? I would be extremely sad if this was the only way to do this :( We should be trying to make things more general. The hmm_range_fault_path() doesn't have major special cases for device, I am struggling to understand why gup fast and slow do. What we've talked about is changing the calling convention across all of this to something like: struct gup_output { struct page **cur; struct page **end; unsigned long vaddr; [..] } And making the manipulator like you saw for GUP common: gup_output_single_page() gup_output_pages() Then putting this eveywhere. This is the pattern that we ended up with in hmm_range_fault, and it seems to be working quite well. fast/slow should be much more symmetric in code than they are today, IMHO.. I think those differences mainly exist because it used to be siloed in arch code. Some of the differences might be bugs, we've seen that a few times at least.. Jason
On 12/8/20 9:29 AM, Joao Martins wrote: > Similar to follow_hugetlb_page() add a follow_devmap_page which rather > than calling follow_page() per 4K page in a PMD/PUD it does so for the > entire PMD, where we lock the pmd/pud, get all pages , unlock. > > While doing so, we only change the refcount once when PGMAP_COMPOUND is > passed in. > > This let us improve {pin,get}_user_pages{,_longterm}() considerably: > > $ gup_benchmark -f /dev/dax0.2 -m 16384 -r 10 -S [-U,-b,-L] -n 512 -w > d> (<test>) [before] -> [after] > (get_user_pages 2M pages) ~150k us -> ~8.9k us > (pin_user_pages 2M pages) ~192k us -> ~9k us > (pin_user_pages_longterm 2M pages) ~200k us -> ~19k us > Yes, this is a massive improvement. > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > --- > I've special-cased this to device-dax vmas given its similar page size > guarantees as hugetlbfs, but I feel this is a bit wrong. I am > replicating follow_hugetlb_page() as RFC ought to seek feedback whether > this should be generalized if no fundamental issues exist. In such case, > should I be changing follow_page_mask() to take either an array of pages > or a function pointer and opaque arguments which would let caller pick > its structure? I don't know which approach is better because I haven't yet attempted to find the common elements in these routines. But if there is *any way* to avoid this copy-paste creation of yet more following of pages, then it's *very* good to do. thanks,
On Tue, Dec 08, 2020 at 03:57:54PM -0400, Jason Gunthorpe wrote: > What we've talked about is changing the calling convention across all > of this to something like: > > struct gup_output { > struct page **cur; > struct page **end; > unsigned long vaddr; > [..] > } > > And making the manipulator like you saw for GUP common: > > gup_output_single_page() > gup_output_pages() > > Then putting this eveywhere. This is the pattern that we ended up with > in hmm_range_fault, and it seems to be working quite well. > > fast/slow should be much more symmetric in code than they are today, > IMHO.. I think those differences mainly exist because it used to be > siloed in arch code. Some of the differences might be bugs, we've seen > that a few times at least.. something like this: http://git.infradead.org/users/hch/misc.git/commitdiff/c3d019802dbde5a4cc4160e7ec8ccba479b19f97 from this old and not fully working series: http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/gup-bvec
On 12/8/20 7:57 PM, Jason Gunthorpe wrote: > On Tue, Dec 08, 2020 at 05:29:01PM +0000, Joao Martins wrote: >> Similar to follow_hugetlb_page() add a follow_devmap_page which rather >> than calling follow_page() per 4K page in a PMD/PUD it does so for the >> entire PMD, where we lock the pmd/pud, get all pages , unlock. >> >> While doing so, we only change the refcount once when PGMAP_COMPOUND is >> passed in. >> >> This let us improve {pin,get}_user_pages{,_longterm}() considerably: >> >> $ gup_benchmark -f /dev/dax0.2 -m 16384 -r 10 -S [-U,-b,-L] -n 512 -w >> >> (<test>) [before] -> [after] >> (get_user_pages 2M pages) ~150k us -> ~8.9k us >> (pin_user_pages 2M pages) ~192k us -> ~9k us >> (pin_user_pages_longterm 2M pages) ~200k us -> ~19k us >> >> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >> --- >> I've special-cased this to device-dax vmas given its similar page size >> guarantees as hugetlbfs, but I feel this is a bit wrong. I am >> replicating follow_hugetlb_page() as RFC ought to seek feedback whether >> this should be generalized if no fundamental issues exist. In such case, >> should I be changing follow_page_mask() to take either an array of pages >> or a function pointer and opaque arguments which would let caller pick >> its structure? > > I would be extremely sad if this was the only way to do this :( > > We should be trying to make things more general. Yeap, indeed. Specially, when similar problem is observed for THP, at least from the measurements I saw. It is all slow, except for hugetlbfs. > The > hmm_range_fault_path() doesn't have major special cases for device, I > am struggling to understand why gup fast and slow do. > > What we've talked about is changing the calling convention across all > of this to something like: > > struct gup_output { > struct page **cur; > struct page **end; > unsigned long vaddr; > [..] > } > > And making the manipulator like you saw for GUP common: > > gup_output_single_page() > gup_output_pages() > > Then putting this eveywhere. This is the pattern that we ended up with > in hmm_range_fault, and it seems to be working quite well. > > fast/slow should be much more symmetric in code than they are today, > IMHO.. Thanks for the suggestions above. I think those differences mainly exist because it used to be > siloed in arch code. Some of the differences might be bugs, we've seen > that a few times at least.. Interesting, wasn't aware of the siloing. I'll go investigate how this all refactoring goes together, at the point of which a future iteration of this particular patch probably needs to move independently from this series. Joao
diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h index 0365aa97f8e7..da87ecea19e6 100644 --- a/include/linux/huge_mm.h +++ b/include/linux/huge_mm.h @@ -293,6 +293,10 @@ struct page *follow_devmap_pmd(struct vm_area_struct *vma, unsigned long addr, pmd_t *pmd, int flags, struct dev_pagemap **pgmap); struct page *follow_devmap_pud(struct vm_area_struct *vma, unsigned long addr, pud_t *pud, int flags, struct dev_pagemap **pgmap); +long follow_devmap_page(struct mm_struct *mm, struct vm_area_struct *vma, + struct page **pages, struct vm_area_struct **vmas, + unsigned long *position, unsigned long *nr_pages, + long i, unsigned int flags, int *locked); extern vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf, pmd_t orig_pmd); diff --git a/include/linux/mm.h b/include/linux/mm.h index 8b0155441835..466c88679628 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -1164,6 +1164,8 @@ static inline void get_page(struct page *page) page_ref_inc(page); } +__maybe_unused struct page *try_grab_compound_head(struct page *page, int refs, + unsigned int flags); bool __must_check try_grab_page(struct page *page, unsigned int flags); static inline __must_check bool try_get_page(struct page *page) diff --git a/mm/gup.c b/mm/gup.c index 3a9a7229f418..50effb9cc349 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -78,7 +78,7 @@ static inline struct page *try_get_compound_head(struct page *page, int refs) * considered failure, and furthermore, a likely bug in the caller, so a warning * is also emitted. */ -static __maybe_unused struct page *try_grab_compound_head(struct page *page, +__maybe_unused struct page *try_grab_compound_head(struct page *page, int refs, unsigned int flags) { @@ -880,8 +880,8 @@ static int get_gate_page(struct mm_struct *mm, unsigned long address, * does not include FOLL_NOWAIT, the mmap_lock may be released. If it * is, *@locked will be set to 0 and -EBUSY returned. */ -static int faultin_page(struct vm_area_struct *vma, - unsigned long address, unsigned int *flags, int *locked) +int faultin_page(struct vm_area_struct *vma, + unsigned long address, unsigned int *flags, int *locked) { unsigned int fault_flags = 0; vm_fault_t ret; @@ -1103,6 +1103,22 @@ static long __get_user_pages(struct mm_struct *mm, } continue; } + if (vma_is_dax(vma)) { + i = follow_devmap_page(mm, vma, pages, vmas, + &start, &nr_pages, i, + gup_flags, locked); + if (locked && *locked == 0) { + /* + * We've got a VM_FAULT_RETRY + * and we've lost mmap_lock. + * We must stop here. + */ + BUG_ON(gup_flags & FOLL_NOWAIT); + BUG_ON(ret != 0); + goto out; + } + continue; + } } retry: /* diff --git a/mm/huge_memory.c b/mm/huge_memory.c index ec2bb93f7431..20bfbf211dc3 100644 --- a/mm/huge_memory.c +++ b/mm/huge_memory.c @@ -1168,6 +1168,208 @@ struct page *follow_devmap_pud(struct vm_area_struct *vma, unsigned long addr, return page; } +long follow_devmap_page(struct mm_struct *mm, struct vm_area_struct *vma, + struct page **pages, struct vm_area_struct **vmas, + unsigned long *position, unsigned long *nr_pages, + long i, unsigned int flags, int *locked) +{ + unsigned long pfn_offset; + unsigned long vaddr = *position; + unsigned long remainder = *nr_pages; + unsigned long align = vma_kernel_pagesize(vma); + unsigned long align_nr_pages = align >> PAGE_SHIFT; + unsigned long mask = ~(align-1); + unsigned long nr_pages_hpage = 0; + struct dev_pagemap *pgmap = NULL; + int err = -EFAULT; + + if (align == PAGE_SIZE) + return i; + + while (vaddr < vma->vm_end && remainder) { + pte_t *pte; + spinlock_t *ptl = NULL; + int absent; + struct page *page; + + /* + * If we have a pending SIGKILL, don't keep faulting pages and + * potentially allocating memory. + */ + if (fatal_signal_pending(current)) { + remainder = 0; + break; + } + + /* + * Some archs (sparc64, sh*) have multiple pte_ts to + * each hugepage. We have to make sure we get the + * first, for the page indexing below to work. + * + * Note that page table lock is not held when pte is null. + */ + pte = huge_pte_offset(mm, vaddr & mask, align); + if (pte) { + if (align == PMD_SIZE) + ptl = pmd_lockptr(mm, (pmd_t *) pte); + else if (align == PUD_SIZE) + ptl = pud_lockptr(mm, (pud_t *) pte); + spin_lock(ptl); + } + absent = !pte || pte_none(ptep_get(pte)); + + if (absent && (flags & FOLL_DUMP)) { + if (pte) + spin_unlock(ptl); + remainder = 0; + break; + } + + if (absent || + ((flags & FOLL_WRITE) && + !pte_write(ptep_get(pte)))) { + vm_fault_t ret; + unsigned int fault_flags = 0; + + if (pte) + spin_unlock(ptl); + if (flags & FOLL_WRITE) + fault_flags |= FAULT_FLAG_WRITE; + if (locked) + fault_flags |= FAULT_FLAG_ALLOW_RETRY | + FAULT_FLAG_KILLABLE; + if (flags & FOLL_NOWAIT) + fault_flags |= FAULT_FLAG_ALLOW_RETRY | + FAULT_FLAG_RETRY_NOWAIT; + if (flags & FOLL_TRIED) { + /* + * Note: FAULT_FLAG_ALLOW_RETRY and + * FAULT_FLAG_TRIED can co-exist + */ + fault_flags |= FAULT_FLAG_TRIED; + } + ret = handle_mm_fault(vma, vaddr, flags, NULL); + if (ret & VM_FAULT_ERROR) { + err = vm_fault_to_errno(ret, flags); + remainder = 0; + break; + } + if (ret & VM_FAULT_RETRY) { + if (locked && + !(fault_flags & FAULT_FLAG_RETRY_NOWAIT)) + *locked = 0; + *nr_pages = 0; + /* + * VM_FAULT_RETRY must not return an + * error, it will return zero + * instead. + * + * No need to update "position" as the + * caller will not check it after + * *nr_pages is set to 0. + */ + return i; + } + continue; + } + + pfn_offset = (vaddr & ~mask) >> PAGE_SHIFT; + page = pte_page(ptep_get(pte)); + + pgmap = get_dev_pagemap(page_to_pfn(page), pgmap); + if (!pgmap) { + spin_unlock(ptl); + remainder = 0; + err = -EFAULT; + break; + } + + /* + * If subpage information not requested, update counters + * and skip the same_page loop below. + */ + if (!pages && !vmas && !pfn_offset && + (vaddr + align < vma->vm_end) && + (remainder >= (align_nr_pages))) { + vaddr += align; + remainder -= align_nr_pages; + i += align_nr_pages; + spin_unlock(ptl); + continue; + } + + nr_pages_hpage = 0; + +same_page: + if (pages) { + pages[i] = mem_map_offset(page, pfn_offset); + + /* + * try_grab_page() should always succeed here, because: + * a) we hold the ptl lock, and b) we've just checked + * that the huge page is present in the page tables. + */ + if (!(pgmap->flags & PGMAP_COMPOUND) && + WARN_ON_ONCE(!try_grab_page(pages[i], flags))) { + spin_unlock(ptl); + remainder = 0; + err = -ENOMEM; + break; + } + + } + + if (vmas) + vmas[i] = vma; + + vaddr += PAGE_SIZE; + ++pfn_offset; + --remainder; + ++i; + nr_pages_hpage++; + if (vaddr < vma->vm_end && remainder && + pfn_offset < align_nr_pages) { + /* + * We use pfn_offset to avoid touching the pageframes + * of this compound page. + */ + goto same_page; + } else { + /* + * try_grab_compound_head() should always succeed here, + * because: a) we hold the ptl lock, and b) we've just + * checked that the huge page is present in the page + * tables. If the huge page is present, then the tail + * pages must also be present. The ptl prevents the + * head page and tail pages from being rearranged in + * any way. So this page must be available at this + * point, unless the page refcount overflowed: + */ + if ((pgmap->flags & PGMAP_COMPOUND) && + WARN_ON_ONCE(!try_grab_compound_head(pages[i-1], + nr_pages_hpage, + flags))) { + put_dev_pagemap(pgmap); + spin_unlock(ptl); + remainder = 0; + err = -ENOMEM; + break; + } + put_dev_pagemap(pgmap); + } + spin_unlock(ptl); + } + *nr_pages = remainder; + /* + * setting position is actually required only if remainder is + * not zero but it's faster not to add a "if (remainder)" + * branch. + */ + *position = vaddr; + + return i ? i : err; +} + int copy_huge_pud(struct mm_struct *dst_mm, struct mm_struct *src_mm, pud_t *dst_pud, pud_t *src_pud, unsigned long addr, struct vm_area_struct *vma)
Similar to follow_hugetlb_page() add a follow_devmap_page which rather than calling follow_page() per 4K page in a PMD/PUD it does so for the entire PMD, where we lock the pmd/pud, get all pages , unlock. While doing so, we only change the refcount once when PGMAP_COMPOUND is passed in. This let us improve {pin,get}_user_pages{,_longterm}() considerably: $ gup_benchmark -f /dev/dax0.2 -m 16384 -r 10 -S [-U,-b,-L] -n 512 -w (<test>) [before] -> [after] (get_user_pages 2M pages) ~150k us -> ~8.9k us (pin_user_pages 2M pages) ~192k us -> ~9k us (pin_user_pages_longterm 2M pages) ~200k us -> ~19k us Signed-off-by: Joao Martins <joao.m.martins@oracle.com> --- I've special-cased this to device-dax vmas given its similar page size guarantees as hugetlbfs, but I feel this is a bit wrong. I am replicating follow_hugetlb_page() as RFC ought to seek feedback whether this should be generalized if no fundamental issues exist. In such case, should I be changing follow_page_mask() to take either an array of pages or a function pointer and opaque arguments which would let caller pick its structure? --- include/linux/huge_mm.h | 4 + include/linux/mm.h | 2 + mm/gup.c | 22 ++++- mm/huge_memory.c | 202 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 227 insertions(+), 3 deletions(-)