patch to support topdown mmap allocation in MIPS

patch to support topdown mmap allocation in MIPS

[Jian Peng]

RE: patch to support topdown mmap allocation in MIPS

[Jian Peng]

Here is my testing program that intentionally caused OOM by calling mmap() to allocate 8MB each time.

Without patch, it failed at 168th mmap call, and with patch, it failed at 254th mmap call. It is 688MB more usable virtual address space.

/*
 * A simple testing program to cause OOM by calling mmap()
 *
 */
#include <stdio.h>
#include <stdlib.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/mman.h>

void *test_mmap(unsigned long addr, unsigned long size)
{
	void *pMem;

    	pMem = (char *)mmap((void *)addr, size, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
	printf("%s:addr=0x%08x, size=0x%x, return 0x%08x\n", __FUNCTION__, addr, size, pMem);
	
    	if (MAP_FAILED == pMem)
    	{
		printf("test_mmap FAILED\n");
        	pMem = NULL;
	}

    	return pMem;
}

int main(int argc, char **argv)
{
	int done = 0;
	int keep = 1;

	while(keep)
	{
		int i;
		int loops = 2000000000;

		if(!done) {
			for(i=0; i<loops; i++)
				if(test_mmap(0, 0x800000) == NULL)
				{	
					printf("\t\tFAILED at %d try\n", i);
					break;
				}
			done = 1;
		}	

		sleep(1);
	}
}

-----Original Message-----
From: linux-mips-bounce@linux-mips.org [mailto:linux-mips-bounce@linux-mips.org] On Behalf Of Jian Peng
Sent: Monday, May 16, 2011 2:10 PM
To: linux-mips@linux-mips.org
Cc: Ralf Baechle
Subject: patch to support topdown mmap allocation in MIPS

Re: patch to support topdown mmap allocation in MIPS

[David Daney]

On 05/16/2011 02:09 PM, Jian Peng wrote:
>> From cda3f14f0201342db9649376e9124167b42bbeba Mon Sep 17 00:00:00 2001
> From: Jian Peng<jipeng2005@gmail.com>
> Date: Mon, 16 May 2011 12:07:37 -0700
> Subject: [PATCH 1/1] MIPS: topdown mmap support
>
> This patch introduced topdown mmap support in user process address
> space allocation policy.
>
> Recently, we ran some large applications that use mmap heavily and
> lead to OOM due to inflexible mmap allocation policy on MIPS32.
>
> Since most other major archs supported it for years, it is reasonable
> to follow the trend and reduce the pain of porting applications.
>
> Due to cache aliasing concern, arch_get_unmapped_area_topdown() and
> other helper functions are implemented in arch/mips/kernel/syscall.c.
>
> Signed-off-by: Jian Peng<jipeng2005@gmail.com>
[...]
> +
> +/* add COLOUR_ALIGN_DOWN */

That is not a good comment.  We know you are adding it by all the '+' 
characters in the patch.

> +static inline unsigned long COLOUR_ALIGN_DOWN(unsigned long addr,
> +					      unsigned long pgoff)
> +{
> +	unsigned long base = addr&  ~shm_align_mask;
> +	unsigned long off = (pgoff<<  PAGE_SHIFT)&  shm_align_mask;
> +
> +	if (base + off<= addr)
> +		return base + off;
> +
> +	return base - off;
> +}
> +
>   #define COLOUR_ALIGN(addr,pgoff)				\
>   	((((addr) + shm_align_mask)&  ~shm_align_mask) +	\
>   	 (((pgoff)<<  PAGE_SHIFT)&  shm_align_mask))
> @@ -136,6 +185,125 @@ unsigned long arch_get_unmapped_area(struct file *filp, unsigned long addr,
>   	}
>   }
>
> +/* add  arch_get_unmapped_area_topdown */

Another bad comment.

