| Message ID | 8f54a8d097c402d808147b2044365ebfda2862dd.1638976229.git.christophe.leroy@csgroup.eu (mailing list archive) |
|---|---|
| State | New |
| 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_get_unmapped_area() which > is now available at all time instead of its copy > radix__arch_get_unmapped_area() > > Instead of setting mm->get_unmapped_area() to either > arch_get_unmapped_area() or generic_get_unmapped_area(), > always set it to arch_get_unmapped_area() and call > generic_get_unmapped_area() from there when radix is enabled. > > Do the same with radix__arch_get_unmapped_area_topdown() > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > arch/powerpc/mm/mmap.c | 127 ++--------------------------------------- > 1 file changed, 6 insertions(+), 121 deletions(-) > > diff --git a/arch/powerpc/mm/mmap.c b/arch/powerpc/mm/mmap.c > index 9b0d6e395bc0..46781d0103d1 100644 > --- a/arch/powerpc/mm/mmap.c > +++ b/arch/powerpc/mm/mmap.c > @@ -81,115 +81,15 @@ static inline unsigned long mmap_base(unsigned long rnd, > } > > #ifdef HAVE_ARCH_UNMAPPED_AREA > -#ifdef CONFIG_PPC_RADIX_MMU > -/* > - * Same function as generic code used only for radix, because we don't need to overload > - * the generic one. But we will have to duplicate, because hash select > - * HAVE_ARCH_UNMAPPED_AREA > - */ > -static unsigned long > -radix__arch_get_unmapped_area(struct file *filp, unsigned long addr, > - unsigned long len, unsigned long pgoff, > - unsigned long flags) > -{ > - struct mm_struct *mm = current->mm; > - struct vm_area_struct *vma; > - 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; Does 64s radix need to define arch_get_mmap_end() to do the above now? Otherwise great to consolidate this with core code, nice patch. Thanks, Nick
Le 09/12/2021 à 10:50, Nicholas Piggin a écrit : > Excerpts from Christophe Leroy's message of December 9, 2021 3:18 am: >> Use the generic version of arch_get_unmapped_area() which >> is now available at all time instead of its copy >> radix__arch_get_unmapped_area() >> >> Instead of setting mm->get_unmapped_area() to either >> arch_get_unmapped_area() or generic_get_unmapped_area(), >> always set it to arch_get_unmapped_area() and call >> generic_get_unmapped_area() from there when radix is enabled. >> >> Do the same with radix__arch_get_unmapped_area_topdown() >> >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >> --- >> arch/powerpc/mm/mmap.c | 127 ++--------------------------------------- >> 1 file changed, 6 insertions(+), 121 deletions(-) >> >> diff --git a/arch/powerpc/mm/mmap.c b/arch/powerpc/mm/mmap.c >> index 9b0d6e395bc0..46781d0103d1 100644 >> --- a/arch/powerpc/mm/mmap.c >> +++ b/arch/powerpc/mm/mmap.c >> @@ -81,115 +81,15 @@ static inline unsigned long mmap_base(unsigned long rnd, >> } >> >> #ifdef HAVE_ARCH_UNMAPPED_AREA >> -#ifdef CONFIG_PPC_RADIX_MMU >> -/* >> - * Same function as generic code used only for radix, because we don't need to overload >> - * the generic one. But we will have to duplicate, because hash select >> - * HAVE_ARCH_UNMAPPED_AREA >> - */ >> -static unsigned long >> -radix__arch_get_unmapped_area(struct file *filp, unsigned long addr, >> - unsigned long len, unsigned long pgoff, >> - unsigned long flags) >> -{ >> - struct mm_struct *mm = current->mm; >> - struct vm_area_struct *vma; >> - 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; > > Does 64s radix need to define arch_get_mmap_end() to do the above now? Sure, good point. Seems like I got hypnotised by the comment "- * Same function as generic code used only for radix", I didn't catch that little difference. > > Otherwise great to consolidate this with core code, nice patch. > > Thanks, > Nick >
Christophe Leroy <christophe.leroy@csgroup.eu> writes: > Use the generic version of arch_get_unmapped_area() which > is now available at all time instead of its copy > radix__arch_get_unmapped_area() > > Instead of setting mm->get_unmapped_area() to either > arch_get_unmapped_area() or generic_get_unmapped_area(), > always set it to arch_get_unmapped_area() and call > generic_get_unmapped_area() from there when radix is enabled. > > Do the same with radix__arch_get_unmapped_area_topdown() > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > arch/powerpc/mm/mmap.c | 127 ++--------------------------------------- > 1 file changed, 6 insertions(+), 121 deletions(-) > > diff --git a/arch/powerpc/mm/mmap.c b/arch/powerpc/mm/mmap.c > index 9b0d6e395bc0..46781d0103d1 100644 > --- a/arch/powerpc/mm/mmap.c > +++ b/arch/powerpc/mm/mmap.c > @@ -81,115 +81,15 @@ static inline unsigned long mmap_base(unsigned long rnd, > } > > #ifdef HAVE_ARCH_UNMAPPED_AREA > -#ifdef CONFIG_PPC_RADIX_MMU > -/* > - * Same function as generic code used only for radix, because we don't need to overload > - * the generic one. But we will have to duplicate, because hash select > - * HAVE_ARCH_UNMAPPED_AREA > - */ > -static unsigned long > -radix__arch_get_unmapped_area(struct file *filp, unsigned long addr, > - unsigned long len, unsigned long pgoff, > - unsigned long flags) > -{ > - struct mm_struct *mm = current->mm; > - struct vm_area_struct *vma; > - 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 > high_limit) > - return -ENOMEM; There are some differences in the above vs the generic code, the generic arch_get_unmapped_area_topdown() in mm/mmap.c does: const unsigned long mmap_end = arch_get_mmap_end(addr); if (len > mmap_end - mmap_min_addr) return -ENOMEM; if (flags & MAP_FIXED) return addr; Our current code adjusts high_limit for fixed mappings that span above the default map window. We added that logic in: 35602f82d0c7 ("powerpc/64s/hash: Allow MAP_FIXED allocations to cross 128TB boundary") That means a fixed mapping that crosses the 128T boundary will be allowed by our code. On the other hand the generic code will allow a fixed mapping to cross the 128T boundary, but only if the size of the mapping is < ~128T. (The actual size limit is (128T - mmap_min_addr), which is usually 4K or 64K, but is adjustable.) It's unlikely that any apps are doing fixed mappings larger than 128T that cross the 128T boundary, but I think we need to allow it. 128T seems like a lot, but is not compared to the entire 4PB address space. So I think we need to fix that in the generic code. The easiest option is probably to pass flags to arch_get_mmap_end(), and then the arches can decide whether to adjust the return value based on flags. Then there's also the extra check we have here: > - if (fixed) { > - if (addr > high_limit - len) > - return -ENOMEM; > - return addr; > - } I think we can drop that when converting to the generic version, the only case in which it matters is when high_limit == TASK_SIZE, and get_unmapped_area() already does that check after calling us: if (addr > TASK_SIZE - len) return -ENOMEM; cheers
Le 09/12/2021 à 10:50, Nicholas Piggin a écrit : > Excerpts from Christophe Leroy's message of December 9, 2021 3:18 am: >> Use the generic version of arch_get_unmapped_area() which >> is now available at all time instead of its copy >> radix__arch_get_unmapped_area() >> >> Instead of setting mm->get_unmapped_area() to either >> arch_get_unmapped_area() or generic_get_unmapped_area(), >> always set it to arch_get_unmapped_area() and call >> generic_get_unmapped_area() from there when radix is enabled. >> >> Do the same with radix__arch_get_unmapped_area_topdown() >> >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >> --- >> arch/powerpc/mm/mmap.c | 127 ++--------------------------------------- >> 1 file changed, 6 insertions(+), 121 deletions(-) >> >> diff --git a/arch/powerpc/mm/mmap.c b/arch/powerpc/mm/mmap.c >> index 9b0d6e395bc0..46781d0103d1 100644 >> --- a/arch/powerpc/mm/mmap.c >> +++ b/arch/powerpc/mm/mmap.c >> @@ -81,115 +81,15 @@ static inline unsigned long mmap_base(unsigned long rnd, >> } >> >> #ifdef HAVE_ARCH_UNMAPPED_AREA >> -#ifdef CONFIG_PPC_RADIX_MMU >> -/* >> - * Same function as generic code used only for radix, because we don't need to overload >> - * the generic one. But we will have to duplicate, because hash select >> - * HAVE_ARCH_UNMAPPED_AREA >> - */ >> -static unsigned long >> -radix__arch_get_unmapped_area(struct file *filp, unsigned long addr, >> - unsigned long len, unsigned long pgoff, >> - unsigned long flags) >> -{ >> - struct mm_struct *mm = current->mm; >> - struct vm_area_struct *vma; >> - 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; > > Does 64s radix need to define arch_get_mmap_end() to do the above now? > > Otherwise great to consolidate this with core code, nice patch. > Yes it needs arch_get_mmap_end() and also arch_get_mmap_base(). I added it in v5, taking also suggestion from Michael. Thanks Christophe
diff --git a/arch/powerpc/mm/mmap.c b/arch/powerpc/mm/mmap.c index 9b0d6e395bc0..46781d0103d1 100644 --- a/arch/powerpc/mm/mmap.c +++ b/arch/powerpc/mm/mmap.c @@ -81,115 +81,15 @@ static inline unsigned long mmap_base(unsigned long rnd, } #ifdef HAVE_ARCH_UNMAPPED_AREA -#ifdef CONFIG_PPC_RADIX_MMU -/* - * Same function as generic code used only for radix, because we don't need to overload - * the generic one. But we will have to duplicate, because hash select - * HAVE_ARCH_UNMAPPED_AREA - */ -static unsigned long -radix__arch_get_unmapped_area(struct file *filp, unsigned long addr, - unsigned long len, unsigned long pgoff, - unsigned long flags) -{ - struct mm_struct *mm = current->mm; - struct vm_area_struct *vma; - 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 > high_limit) - return -ENOMEM; - - if (fixed) { - if (addr > high_limit - len) - return -ENOMEM; - return addr; - } - - if (addr) { - addr = PAGE_ALIGN(addr); - vma = find_vma(mm, addr); - if (high_limit - len >= addr && addr >= mmap_min_addr && - (!vma || addr + len <= vm_start_gap(vma))) - return addr; - } - - info.flags = 0; - info.length = len; - info.low_limit = mm->mmap_base; - info.high_limit = high_limit; - info.align_mask = 0; - - return vm_unmapped_area(&info); -} - -static unsigned long -radix__arch_get_unmapped_area_topdown(struct file *filp, - const unsigned long addr0, - const unsigned long len, - const unsigned long pgoff, - const unsigned long flags) -{ - struct vm_area_struct *vma; - struct mm_struct *mm = current->mm; - unsigned long addr = addr0; - 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 > high_limit) - return -ENOMEM; - - if (fixed) { - if (addr > high_limit - len) - return -ENOMEM; - return addr; - } - - if (addr) { - addr = PAGE_ALIGN(addr); - vma = find_vma(mm, addr); - if (high_limit - len >= addr && addr >= mmap_min_addr && - (!vma || addr + len <= vm_start_gap(vma))) - return addr; - } - - 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 = 0; - - addr = vm_unmapped_area(&info); - if (!(addr & ~PAGE_MASK)) - return addr; - VM_BUG_ON(addr != -ENOMEM); - - /* - * A failed mmap() very likely causes application failure, - * so fall back to the bottom-up function here. This scenario - * can happen with large stack limits and large mmap() - * allocations. - */ - return radix__arch_get_unmapped_area(filp, addr0, len, pgoff, flags); -} -#endif - unsigned long arch_get_unmapped_area(struct file *filp, unsigned long addr, unsigned long len, unsigned long pgoff, unsigned long flags) { + if (radix_enabled()) + return generic_get_unmapped_area(filp, addr, len, pgoff, flags); + #ifdef CONFIG_PPC_64S_HASH_MMU return slice_get_unmapped_area(addr, len, flags, mm_ctx_user_psize(¤t->mm->context), 0); @@ -204,6 +104,9 @@ unsigned long arch_get_unmapped_area_topdown(struct file *filp, const unsigned long pgoff, const unsigned long flags) { + if (radix_enabled()) + return generic_get_unmapped_area_topdown(filp, addr0, len, pgoff, flags); + #ifdef CONFIG_PPC_64S_HASH_MMU return slice_get_unmapped_area(addr0, len, flags, mm_ctx_user_psize(¤t->mm->context), 1); @@ -213,21 +116,6 @@ unsigned long arch_get_unmapped_area_topdown(struct file *filp, } #endif /* HAVE_ARCH_UNMAPPED_AREA */ -static void radix__arch_pick_mmap_layout(struct mm_struct *mm, - unsigned long random_factor, - struct rlimit *rlim_stack) -{ -#ifdef CONFIG_PPC_RADIX_MMU - if (mmap_is_legacy(rlim_stack)) { - mm->mmap_base = TASK_UNMAPPED_BASE; - mm->get_unmapped_area = radix__arch_get_unmapped_area; - } else { - mm->mmap_base = mmap_base(random_factor, rlim_stack); - mm->get_unmapped_area = radix__arch_get_unmapped_area_topdown; - } -#endif -} - /* * This function, called very early during the creation of a new * process VM image, sets up which VM layout function to use: @@ -239,9 +127,6 @@ void arch_pick_mmap_layout(struct mm_struct *mm, struct rlimit *rlim_stack) if (current->flags & PF_RANDOMIZE) random_factor = arch_mmap_rnd(); - if (radix_enabled()) - return radix__arch_pick_mmap_layout(mm, random_factor, - rlim_stack); /* * Fall back to the standard layout if the personality * bit is set, or if the expected stack growth is unlimited:
Use the generic version of arch_get_unmapped_area() which is now available at all time instead of its copy radix__arch_get_unmapped_area() Instead of setting mm->get_unmapped_area() to either arch_get_unmapped_area() or generic_get_unmapped_area(), always set it to arch_get_unmapped_area() and call generic_get_unmapped_area() from there when radix is enabled. Do the same with radix__arch_get_unmapped_area_topdown() Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> --- arch/powerpc/mm/mmap.c | 127 ++--------------------------------------- 1 file changed, 6 insertions(+), 121 deletions(-)