| Message ID | f292a83707b64c73fcb02a8708f18f09422f7eea.1638976229.git.christophe.leroy@csgroup.eu (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Convert powerpc to default topdown mmap layout | expand |
Excerpts from Christophe Leroy's message of December 9, 2021 3:18 am: > Use the generic version of arch_hugetlb_get_unmapped_area() > which is now available at all time. > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > arch/powerpc/include/asm/book3s/64/hugetlb.h | 4 -- > arch/powerpc/mm/book3s64/radix_hugetlbpage.c | 55 -------------------- > arch/powerpc/mm/hugetlbpage.c | 4 +- > 3 files changed, 1 insertion(+), 62 deletions(-) > > diff --git a/arch/powerpc/include/asm/book3s/64/hugetlb.h b/arch/powerpc/include/asm/book3s/64/hugetlb.h > index 12e150e615b7..b37a28f62cf6 100644 > --- a/arch/powerpc/include/asm/book3s/64/hugetlb.h > +++ b/arch/powerpc/include/asm/book3s/64/hugetlb.h > @@ -8,10 +8,6 @@ > */ > void radix__flush_hugetlb_page(struct vm_area_struct *vma, unsigned long vmaddr); > void radix__local_flush_hugetlb_page(struct vm_area_struct *vma, unsigned long vmaddr); > -extern unsigned long > -radix__hugetlb_get_unmapped_area(struct file *file, unsigned long addr, > - unsigned long len, unsigned long pgoff, > - unsigned long flags); > > extern void radix__huge_ptep_modify_prot_commit(struct vm_area_struct *vma, > unsigned long addr, pte_t *ptep, > diff --git a/arch/powerpc/mm/book3s64/radix_hugetlbpage.c b/arch/powerpc/mm/book3s64/radix_hugetlbpage.c > index 23d3e08911d3..d2fb776febb4 100644 > --- a/arch/powerpc/mm/book3s64/radix_hugetlbpage.c > +++ b/arch/powerpc/mm/book3s64/radix_hugetlbpage.c > @@ -41,61 +41,6 @@ void radix__flush_hugetlb_tlb_range(struct vm_area_struct *vma, unsigned long st > radix__flush_tlb_range_psize(vma->vm_mm, start, end, psize); > } > > -/* > - * A vairant of hugetlb_get_unmapped_area doing topdown search > - * FIXME!! should we do as x86 does or non hugetlb area does ? > - * ie, use topdown or not based on mmap_is_legacy check ? > - */ > -unsigned long > -radix__hugetlb_get_unmapped_area(struct file *file, unsigned long addr, > - unsigned long len, unsigned long pgoff, > - unsigned long flags) > -{ > - struct mm_struct *mm = current->mm; > - struct vm_area_struct *vma; > - struct hstate *h = hstate_file(file); > - int fixed = (flags & MAP_FIXED); > - unsigned long high_limit; > - struct vm_unmapped_area_info info; > - > - high_limit = DEFAULT_MAP_WINDOW; > - if (addr >= high_limit || (fixed && (addr + len > high_limit))) > - high_limit = TASK_SIZE; I wonder if generic hugetlb_get_unmapped_area needs to have the arch_get_mmap_end() added. arm64 has arch_get_mmap_end() and !HAVE_ARCH_HUGETLB_UNMAPPED_AREA so it looks like it has broken large address hint logic for hugetlbfs mappings? x86-64 defines their own and does the same hinting for normal and hugetlbfs mmap. If we had that and defied arch_get_mmap_end(), then this patch should work. Thanks, Nick
Le 09/12/2021 à 11:02, Nicholas Piggin a écrit : > Excerpts from Christophe Leroy's message of December 9, 2021 3:18 am: >> Use the generic version of arch_hugetlb_get_unmapped_area() >> which is now available at all time. >> >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >> --- >> arch/powerpc/include/asm/book3s/64/hugetlb.h | 4 -- >> arch/powerpc/mm/book3s64/radix_hugetlbpage.c | 55 -------------------- >> arch/powerpc/mm/hugetlbpage.c | 4 +- >> 3 files changed, 1 insertion(+), 62 deletions(-) >> >> diff --git a/arch/powerpc/include/asm/book3s/64/hugetlb.h b/arch/powerpc/include/asm/book3s/64/hugetlb.h >> index 12e150e615b7..b37a28f62cf6 100644 >> --- a/arch/powerpc/include/asm/book3s/64/hugetlb.h >> +++ b/arch/powerpc/include/asm/book3s/64/hugetlb.h >> @@ -8,10 +8,6 @@ >> */ >> void radix__flush_hugetlb_page(struct vm_area_struct *vma, unsigned long vmaddr); >> void radix__local_flush_hugetlb_page(struct vm_area_struct *vma, unsigned long vmaddr); >> -extern unsigned long >> -radix__hugetlb_get_unmapped_area(struct file *file, unsigned long addr, >> - unsigned long len, unsigned long pgoff, >> - unsigned long flags); >> >> extern void radix__huge_ptep_modify_prot_commit(struct vm_area_struct *vma, >> unsigned long addr, pte_t *ptep, >> diff --git a/arch/powerpc/mm/book3s64/radix_hugetlbpage.c b/arch/powerpc/mm/book3s64/radix_hugetlbpage.c >> index 23d3e08911d3..d2fb776febb4 100644 >> --- a/arch/powerpc/mm/book3s64/radix_hugetlbpage.c >> +++ b/arch/powerpc/mm/book3s64/radix_hugetlbpage.c >> @@ -41,61 +41,6 @@ void radix__flush_hugetlb_tlb_range(struct vm_area_struct *vma, unsigned long st >> radix__flush_tlb_range_psize(vma->vm_mm, start, end, psize); >> } >> >> -/* >> - * A vairant of hugetlb_get_unmapped_area doing topdown search >> - * FIXME!! should we do as x86 does or non hugetlb area does ? >> - * ie, use topdown or not based on mmap_is_legacy check ? >> - */ >> -unsigned long >> -radix__hugetlb_get_unmapped_area(struct file *file, unsigned long addr, >> - unsigned long len, unsigned long pgoff, >> - unsigned long flags) >> -{ >> - struct mm_struct *mm = current->mm; >> - struct vm_area_struct *vma; >> - struct hstate *h = hstate_file(file); >> - int fixed = (flags & MAP_FIXED); >> - unsigned long high_limit; >> - struct vm_unmapped_area_info info; >> - >> - high_limit = DEFAULT_MAP_WINDOW; >> - if (addr >= high_limit || (fixed && (addr + len > high_limit))) >> - high_limit = TASK_SIZE; > > I wonder if generic hugetlb_get_unmapped_area needs to have the > arch_get_mmap_end() added. > > arm64 has arch_get_mmap_end() and !HAVE_ARCH_HUGETLB_UNMAPPED_AREA so > it looks like it has broken large address hint logic for hugetlbfs > mappings? x86-64 defines their own and does the same hinting for > normal and hugetlbfs mmap. > > If we had that and defied arch_get_mmap_end(), then this patch should > work. > As far as I can see, hugetlb_get_unmapped_area() variants used to be very similar to get_unmapped_area() until commit 1be7107fbe18 ("mm: larger stack guard gap, between vmas") and commit f6795053dac8 ("mm: mmap: Allow for "high" userspace addresses") I see no reason why those changes couldn't apply to hugetlb_get_unmapped_area() as well. Need to know what ARM64 think about it thought. Will, Catalin, any opinion ? Christophe
On Thu, Dec 16, 2021 at 05:13:47PM +0000, Christophe Leroy wrote: > Le 09/12/2021 à 11:02, Nicholas Piggin a écrit : > > Excerpts from Christophe Leroy's message of December 9, 2021 3:18 am: > >> Use the generic version of arch_hugetlb_get_unmapped_area() > >> which is now available at all time. > >> > >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > >> --- > >> arch/powerpc/include/asm/book3s/64/hugetlb.h | 4 -- > >> arch/powerpc/mm/book3s64/radix_hugetlbpage.c | 55 -------------------- > >> arch/powerpc/mm/hugetlbpage.c | 4 +- > >> 3 files changed, 1 insertion(+), 62 deletions(-) > >> > >> diff --git a/arch/powerpc/include/asm/book3s/64/hugetlb.h b/arch/powerpc/include/asm/book3s/64/hugetlb.h > >> index 12e150e615b7..b37a28f62cf6 100644 > >> --- a/arch/powerpc/include/asm/book3s/64/hugetlb.h > >> +++ b/arch/powerpc/include/asm/book3s/64/hugetlb.h > >> @@ -8,10 +8,6 @@ > >> */ > >> void radix__flush_hugetlb_page(struct vm_area_struct *vma, unsigned long vmaddr); > >> void radix__local_flush_hugetlb_page(struct vm_area_struct *vma, unsigned long vmaddr); > >> -extern unsigned long > >> -radix__hugetlb_get_unmapped_area(struct file *file, unsigned long addr, > >> - unsigned long len, unsigned long pgoff, > >> - unsigned long flags); > >> > >> extern void radix__huge_ptep_modify_prot_commit(struct vm_area_struct *vma, > >> unsigned long addr, pte_t *ptep, > >> diff --git a/arch/powerpc/mm/book3s64/radix_hugetlbpage.c b/arch/powerpc/mm/book3s64/radix_hugetlbpage.c > >> index 23d3e08911d3..d2fb776febb4 100644 > >> --- a/arch/powerpc/mm/book3s64/radix_hugetlbpage.c > >> +++ b/arch/powerpc/mm/book3s64/radix_hugetlbpage.c > >> @@ -41,61 +41,6 @@ void radix__flush_hugetlb_tlb_range(struct vm_area_struct *vma, unsigned long st > >> radix__flush_tlb_range_psize(vma->vm_mm, start, end, psize); > >> } > >> > >> -/* > >> - * A vairant of hugetlb_get_unmapped_area doing topdown search > >> - * FIXME!! should we do as x86 does or non hugetlb area does ? > >> - * ie, use topdown or not based on mmap_is_legacy check ? > >> - */ > >> -unsigned long > >> -radix__hugetlb_get_unmapped_area(struct file *file, unsigned long addr, > >> - unsigned long len, unsigned long pgoff, > >> - unsigned long flags) > >> -{ > >> - struct mm_struct *mm = current->mm; > >> - struct vm_area_struct *vma; > >> - struct hstate *h = hstate_file(file); > >> - int fixed = (flags & MAP_FIXED); > >> - unsigned long high_limit; > >> - struct vm_unmapped_area_info info; > >> - > >> - high_limit = DEFAULT_MAP_WINDOW; > >> - if (addr >= high_limit || (fixed && (addr + len > high_limit))) > >> - high_limit = TASK_SIZE; > > > > I wonder if generic hugetlb_get_unmapped_area needs to have the > > arch_get_mmap_end() added. > > > > arm64 has arch_get_mmap_end() and !HAVE_ARCH_HUGETLB_UNMAPPED_AREA so > > it looks like it has broken large address hint logic for hugetlbfs > > mappings? x86-64 defines their own and does the same hinting for > > normal and hugetlbfs mmap. > > > > If we had that and defied arch_get_mmap_end(), then this patch should > > work. > > > > As far as I can see, hugetlb_get_unmapped_area() variants used to be > very similar to get_unmapped_area() until commit 1be7107fbe18 ("mm: > larger stack guard gap, between vmas") and commit f6795053dac8 ("mm: > mmap: Allow for "high" userspace addresses") > > I see no reason why those changes couldn't apply to > hugetlb_get_unmapped_area() as well. > > Need to know what ARM64 think about it thought. Will, Catalin, any opinion ? I think we should have fixed hugetlb_get_unmapped_area() as well when we added support for 52-bit VA. The reason for commit f6795053dac8 was to prevent normal mmap() from returning addresses above 48-bit by default as some user-space had hard assumptions about this. It's a slight ABI change if you do this for hugetlb_get_unmapped_area() but I doubt anyone would notice. It's more likely that the current behaviour would cause issues, so I'd rather have them consistent.
diff --git a/arch/powerpc/include/asm/book3s/64/hugetlb.h b/arch/powerpc/include/asm/book3s/64/hugetlb.h index 12e150e615b7..b37a28f62cf6 100644 --- a/arch/powerpc/include/asm/book3s/64/hugetlb.h +++ b/arch/powerpc/include/asm/book3s/64/hugetlb.h @@ -8,10 +8,6 @@ */ void radix__flush_hugetlb_page(struct vm_area_struct *vma, unsigned long vmaddr); void radix__local_flush_hugetlb_page(struct vm_area_struct *vma, unsigned long vmaddr); -extern unsigned long -radix__hugetlb_get_unmapped_area(struct file *file, unsigned long addr, - unsigned long len, unsigned long pgoff, - unsigned long flags); extern void radix__huge_ptep_modify_prot_commit(struct vm_area_struct *vma, unsigned long addr, pte_t *ptep, diff --git a/arch/powerpc/mm/book3s64/radix_hugetlbpage.c b/arch/powerpc/mm/book3s64/radix_hugetlbpage.c index 23d3e08911d3..d2fb776febb4 100644 --- a/arch/powerpc/mm/book3s64/radix_hugetlbpage.c +++ b/arch/powerpc/mm/book3s64/radix_hugetlbpage.c @@ -41,61 +41,6 @@ void radix__flush_hugetlb_tlb_range(struct vm_area_struct *vma, unsigned long st radix__flush_tlb_range_psize(vma->vm_mm, start, end, psize); } -/* - * A vairant of hugetlb_get_unmapped_area doing topdown search - * FIXME!! should we do as x86 does or non hugetlb area does ? - * ie, use topdown or not based on mmap_is_legacy check ? - */ -unsigned long -radix__hugetlb_get_unmapped_area(struct file *file, unsigned long addr, - unsigned long len, unsigned long pgoff, - unsigned long flags) -{ - struct mm_struct *mm = current->mm; - struct vm_area_struct *vma; - struct hstate *h = hstate_file(file); - int fixed = (flags & MAP_FIXED); - unsigned long high_limit; - struct vm_unmapped_area_info info; - - high_limit = DEFAULT_MAP_WINDOW; - if (addr >= high_limit || (fixed && (addr + len > high_limit))) - high_limit = TASK_SIZE; - - if (len & ~huge_page_mask(h)) - return -EINVAL; - if (len > high_limit) - return -ENOMEM; - - if (fixed) { - if (addr > high_limit - len) - return -ENOMEM; - if (prepare_hugepage_range(file, addr, len)) - return -EINVAL; - return addr; - } - - if (addr) { - addr = ALIGN(addr, huge_page_size(h)); - vma = find_vma(mm, addr); - if (high_limit - len >= addr && addr >= mmap_min_addr && - (!vma || addr + len <= vm_start_gap(vma))) - return addr; - } - /* - * We are always doing an topdown search here. Slice code - * does that too. - */ - info.flags = VM_UNMAPPED_AREA_TOPDOWN; - info.length = len; - info.low_limit = max(PAGE_SIZE, mmap_min_addr); - info.high_limit = mm->mmap_base + (high_limit - DEFAULT_MAP_WINDOW); - info.align_mask = PAGE_MASK & ~huge_page_mask(h); - info.align_offset = 0; - - return vm_unmapped_area(&info); -} - void radix__huge_ptep_modify_prot_commit(struct vm_area_struct *vma, unsigned long addr, pte_t *ptep, pte_t old_pte, pte_t pte) diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c index f18b3a1d18f0..bfd7f4af1e58 100644 --- a/arch/powerpc/mm/hugetlbpage.c +++ b/arch/powerpc/mm/hugetlbpage.c @@ -553,11 +553,9 @@ unsigned long hugetlb_get_unmapped_area(struct file *file, unsigned long addr, unsigned long len, unsigned long pgoff, unsigned long flags) { -#ifdef CONFIG_PPC_RADIX_MMU if (radix_enabled()) - return radix__hugetlb_get_unmapped_area(file, addr, len, + return generic_hugetlb_get_unmapped_area(file, addr, len, pgoff, flags); -#endif #ifdef CONFIG_PPC_64S_HASH_MMU return slice_get_unmapped_area(addr, len, flags, file_to_psize(file), 1); #endif
Use the generic version of arch_hugetlb_get_unmapped_area() which is now available at all time. Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> --- arch/powerpc/include/asm/book3s/64/hugetlb.h | 4 -- arch/powerpc/mm/book3s64/radix_hugetlbpage.c | 55 -------------------- arch/powerpc/mm/hugetlbpage.c | 4 +- 3 files changed, 1 insertion(+), 62 deletions(-)