> +unsigned long
> +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 do_colour_align;
> +	unsigned long task_size;
> +
> +#ifdef CONFIG_32BIT
> +	task_size = TASK_SIZE;
> +#else /* Must be CONFIG_64BIT*/
> +	task_size = test_thread_flag(TIF_32BIT_ADDR) ? TASK_SIZE32 : TASK_SIZE;
> +#endif
> +
> +	if (flags&  MAP_FIXED) {
> +		/* We do not accept a shared mapping if it would violate
> +		 * cache aliasing constraints.
> +		 */
> +		if ((flags&  MAP_SHARED)&&
> +		    ((addr - (pgoff<<  PAGE_SHIFT))&  shm_align_mask))
> +			return -EINVAL;
> +		return addr;
> +	}
> +
> +	if (unlikely(len>  task_size))
> +		return -ENOMEM;
> +


All this code you are duplicating from arch_get_unmapped_area(), but you 
introduce subtle bugs by removing some needed checks.

Why duplicate the code?

Why remove the checks?


David Daney

RE: patch to support topdown mmap allocation in MIPS

[Jian Peng]

Thank you for reviewing. Here is new one with all fixings you want.

Re: patch to support topdown mmap allocation in MIPS

[David Daney]

On 05/16/2011 06:06 PM, Jian Peng wrote:
> Thank you for reviewing. Here is new one with all fixings you want.
>
>> From 8dee85a8da61f74568ab9e0eadc8d7602a3f6fc6 Mon Sep 17 00:00:00 2001
> From: Jian Peng<jipeng2005@gmail.com>
> Date: Mon, 16 May 2011 18:00:33 -0700
> Subject: [PATCH 1/1] MIPS: topdown mmap support
>
> This patch introduced topdown mmap support in user process address
> space allocation policy.
>
> Recently, we ran some large applications that use mmap heavily and
> lead to OOM due to inflexible mmap allocation policy on MIPS32.
>
> Since most other major archs supported it for years, it is reasonable
> to follow the trend and reduce the pain of porting applications.
>
> Due to cache aliasing concern, arch_get_unmapped_area_topdown() and
> other helper functions are implemented in arch/mips/kernel/syscall.c.
>
> Signed-off-by: Jian Peng<jipeng2005@gmail.com>
> ---

Still too much code duplication.

Try regenerating after Ralf's recent patch, and do something like:

enum map_allocation_direction { up, down };

long arch_get_unmapped_area_foo(num map_allocation_direction, ....)
{
/* a function that does both without code duplication */
.
.
.
}

long arch_get_unmapped_area(....)
{
	return arch_get_unmapped_area_foo(up, ...);
}

long arch_get_unmapped_area_topdown(....)
{
	return arch_get_unmapped_area_foo(down, ...);
}



>   arch/mips/include/asm/pgtable.h |    1 +
>   arch/mips/kernel/syscall.c      |  185 +++++++++++++++++++++++++++++++++++++-
>   2 files changed, 181 insertions(+), 5 deletions(-)
>
> diff --git a/arch/mips/include/asm/pgtable.h b/arch/mips/include/asm/pgtable.h
> index 7e40f37..b2202a6 100644
> --- a/arch/mips/include/asm/pgtable.h
> +++ b/arch/mips/include/asm/pgtable.h
> @@ -414,6 +414,7 @@ int phys_mem_access_prot_allowed(struct file *file, unsigned long pfn,
>    * constraints placed on us by the cache architecture.
>    */
>   #define HAVE_ARCH_UNMAPPED_AREA
> +#define HAVE_ARCH_UNMAPPED_AREA_TOPDOWN
>
>   /*
>    * No page table caches to initialise
> diff --git a/arch/mips/kernel/syscall.c b/arch/mips/kernel/syscall.c
> index 58beabf..e2f0ed5 100644
> --- a/arch/mips/kernel/syscall.c
> +++ b/arch/mips/kernel/syscall.c
> @@ -42,6 +42,7 @@
>   #include<asm/shmparam.h>
>   #include<asm/sysmips.h>
>   #include<asm/uaccess.h>
> +#include<linux/personality.h>
>
>   /*
>    * For historic reasons the pipe(2) syscall on MIPS has an unusual calling
> @@ -70,14 +71,60 @@ unsigned long shm_align_mask = PAGE_SIZE - 1;	/* Sane caches */
>
>   EXPORT_SYMBOL(shm_align_mask);
>
> -#define COLOUR_ALIGN(addr,pgoff)				\
> +/* gap between mmap and stack */
> +#define MIN_GAP (128*1024*1024UL)
> +#define MAX_GAP(task_size)	((task_size)/6*5)
> +
> +static int mmap_is_legacy(void)
> +{
> +	if (current->personality&  ADDR_COMPAT_LAYOUT)
> +		return 1;
> +
> +	if (rlimit(RLIMIT_STACK) == RLIM_INFINITY)
> +		return 1;
> +
> +	return sysctl_legacy_va_layout;
> +}
> +
> +static unsigned long mmap_base(unsigned long rnd)
> +{
> +	unsigned long gap = rlimit(RLIMIT_STACK);
> +	unsigned long task_size;
> +
> +#ifdef CONFIG_32BIT
> +	task_size = TASK_SIZE;
> +#else /* Must be CONFIG_64BIT*/
> +	task_size = test_thread_flag(TIF_32BIT_ADDR) ? TASK_SIZE32 : TASK_SIZE;
> +#endif
> +
> +	if (gap<  MIN_GAP)
> +		gap = MIN_GAP;
> +	else if (gap>  MAX_GAP(task_size))
> +		gap = MAX_GAP(task_size);
> +
> +	return PAGE_ALIGN(task_size - gap - rnd);
> +}
> +
> +static inline unsigned long COLOUR_ALIGN_DOWN(unsigned long addr,
> +					      unsigned long pgoff)
> +{
> +	unsigned long base = addr&  ~shm_align_mask;
> +	unsigned long off = (pgoff<<  PAGE_SHIFT)&  shm_align_mask;
> +
> +	if (base + off<= addr)
> +		return base + off;
> +
> +	return base - off;
> +}
> +
> +#define COLOUR_ALIGN(addr, pgoff)				\
>   	((((addr) + shm_align_mask)&  ~shm_align_mask) +	\
>   	 (((pgoff)<<  PAGE_SHIFT)&  shm_align_mask))
>
>   unsigned long arch_get_unmapped_area(struct file *filp, unsigned long addr,
>   	unsigned long len, unsigned long pgoff, unsigned long flags)
>   {
> -	struct vm_area_struct * vmm;
> +	struct vm_area_struct *vmm;
>   	int do_color_align;
>   	unsigned long task_size;
>
> @@ -136,6 +183,128 @@ unsigned long arch_get_unmapped_area(struct file *filp, unsigned long addr,
>   	}
>   }
>
> +unsigned long
> +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 do_color_align;
> +	unsigned long task_size;
> +
> +#ifdef CONFIG_32BIT
> +	task_size = TASK_SIZE;
> +#else /* Must be CONFIG_64BIT*/
> +	task_size = test_thread_flag(TIF_32BIT_ADDR) ? TASK_SIZE32 : TASK_SIZE;
> +#endif
> +
> +	if (unlikely(len>  task_size))
> +		return -ENOMEM;
> +
> +	if (flags&  MAP_FIXED) {
> +		/* Even MAP_FIXED mappings must reside within task_size.  */
> +		if (task_size - len<  addr)
> +			return -EINVAL;
> +
> +		/* We do not accept a shared mapping if it would violate
> +		 * cache aliasing constraints.
> +		 */
> +		if ((flags&  MAP_SHARED)&&
> +		    ((addr - (pgoff<<  PAGE_SHIFT))&  shm_align_mask))
> +			return -EINVAL;
> +		return addr;
> +	}
> +
> +	do_color_align = 0;
> +	if (filp || (flags&  MAP_SHARED))
> +		do_color_align = 1;
> +
> +	/* requesting a specific address */
> +	if (addr) {
> +		if (do_color_align)
> +			addr = COLOUR_ALIGN(addr, pgoff);
> +		else
> +			addr = PAGE_ALIGN(addr);
> +
> +		vma = find_vma(mm, addr);
> +		if (task_size - len>= addr&&
> +		    (!vma || addr + len<= vma->vm_start))
> +			return addr;
> +	}
> +
> +	/* check if free_area_cache is useful for us */
> +	if (len<= mm->cached_hole_size) {
> +		mm->cached_hole_size = 0;
> +		mm->free_area_cache = mm->mmap_base;
> +	}
> +
> +	/* either no address requested or can't fit in requested address hole */
> +	addr = mm->free_area_cache;
> +	if (do_color_align) {
> +		unsigned long base = COLOUR_ALIGN_DOWN(addr-len, pgoff);
> +
> +		addr = base + len;
> +	}
> +
> +	/* make sure it can fit in the remaining address space */
> +	if (likely(addr>  len)) {
> +		vma = find_vma(mm, addr-len);
> +		if (!vma || addr<= vma->vm_start) {
> +			/* remember the address as a hint for next time */
> +			return mm->free_area_cache = addr-len;
> +		}
> +	}
> +
> +	if (unlikely(mm->mmap_base<  len))
> +		goto bottomup;
> +
> +	addr = mm->mmap_base-len;
> +	if (do_color_align)
> +		addr = COLOUR_ALIGN_DOWN(addr, pgoff);
> +
> +	do {
> +		/*
> +		 * Lookup failure means no vma is above this address,
> +		 * else if new region fits below vma->vm_start,
> +		 * return with success:
> +		 */
> +		vma = find_vma(mm, addr);
> +		if (likely(!vma || addr+len<= vma->vm_start)) {
> +			/* remember the address as a hint for next time */
> +			return mm->free_area_cache = addr;
> +		}
> +
> +		/* remember the largest hole we saw so far */
> +		if (addr + mm->cached_hole_size<  vma->vm_start)
> +			mm->cached_hole_size = vma->vm_start - addr;
> +
> +		/* try just below the current vma->vm_start */
> +		addr = vma->vm_start-len;
> +		if (do_color_align)
> +			addr = COLOUR_ALIGN_DOWN(addr, pgoff);
> +	} while (likely(len<  vma->vm_start));
> +
> +bottomup:
> +	/*
> +	 * 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.
> +	 */
> +	mm->cached_hole_size = ~0UL;
> +	mm->free_area_cache = TASK_UNMAPPED_BASE;
> +	addr = arch_get_unmapped_area(filp, addr0, len, pgoff, flags);
> +	/*
> +	 * Restore the topdown base:
> +	 */
> +	mm->free_area_cache = mm->mmap_base;
> +	mm->cached_hole_size = ~0UL;
> +
> +	return addr;
> +}
> +
>   void arch_pick_mmap_layout(struct mm_struct *mm)
>   {
>   	unsigned long random_factor = 0UL;
> @@ -149,9 +318,15 @@ void arch_pick_mmap_layout(struct mm_struct *mm)
>   			random_factor&= 0xffffffful;
>   	}
>
> -	mm->mmap_base = TASK_UNMAPPED_BASE + random_factor;
> -	mm->get_unmapped_area = arch_get_unmapped_area;
> -	mm->unmap_area = arch_unmap_area;
> +	if (mmap_is_legacy()) {
> +		mm->mmap_base = TASK_UNMAPPED_BASE + random_factor;
> +		mm->get_unmapped_area = arch_get_unmapped_area;
> +		mm->unmap_area = arch_unmap_area;
> +	} else {
> +		mm->mmap_base = mmap_base(random_factor);
> +		mm->get_unmapped_area = arch_get_unmapped_area_topdown;
> +		mm->unmap_area = arch_unmap_area_topdown;
> +	}
>   }
>
>   static inline unsigned long brk_rnd(void)

