| Message ID | CA+55aFw+0_Wf+_2EAdi=OhVK3cZdMwsCV5cditaLz7LJroeByA@mail.gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Linus, On 05/01/2014 09:23 PM, Linus Torvalds wrote: > On Thu, May 1, 2014 at 11:38 AM, Helge Deller <deller@gmx.de> 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. I agree that this idea looks much better. I will try to rewrite this code as you suggested (it sadly didn't worked out of the box). But since it will take me some time, do you mind to pull in the other two remaining patches? They fix a bug with GNU make on our debian build servers and I would like one of them to be backported into stable for 3.14... Here is the *new* pull request (I dropped the patch above!): Hi Linus, please pull the latest parisc architecture fixes for kernel 3.15 from: git://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux.git parisc-3.15-3 Those two patches drop the architecture-specifc value for_STK_LIM_MAX to fix stack related problems with GNU make. Thanks, Helge ---------------------------------------------------------------- Helge Deller (1): parisc: Use generic uapi/asm/resource.h file John David Anglin (1): parisc: remove _STK_LIM_MAX override arch/parisc/include/uapi/asm/Kbuild | 3 ++- arch/parisc/include/uapi/asm/resource.h | 7 ------- 2 files changed, 2 insertions(+), 8 deletions(-) delete mode 100644 arch/parisc/include/uapi/asm/resource.h -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 1, 2014 at 2:55 PM, Helge Deller <deller@gmx.de> wrote: > > I agree that this idea looks much better. > I will try to rewrite this code as you suggested (it sadly didn't worked out of the box). > But since it will take me some time, do you mind to pull in the other two remaining patches? Done. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }