On 05/16/2011 02:09 PM, Jian Peng wrote:
From cda3f14f0201342db9649376e9124167b42bbeba Mon Sep 17 00:00:00 2001
From: Jian Peng<jipeng2005@xxxxxxxxx>
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@xxxxxxxxx>
[...]
+
+/* 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