RE: patch to support topdown mmap allocation in MIPS

[Jian Peng]

This one merged Ralf's patch and removed duplication. I can only test it on MIPS32 system for now.

RE: patch to support topdown mmap allocation in MIPS

[Jian Peng]

Hi, Ralf/David,

What else should I do to get this patch merged?

Thanks,
Jian

-----Original Message-----
From: linux-mips-bounce@linux-mips.org [mailto:linux-mips-bounce@linux-mips.org] On Behalf Of Jian Peng
Sent: Tuesday, May 17, 2011 12:37 PM
To: David Daney
Cc: linux-mips@linux-mips.org; Ralf Baechle
Subject: RE: patch to support topdown mmap allocation in MIPS

This one merged Ralf's patch and removed duplication. I can only test it on MIPS32 system for now.

Re: patch to support topdown mmap allocation in MIPS

[David Daney]

On 05/25/2011 10:47 AM, Jian Peng wrote:
> Hi, Ralf/David,
>
> What else should I do to get this patch merged?
>

Be patient.  And tell how it was tested.


Also ....

[...]
> +
> +unsigned long arch_get_unmapped_area_foo(struct file *filp, unsigned long addr0,
> +               unsigned long len, unsigned long pgoff, unsigned long flags,
> +               enum mmap_allocation_direction dir)

