Re: [PATCH v4 06/10] powerpc/mm: Use generic_get_unmapped_area() and call it from arch_get_unmapped_area()
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
>> ---
>> 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
Re: [PATCH v4 06/10] powerpc/mm: Use generic_get_unmapped_area() and call it from arch_get_unmapped_area()
Christophe Leroy 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
> ---
> 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
Re: [PATCH v4 06/10] powerpc/mm: Use generic_get_unmapped_area() and call it from arch_get_unmapped_area()
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
>> ---
>> 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
>
Re: [PATCH v4 06/10] powerpc/mm: Use generic_get_unmapped_area() and call it from arch_get_unmapped_area()
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
> ---
> 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
[PATCH v4 06/10] powerpc/mm: Use generic_get_unmapped_area() and call it from arch_get_unmapped_area()
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
---
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;
-
- 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();
-}
-
-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();
- 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(>mm->context), 0);
@@ -204,6 +104,9 @@ unsigned long arch_get_unmapped_area_topdown(struct file
*filp,
const
