Re: [PATCH mm-unstable v2 06/16] mm: csky: Introduce arch_mmap_hint()

["Liam R. Howlett" <Liam.Howlett@oracle.com>]

* Kalesh Singh <kaleshsingh@google.com> [241211 18:28]:
> Introduce csky arch_mmap_hint() and define HAVE_ARCH_MMAP_HINT.
> This is a preparatory patch, no functional change is introduced.

This also looks like it has changed the validation order and potentially
introduced functional changes?

All these stem from the same cloned code (sparc32 iirc), but were not
updated when the cloned code was updated.  This is why I am against
arch_* code.  We should find a better way to unify the code so that
there is nothing different.  You seem to have gotten some of the shared
code together, but some still exists.

In the addresses, there are upper and lower limits, and sometimes
"colours".  Could we not just define the upper/lower limits in each arch
and if colour is used?  Maybe this is complicated with 32/64 handled
both in the 64 bit code.

Is there any plan to unite this code further?

We have had errors for many years in cloned but not updated code.  I
really wish there was more information in the cover letter on what is
going on here.  I'd like to try and reduce the arch_ code to, basically
nothing.

I was also disappointed that I wasn't Cc'ed because I've spent a lot of
time in this code and this area.  I am probably the last one to crawl
through and change any of this.


> 
> Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
> ---
> 
> Changes in v2:
>   - MAP_FIXED case is also handled in arch_mmap_hint() since this is just a
>     special case of the hint addr being "enforced", per Yang Shi.
>   - Consolidate error handling in arch_mmap_hint().
> 
>  arch/csky/abiv1/inc/abi/pgtable-bits.h |  1 +
>  arch/csky/abiv1/mmap.c                 | 68 ++++++++++++++------------
>  2 files changed, 38 insertions(+), 31 deletions(-)
> 
> diff --git a/arch/csky/abiv1/inc/abi/pgtable-bits.h b/arch/csky/abiv1/inc/abi/pgtable-bits.h
> index ae7a2f76dd42..c346a9fcb522 100644
> --- a/arch/csky/abiv1/inc/abi/pgtable-bits.h
> +++ b/arch/csky/abiv1/inc/abi/pgtable-bits.h
> @@ -51,5 +51,6 @@
>  					((offset) << 10)})
>  
>  #define HAVE_ARCH_UNMAPPED_AREA
> +#define HAVE_ARCH_MMAP_HINT
>  
>  #endif /* __ASM_CSKY_PGTABLE_BITS_H */
> diff --git a/arch/csky/abiv1/mmap.c b/arch/csky/abiv1/mmap.c
> index 1047865e82a9..0c5c51a081e4 100644
> --- a/arch/csky/abiv1/mmap.c
> +++ b/arch/csky/abiv1/mmap.c
> @@ -13,6 +13,39 @@
>  	((((addr)+SHMLBA-1)&~(SHMLBA-1)) +	\
>  	 (((pgoff)<<PAGE_SHIFT) & (SHMLBA-1)))
>  
> +unsigned long arch_mmap_hint(struct file *filp, unsigned long addr,
> +			     unsigned long len, unsigned long pgoff,
> +			     unsigned long flags)
> +{
> +	bool do_align;
> +
> +	if (len > TASK_SIZE)
> +		return -ENOMEM;
> +
> +	/*
> +	 * We only need to do colour alignment if either the I or D
> +	 * caches alias.
> +	 */
> +	do_align = filp || (flags & MAP_SHARED);
> +
> +	/*
> +	 * We enforce the MAP_FIXED case.
> +	 */
> +	if (flags & MAP_FIXED) {
> +		if (flags & MAP_SHARED &&
> +		    (addr - (pgoff << PAGE_SHIFT)) & (SHMLBA - 1))
> +			return -EINVAL;
> +		return addr;
> +	}
> +
> +	if (do_align)
> +		addr = COLOUR_ALIGN(addr, pgoff);
> +	else
> +		addr = PAGE_ALIGN(addr);
> +
> +	return generic_mmap_hint(filp, addr, len, pgoff, flags);
> +}
> +
>  /*
>   * We need to ensure that shared mappings are correctly aligned to
>   * avoid aliasing issues with VIPT caches.  We need to ensure that
> @@ -27,8 +60,7 @@ arch_get_unmapped_area(struct file *filp, unsigned long addr,
>  		unsigned long flags, vm_flags_t vm_flags)
>  {
>  	struct mm_struct *mm = current->mm;
> -	struct vm_area_struct *vma;
> -	int do_align = 0;
> +	bool do_align;
>  	struct vm_unmapped_area_info info = {
>  		.length = len,
>  		.low_limit = mm->mmap_base,
> @@ -36,37 +68,11 @@ arch_get_unmapped_area(struct file *filp, unsigned long addr,
>  		.align_offset = pgoff << PAGE_SHIFT
>  	};
>  
> -	/*
> -	 * We only need to do colour alignment if either the I or D
> -	 * caches alias.
> -	 */

This seems like useful information to keep around?

> -	do_align = filp || (flags & MAP_SHARED);
> -
> -	/*
> -	 * We enforce the MAP_FIXED case.
> -	 */
> -	if (flags & MAP_FIXED) {
> -		if (flags & MAP_SHARED &&
> -		    (addr - (pgoff << PAGE_SHIFT)) & (SHMLBA - 1))
> -			return -EINVAL;
> +	addr = arch_mmap_hint(filp, addr, len, pgoff, flags);
> +	if (addr)
>  		return addr;
> -	}
> -
> -	if (len > TASK_SIZE)
> -		return -ENOMEM;
> -
> -	if (addr) {
> -		if (do_align)
> -			addr = COLOUR_ALIGN(addr, pgoff);
> -		else
> -			addr = PAGE_ALIGN(addr);
> -
> -		vma = find_vma(mm, addr);
> -		if (TASK_SIZE - len >= addr &&
> -		    (!vma || addr + len <= vm_start_gap(vma)))
> -			return addr;
> -	}
>  
> +	do_align = filp || (flags & MAP_SHARED);
>  	info.align_mask = do_align ? (PAGE_MASK & (SHMLBA - 1)) : 0;
>  	return vm_unmapped_area(&info);
>  }
> -- 
> 2.47.0.338.g60cca15819-goog
> 
>