I know I suggested the name *_foo, but really I expected you to choose a 
better name, as the 'foo' is just the default name for examples.

I think it needs a better name than that.

I will try to test it on my Octeon system sometime.

David Daney

RE: patch to support topdown mmap allocation in MIPS

[Jian Peng]

Hi, David,

I am willing to get more feedback and sort out issues before I forgot all details.

I post a simple testing program at http://www.linux-mips.org/archives/linux-mips/2011-05/msg00252.html
And it was also tested in a real application using mmap heavily and need this patch to avoid failure.

It is my bad to take your suggestion literally. How about arch_get_unmapped_area_common()?

Thanks,
Jian

-----Original Message-----
From: David Daney [mailto:ddaney@caviumnetworks.com] 
Sent: Wednesday, May 25, 2011 10:58 AM
To: Jian Peng
Cc: linux-mips@linux-mips.org; Ralf Baechle
Subject: Re: patch to support topdown mmap allocation in MIPS

On 05/25/2011 10:47 AM, Jian Peng wrote:
> Hi, Ralf/David,
>
> What else should I do to get this patch merged?
>

Be patient.  And tell how it was tested.


Also ....

[...]
> +
> +unsigned long arch_get_unmapped_area_foo(struct file *filp, unsigned long addr0,
> +               unsigned long len, unsigned long pgoff, unsigned long flags,
> +               enum mmap_allocation_direction dir)

I know I suggested the name *_foo, but really I expected you to choose a 
better name, as the 'foo' is just the default name for examples.

I think it needs a better name than that.

I will try to test it on my Octeon system sometime.

David Daney

RE: patch to support topdown mmap allocation in MIPS

[Jian Peng]

Hi, David/Ralf,

I tried to regenerate a patch for this, and after 

git clone git://git.linux-mips.org/pub/scm/linux

I found out that there is arch/mips/mm/mmap.c, but could not find out who/how this file was merged.

I doubt that I may "git clone" a wrong repo that is no longer valid.

Thanks,
Jian

-----Original Message-----
From: linux-mips-bounce@linux-mips.org [mailto:linux-mips-bounce@linux-mips.org] On Behalf Of Jian Peng
Sent: Wednesday, May 25, 2011 11:07 AM
To: David Daney
Cc: linux-mips@linux-mips.org; Ralf Baechle
Subject: RE: patch to support topdown mmap allocation in MIPS

Hi, David,

I am willing to get more feedback and sort out issues before I forgot all details.

