Re: [GIT PULL] parisc updates for v3.15

[Linus Torvalds]

On Thu, May 1, 2014 at 11:38 AM, Helge Deller <deller@xxxxxx> wrote:
>
> - Make mmap() behave similiar to other architectures: If a file hasn't been
>   mapped yet, we can now map it at any given page-aligned address.

Ugh, so I pulled this, but I'm going to unpull it, because I dislike
your new "i_mmap_lastmap" field.

Now, the i_mmap_lastmap field itself, I could probably live with, but
this change to generic code:

  +       if (vma->vm_flags & VM_SHARED) {
                  mapping->i_mmap_writable--;
  +#ifdef CONFIG_MMAP_TRACKING
  +               if (mapping->i_mmap_writable == 0)
  +                       mapping->i_mmap_lastmmap = 0;
  +#endif
  +       }

makes me just gouge my eyes out. It's not only uglifying generic code,
it's _stupid_ even when it's used.

What's magic about "i_mmap_lastmmap" having the value zero? Nothing.
Maybe somebody wants to map stuff at that zero value, and has the
permissions to do so.

So zeroing it is wrong. It's also entirely unnecessary, since you can
just look at the "mapping->i_mmap_writable" value instead. So instead
of checking "is i_mmap_lastmmap zero" as a way to check whether you
can now use any virtual address, which is wrong _anyway_, you should
have checked "is i_mmap_writable zero".

Now, I *also* suspect that you could just get rid of i_mmap_lastmmap
_entirely_, and instead just make "GET_LAST_MMAP()" just look up the
first shared mapping it can find in the rb tree (if i_mmap_writable is
non-null).

But if it was just that one (unnecessary) field in the "struct
address_space", I probably wouldn't mind, and say "ok, parisc has
broken virtual caches, what else is new". But the fact that it adds
code to the generic file just adds insult to injury and makes me go
"no, I don't want to pull this".

Mind fixing this? Here's a TOTALLY UNTESTED patch that may or may not
work, but might at least act as a starting point.

Hmm?

              Linus
 arch/parisc/kernel/sys_parisc.c | 38 ++++++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 12 deletions(-)

diff --git a/arch/parisc/kernel/sys_parisc.c b/arch/parisc/kernel/sys_parisc.c
index 31ffa9b55322..dd643459cbef 100644
--- a/arch/parisc/kernel/sys_parisc.c
+++ b/arch/parisc/kernel/sys_parisc.c
@@ -36,12 +36,32 @@
 #include <linux/personality.h>
 #include <linux/random.h>
 
-/* we construct an artificial offset for the mapping based on the physical
- * address of the kernel mapping variable */
-#define GET_LAST_MMAP(filp)		\
-	(filp ? ((unsigned long) filp->f_mapping) >> 8 : 0UL)
-#define SET_LAST_MMAP(filp, val)	\
-	 { /* nothing */ }
+static inline unsigned long find_shared_mapping_address(struct address_space *mapping)
+{
+	struct rb_node *nd;
+
+	for (nd = rb_first(root); nd; nd = rb_next(nd)) {
+		struct vm_area_struct *vma;
+		vma = rb_entry(nd, struct vm_area_struct, vm_rb);
+		if (vma->vm_flags & VM_SHARED)
+			return vma->start;
+	}
+	/* Shouldn't happen */
+	return 0;
+}
+
+/* the address_space struct holds a field i_mmap_lastmmap with the last mapping
+ * of this file for us */
+static inline unsigned long GET_LAST_MMAP(struct file *filp)
+{
+	if (filp) {
+		struct address_space *mapping = filp->f_mapping;
+		if (mapping->i_mmap_writable)
+			return find_shared_mapping_address(mapping);
+	}
+	return 0;
+}
+
 
 static int get_offset(unsigned int last_mmap)
 {
@@ -127,9 +147,6 @@ unsigned long arch_get_unmapped_area(struct file *filp, unsigned long addr,
 	addr = vm_unmapped_area(&info);
 
 found_addr:
-	if (do_color_align && !last_mmap && !(addr & ~PAGE_MASK))
-		SET_LAST_MMAP(filp, addr - (pgoff << PAGE_SHIFT));
-
 	return addr;
 }
 
@@ -198,9 +215,6 @@ arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
 	return arch_get_unmapped_area(filp, addr0, len, pgoff, flags);
 
 found_addr:
-	if (do_color_align && !last_mmap && !(addr & ~PAGE_MASK))
-		SET_LAST_MMAP(filp, addr - (pgoff << PAGE_SHIFT));
-
 	return addr;
 }