I post a simple testing program at http://www.linux-mips.org/archives/linux-mips/2011-05/msg00252.html
And it was also tested in a real application using mmap heavily and need this patch to avoid failure.

It is my bad to take your suggestion literally. How about arch_get_unmapped_area_common()?

Thanks,
Jian

-----Original Message-----
From: David Daney [mailto:ddaney@caviumnetworks.com] 
Sent: Wednesday, May 25, 2011 10:58 AM
To: Jian Peng
Cc: linux-mips@linux-mips.org; Ralf Baechle
Subject: Re: patch to support topdown mmap allocation in MIPS

On 05/25/2011 10:47 AM, Jian Peng wrote:
> Hi, Ralf/David,
>
> What else should I do to get this patch merged?
>

Be patient.  And tell how it was tested.


Also ....

[...]
> +
> +unsigned long arch_get_unmapped_area_foo(struct file *filp, unsigned long addr0,
> +               unsigned long len, unsigned long pgoff, unsigned long flags,
> +               enum mmap_allocation_direction dir)

I know I suggested the name *_foo, but really I expected you to choose a 
better name, as the 'foo' is just the default name for examples.

I think it needs a better name than that.

I will try to test it on my Octeon system sometime.

David Daney

RE: patch to support topdown mmap allocation in MIPS

[Jian Peng]

Hi, David/Ralf,

I found out the commit log

commit 6f6c3c33c027f2c83d53e8562cd9daa73fe8108b
Author: Ralf Baechle <ralf@linux-mips.org>
Date:   Thu May 19 09:21:33 2011 +0100

    MIPS: Move arch_get_unmapped_area and gang to new file.
    
    It never really belonged into syscall.c and it's about to become well more
    complex.
    
    Signed-off-by: Ralf Baechle <ralf@linux-mips.org>


Ralf, do you want to pick up my patch on topdown mmap and merge into mmap.c or want me to do that?

I can test it if you have new patch.

Thanks,
Jian

-----Original Message-----
From: Jian Peng 
Sent: Monday, June 13, 2011 4:43 PM
To: Jian Peng; David Daney
Cc: linux-mips@linux-mips.org; Ralf Baechle
Subject: RE: patch to support topdown mmap allocation in MIPS

Hi, David/Ralf,

I tried to regenerate a patch for this, and after 

git clone git://git.linux-mips.org/pub/scm/linux

I found out that there is arch/mips/mm/mmap.c, but could not find out who/how this file was merged.

I doubt that I may "git clone" a wrong repo that is no longer valid.

Thanks,
Jian

-----Original Message-----
From: linux-mips-bounce@linux-mips.org [mailto:linux-mips-bounce@linux-mips.org] On Behalf Of Jian Peng
Sent: Wednesday, May 25, 2011 11:07 AM
To: David Daney
Cc: linux-mips@linux-mips.org; Ralf Baechle
Subject: RE: patch to support topdown mmap allocation in MIPS

Hi, David,

I am willing to get more feedback and sort out issues before I forgot all details.

I post a simple testing program at http://www.linux-mips.org/archives/linux-mips/2011-05/msg00252.html
And it was also tested in a real application using mmap heavily and need this patch to avoid failure.

It is my bad to take your suggestion literally. How about arch_get_unmapped_area_common()?

Thanks,
Jian

-----Original Message-----
From: David Daney [mailto:ddaney@caviumnetworks.com] 
Sent: Wednesday, May 25, 2011 10:58 AM
To: Jian Peng
Cc: linux-mips@linux-mips.org; Ralf Baechle
Subject: Re: patch to support topdown mmap allocation in MIPS

On 05/25/2011 10:47 AM, Jian Peng wrote:
> Hi, Ralf/David,
>
> What else should I do to get this patch merged?
>

Be patient.  And tell how it was tested.


Also ....

[...]
> +
> +unsigned long arch_get_unmapped_area_foo(struct file *filp, unsigned long addr0,
> +               unsigned long len, unsigned long pgoff, unsigned long flags,
> +               enum mmap_allocation_direction dir)

I know I suggested the name *_foo, but really I expected you to choose a 
better name, as the 'foo' is just the default name for examples.

I think it needs a better name than that.

I will try to test it on my Octeon system sometime.

David Daney

Re: patch to support topdown mmap allocation in MIPS

[Ralf Baechle]

On Wed, May 25, 2011 at 10:47:04AM -0700, Jian Peng wrote:

> What else should I do to get this patch merged?

Since there *still* is confusion about the merge status of this patch:
I merged https://patchwork.linux-mips.org/patch/2389/ already for
Linux 3.0 as commit id d0be89f6c2570a63ac44ccdd12473a54243cd296.

https://patchwork.linux-mips.org/patch/2412/ is still marked as "new"
in patchwork and I'm going to drop it now.

  Ralf

Re: patch to support topdown mmap allocation in MIPS

[Kevin Cernekee]

On Mon, May 16, 2011 at 5:12 PM, David Daney <ddaney@caviumnetworks.com> wrote:
> On 05/16/2011 02:09 PM, Jian Peng wrote:
>>  #define COLOUR_ALIGN(addr,pgoff)                              \
>>        ((((addr) + shm_align_mask)&  ~shm_align_mask) +        \
>>         (((pgoff)<<  PAGE_SHIFT)&  shm_align_mask))

I see COLOUR_ALIGN in arch/{arm,mips,sh,sparc} .  All sorts of
embedded platforms have to worry about cache aliases nowadays.

Do you think this logic could be folded into the generic
implementations in mm/mmap.c ?  Or is there something else inside our
arch_get_unmapped_area* functions that's really, irreparably unique to
MIPS?

>> +#ifdef CONFIG_32BIT
>> +       task_size = TASK_SIZE;
>> +#else /* Must be CONFIG_64BIT*/
>> +       task_size = test_thread_flag(TIF_32BIT_ADDR) ? TASK_SIZE32 :
>> TASK_SIZE;
>> +#endif

Can the "#else" clause and "task_size" local variable be eliminated?
TASK_SIZE now performs this check automatically (although that wasn't
always the case).

Re: patch to support topdown mmap allocation in MIPS

[Jian Peng]

Good points. I cannot access working system now and will investigate it further tomorrow.

On May 16, 2011, at 6:27 PM, "Kevin Cernekee" <cernekee@gmail.com> wrote:

> On Mon, May 16, 2011 at 5:12 PM, David Daney <ddaney@caviumnetworks.com> wrote:
>> On 05/16/2011 02:09 PM, Jian Peng wrote:
>>>  #define COLOUR_ALIGN(addr,pgoff)                              \
>>>        ((((addr) + shm_align_mask)&  ~shm_align_mask) +        \
>>>         (((pgoff)<<  PAGE_SHIFT)&  shm_align_mask))
> 
> I see COLOUR_ALIGN in arch/{arm,mips,sh,sparc} .  All sorts of
> embedded platforms have to worry about cache aliases nowadays.
> 
> Do you think this logic could be folded into the generic
> implementations in mm/mmap.c ?  Or is there something else inside our
> arch_get_unmapped_area* functions that's really, irreparably unique to
> MIPS?
> 
>>> +#ifdef CONFIG_32BIT
>>> +       task_size = TASK_SIZE;
>>> +#else /* Must be CONFIG_64BIT*/
>>> +       task_size = test_thread_flag(TIF_32BIT_ADDR) ? TASK_SIZE32 :
>>> TASK_SIZE;
>>> +#endif
> 
> Can the "#else" clause and "task_size" local variable be eliminated?
> TASK_SIZE now performs this check automatically (although that wasn't
> always the case).
> 

[PATCH] MIPS: Cleanup arch_get_unmapped_area

[Ralf Baechle]

On Mon, May 16, 2011 at 06:27:17PM -0700, Kevin Cernekee wrote:

> >> +#ifdef CONFIG_32BIT
> >> +       task_size = TASK_SIZE;
> >> +#else /* Must be CONFIG_64BIT*/
> >> +       task_size = test_thread_flag(TIF_32BIT_ADDR) ? TASK_SIZE32 :
> >> TASK_SIZE;
> >> +#endif
> 
> Can the "#else" clause and "task_size" local variable be eliminated?
> TASK_SIZE now performs this check automatically (although that wasn't
> always the case).


As noticed by Kevin Cernekee <cernekee@gmail.com> in
http://www.linux-mips.org/cgi-bin/extract-mesg.cgi?a=linux-mips&m=2011-05&i=BANLkTikq04wuK%3Dbz%2BLieavmm3oDtoYWKxg%40mail.gmail.com

Signed-off-by: Ralf Baechle <ralf@linux-mips.org>

 arch/mips/kernel/syscall.c |   17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

Index: linux-queue/arch/mips/kernel/syscall.c
===================================================================
--- linux-queue.orig/arch/mips/kernel/syscall.c
+++ linux-queue/arch/mips/kernel/syscall.c
@@ -79,20 +79,13 @@ unsigned long arch_get_unmapped_area(str
 {
 	struct vm_area_struct * vmm;
 	int do_color_align;
-	unsigned long task_size;
 
-#ifdef CONFIG_32BIT
-	task_size = TASK_SIZE;
-#else /* Must be CONFIG_64BIT*/
-	task_size = test_thread_flag(TIF_32BIT_ADDR) ? TASK_SIZE32 : TASK_SIZE;
-#endif
-
-	if (len > task_size)
+	if (len > TASK_SIZE)
 		return -ENOMEM;
 
 	if (flags & MAP_FIXED) {
-		/* Even MAP_FIXED mappings must reside within task_size.  */
-		if (task_size - len < addr)
+		/* Even MAP_FIXED mappings must reside within TASK_SIZE.  */
+		if (TASK_SIZE - len < addr)
 			return -EINVAL;
 
 		/*
@@ -114,7 +107,7 @@ unsigned long arch_get_unmapped_area(str
 		else
 			addr = PAGE_ALIGN(addr);
 		vmm = find_vma(current->mm, addr);
-		if (task_size - len >= addr &&
+		if (TASK_SIZE - len >= addr &&
 		    (!vmm || addr + len <= vmm->vm_start))
 			return addr;
 	}
@@ -126,7 +119,7 @@ unsigned long arch_get_unmapped_area(str
 
 	for (vmm = find_vma(current->mm, addr); ; vmm = vmm->vm_next) {
 		/* At this point:  (!vmm || addr < vmm->vm_end). */
-		if (task_size - len < addr)
+		if (TASK_SIZE - len < addr)
 			return -ENOMEM;
 		if (!vmm || addr + len <= vmm->vm_start)
 			return addr;

Re: patch to support topdown mmap allocation in MIPS

[Ralf Baechle]

On Mon, May 16, 2011 at 06:27:17PM -0700, Kevin Cernekee wrote:

> >>  #define COLOUR_ALIGN(addr,pgoff)                              \
> >>        ((((addr) + shm_align_mask)&  ~shm_align_mask) +        \
> >>         (((pgoff)<<  PAGE_SHIFT)&  shm_align_mask))
> 
> I see COLOUR_ALIGN in arch/{arm,mips,sh,sparc} .  All sorts of
> embedded platforms have to worry about cache aliases nowadays.
> 
> Do you think this logic could be folded into the generic
> implementations in mm/mmap.c ?  Or is there something else inside our
> arch_get_unmapped_area* functions that's really, irreparably unique to
> MIPS?

There are always slightly odd architectures such as IA-64 where the page
size depends on the address or PARISC which in most of its cache handling
is about as straight forward as programming in Malbolge.

It should be easy enough to come up with a version however that fits most
architectures and everybody else can just override it just like the default
version of arch_get_unmapped_area.

  Ralf