[PATCH v5 0/3] introduce static_vm for ARM-specific static mapped area

[PATCH v5 0/3] introduce static_vm for ARM-specific static mapped area

[Joonsoo Kim]

In current implementation, we used ARM-specific flag, that is,
VM_ARM_STATIC_MAPPING, for distinguishing ARM specific static mapped area.
The purpose of static mapped area is to re-use static mapped area when
entire physical address range of the ioremap request can be covered
by this area.

This implementation causes needless overhead for some cases.
For example, assume that there is only one static mapped area and
vmlist has 300 areas. Every time we call ioremap, we check 300 areas for
deciding whether it is matched or not. Moreover, even if there is
no static mapped area and vmlist has 300 areas, every time we call
ioremap, we check 300 areas in now.

If we construct a extra list for static mapped area, we can eliminate
above mentioned overhead.
With a extra list, if there is one static mapped area,
we just check only one area and proceed next operation quickly.

In fact, it is not a critical problem, because ioremap is not frequently
used. But reducing overhead is better idea.

Another reason for doing this work is for removing vm_struct list management,
entirely. For more information, look at the following link.
http://lkml.org/lkml/2012/12/6/184

Changelog
v4->v5:
  [2/3]: Changes from Nicolas' suggestion
  	 - don't use separate flags for static_vm
  	 - remove a lock
  	 - declare add_static_vm_early() as __init
  [3/3]: Changes from Nicolas' suggestion
  	 - add / leave comments

v3->v4:
  [2/3]: Changes from Nicolas' suggestion
         - embed static_vm code in ioremap.c
         - simplify struct static_vm
         - remove init_static_vm, instead, add_static_vm_early() init static_vm
         Use generic list for list management of static_vm
         Convert spin_lock to rw_lock
         Modify static_vm's flags bits
  [3/3]: Rework according to [2/3] change
  Rebased on v3.8-rc5

v2->v3:
  coverletter: refer a link related to this work
  [2/3]: drop @flags of find_static_vm_vaddr
  Rebased on v3.8-rc4

v1->v2: 
  [2/3]: patch description is improved.
  Rebased on v3.7-rc7

Joonsoo Kim (3):
  ARM: vmregion: remove vmregion code entirely
  ARM: ioremap: introduce an infrastructure for static mapped area
  ARM: mm: use static_vm for managing static mapped areas

 arch/arm/mm/Makefile   |    2 +-
 arch/arm/mm/ioremap.c  |  135 +++++++++++++++++++++----------
 arch/arm/mm/mm.h       |   12 +++
 arch/arm/mm/mmu.c      |   34 ++++----
 arch/arm/mm/vmregion.c |  205 ------------------------------------------------
 arch/arm/mm/vmregion.h |   31 --------
 6 files changed, 123 insertions(+), 296 deletions(-)
 delete mode 100644 arch/arm/mm/vmregion.c
 delete mode 100644 arch/arm/mm/vmregion.h

-- 
1.7.9.5

[PATCH v5 1/3] ARM: vmregion: remove vmregion code entirely

[Joonsoo Kim]

Now, there is no user for vmregion.
So remove it.

Acked-by: Nicolas Pitre <nico@linaro.org>
Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

diff --git a/arch/arm/mm/Makefile b/arch/arm/mm/Makefile
index 8a9c4cb..4e333fa 100644
--- a/arch/arm/mm/Makefile
+++ b/arch/arm/mm/Makefile
@@ -6,7 +6,7 @@ obj-y				:= dma-mapping.o extable.o fault.o init.o \
 				   iomap.o
 
 obj-$(CONFIG_MMU)		+= fault-armv.o flush.o idmap.o ioremap.o \
-				   mmap.o pgd.o mmu.o vmregion.o
+				   mmap.o pgd.o mmu.o
 
 ifneq ($(CONFIG_MMU),y)
 obj-y				+= nommu.o
diff --git a/arch/arm/mm/vmregion.c b/arch/arm/mm/vmregion.c
deleted file mode 100644
index a631016..0000000
--- a/arch/arm/mm/vmregion.c
+++ /dev/null
@@ -1,205 +0,0 @@
-#include <linux/fs.h>
-#include <linux/spinlock.h>
-#include <linux/list.h>
-#include <linux/proc_fs.h>
-#include <linux/seq_file.h>
-#include <linux/slab.h>
-
-#include "vmregion.h"
-
-/*
- * VM region handling support.
- *
- * This should become something generic, handling VM region allocations for
- * vmalloc and similar (ioremap, module space, etc).
- *
- * I envisage vmalloc()'s supporting vm_struct becoming:
- *
- *  struct vm_struct {
- *    struct vmregion	region;
- *    unsigned long	flags;
- *    struct page	**pages;
- *    unsigned int	nr_pages;
- *    unsigned long	phys_addr;
- *  };
- *
- * get_vm_area() would then call vmregion_alloc with an appropriate
- * struct vmregion head (eg):
- *
- *  struct vmregion vmalloc_head = {
- *	.vm_list	= LIST_HEAD_INIT(vmalloc_head.vm_list),
- *	.vm_start	= VMALLOC_START,
- *	.vm_end		= VMALLOC_END,
- *  };
- *
- * However, vmalloc_head.vm_start is variable (typically, it is dependent on
- * the amount of RAM found at boot time.)  I would imagine that get_vm_area()
- * would have to initialise this each time prior to calling vmregion_alloc().
- */
-
-struct arm_vmregion *
-arm_vmregion_alloc(struct arm_vmregion_head *head, size_t align,
-		   size_t size, gfp_t gfp, const void *caller)
-{
-	unsigned long start = head->vm_start, addr = head->vm_end;
-	unsigned long flags;
-	struct arm_vmregion *c, *new;
-
-	if (head->vm_end - head->vm_start < size) {
-		printk(KERN_WARNING "%s: allocation too big (requested %#x)\n",
-			__func__, size);
-		goto out;
-	}
-
-	new = kmalloc(sizeof(struct arm_vmregion), gfp);
-	if (!new)
-		goto out;
-
-	new->caller = caller;
-
-	spin_lock_irqsave(&head->vm_lock, flags);
-
-	addr = rounddown(addr - size, align);
-	list_for_each_entry_reverse(c, &head->vm_list, vm_list) {
-		if (addr >= c->vm_end)
-			goto found;
-		addr = rounddown(c->vm_start - size, align);
-		if (addr < start)
-			goto nospc;
-	}
-
- found:
-	/*
-	 * Insert this entry after the one we found.
-	 */
-	list_add(&new->vm_list, &c->vm_list);
-	new->vm_start = addr;
-	new->vm_end = addr + size;
-	new->vm_active = 1;
-
-	spin_unlock_irqrestore(&head->vm_lock, flags);
-	return new;
-
- nospc:
-	spin_unlock_irqrestore(&head->vm_lock, flags);
-	kfree(new);
- out:
-	return NULL;
-}
-
-static struct arm_vmregion *__arm_vmregion_find(struct arm_vmregion_head *head, unsigned long addr)
-{
-	struct arm_vmregion *c;
-
-	list_for_each_entry(c, &head->vm_list, vm_list) {
-		if (c->vm_active && c->vm_start == addr)
-			goto out;
-	}
-	c = NULL;
- out:
-	return c;
-}
-
-struct arm_vmregion *arm_vmregion_find(struct arm_vmregion_head *head, unsigned long addr)
-{
-	struct arm_vmregion *c;
-	unsigned long flags;
-
-	spin_lock_irqsave(&head->vm_lock, flags);
-	c = __arm_vmregion_find(head, addr);
-	spin_unlock_irqrestore(&head->vm_lock, flags);
-	return c;
-}
-
-struct arm_vmregion *arm_vmregion_find_remove(struct arm_vmregion_head *head, unsigned long addr)
-{
-	struct arm_vmregion *c;
-	unsigned long flags;
-
-	spin_lock_irqsave(&head->vm_lock, flags);
-	c = __arm_vmregion_find(head, addr);
-	if (c)
-		c->vm_active = 0;
-	spin_unlock_irqrestore(&head->vm_lock, flags);
-	return c;
-}
-
-void arm_vmregion_free(struct arm_vmregion_head *head, struct arm_vmregion *c)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&head->vm_lock, flags);
-	list_del(&c->vm_list);
-	spin_unlock_irqrestore(&head->vm_lock, flags);
-
-	kfree(c);
-}
-
-#ifdef CONFIG_PROC_FS
-static int arm_vmregion_show(struct seq_file *m, void *p)
-{
-	struct arm_vmregion *c = list_entry(p, struct arm_vmregion, vm_list);
-
-	seq_printf(m, "0x%08lx-0x%08lx %7lu", c->vm_start, c->vm_end,
-		c->vm_end - c->vm_start);
-	if (c->caller)
-		seq_printf(m, " %pS", (void *)c->caller);
-	seq_putc(m, '\n');
-	return 0;
-}
-
-static void *arm_vmregion_start(struct seq_file *m, loff_t *pos)
-{
-	struct arm_vmregion_head *h = m->private;
-	spin_lock_irq(&h->vm_lock);
-	return seq_list_start(&h->vm_list, *pos);
-}
-
-static void *arm_vmregion_next(struct seq_file *m, void *p, loff_t *pos)
-{
-	struct arm_vmregion_head *h = m->private;
-	return seq_list_next(p, &h->vm_list, pos);
-}
-
-static void arm_vmregion_stop(struct seq_file *m, void *p)
-{
-	struct arm_vmregion_head *h = m->private;
-	spin_unlock_irq(&h->vm_lock);
-}
-
-static const struct seq_operations arm_vmregion_ops = {
-	.start	= arm_vmregion_start,
-	.stop	= arm_vmregion_stop,
-	.next	= arm_vmregion_next,
-	.show	= arm_vmregion_show,
-};
-
-static int arm_vmregion_open(struct inode *inode, struct file *file)
-{
-	struct arm_vmregion_head *h = PDE(inode)->data;
-	int ret = seq_open(file, &arm_vmregion_ops);
-	if (!ret) {
-		struct seq_file *m = file->private_data;
-		m->private = h;
-	}
-	return ret;
-}
-
-static const struct file_operations arm_vmregion_fops = {
-	.open	= arm_vmregion_open,
-	.read	= seq_read,
-	.llseek	= seq_lseek,
-	.release = seq_release,
-};
-
-int arm_vmregion_create_proc(const char *path, struct arm_vmregion_head *h)
-{
-	proc_create_data(path, S_IRUSR, NULL, &arm_vmregion_fops, h);
-	return 0;
-}
-#else
-int arm_vmregion_create_proc(const char *path, struct arm_vmregion_head *h)
-{
-	return 0;
-}
-#endif
diff --git a/arch/arm/mm/vmregion.h b/arch/arm/mm/vmregion.h
deleted file mode 100644
index 0f5a5f2..0000000
--- a/arch/arm/mm/vmregion.h
+++ /dev/null
@@ -1,31 +0,0 @@
-#ifndef VMREGION_H
-#define VMREGION_H
-
-#include <linux/spinlock.h>
-#include <linux/list.h>
-
-struct page;
-
-struct arm_vmregion_head {
-	spinlock_t		vm_lock;
-	struct list_head	vm_list;
-	unsigned long		vm_start;
-	unsigned long		vm_end;
-};
-
-struct arm_vmregion {
-	struct list_head	vm_list;
-	unsigned long		vm_start;
-	unsigned long		vm_end;
-	int			vm_active;
-	const void		*caller;
-};
-
-struct arm_vmregion *arm_vmregion_alloc(struct arm_vmregion_head *, size_t, size_t, gfp_t, const void *);
-struct arm_vmregion *arm_vmregion_find(struct arm_vmregion_head *, unsigned long);
-struct arm_vmregion *arm_vmregion_find_remove(struct arm_vmregion_head *, unsigned long);
-void arm_vmregion_free(struct arm_vmregion_head *, struct arm_vmregion *);
-
-int arm_vmregion_create_proc(const char *, struct arm_vmregion_head *);
-
-#endif
-- 
1.7.9.5

[PATCH v5 2/3] ARM: ioremap: introduce an infrastructure for static mapped area

[Joonsoo Kim]

In current implementation, we used ARM-specific flag, that is,
VM_ARM_STATIC_MAPPING, for distinguishing ARM specific static mapped area.
The purpose of static mapped area is to re-use static mapped area when
entire physical address range of the ioremap request can be covered
by this area.

This implementation causes needless overhead for some cases.
For example, assume that there is only one static mapped area and
vmlist has 300 areas. Every time we call ioremap, we check 300 areas for
deciding whether it is matched or not. Moreover, even if there is
no static mapped area and vmlist has 300 areas, every time we call
ioremap, we check 300 areas in now.

If we construct a extra list for static mapped area, we can eliminate
above mentioned overhead.
With a extra list, if there is one static mapped area,
we just check only one area and proceed next operation quickly.

In fact, it is not a critical problem, because ioremap is not frequently
used. But reducing overhead is better idea.

Another reason for doing this work is for removing architecture dependency
on vmalloc layer. I think that vmlist and vmlist_lock is internal data
structure for vmalloc layer. Some codes for debugging and stat inevitably
use vmlist and vmlist_lock. But it is preferable that they are used
as least as possible in outside of vmalloc.c

Now, I introduce an ARM-specific infrastructure for static mapped area. In
the following patch, we will use this and resolve above mentioned problem.

Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
index 88fd86c..904c15e 100644
--- a/arch/arm/mm/ioremap.c
+++ b/arch/arm/mm/ioremap.c
@@ -39,6 +39,70 @@
 #include <asm/mach/pci.h>
 #include "mm.h"
 
+
+LIST_HEAD(static_vmlist);
+
+static struct static_vm *find_static_vm_paddr(phys_addr_t paddr,
+			size_t size, unsigned int mtype)
+{
+	struct static_vm *svm;
+	struct vm_struct *vm;
+
+	list_for_each_entry(svm, &static_vmlist, list) {
+		vm = &svm->vm;
+		if (!(vm->flags & VM_ARM_STATIC_MAPPING))
+			continue;
+		if ((vm->flags & VM_ARM_MTYPE_MASK) != VM_ARM_MTYPE(mtype))
+			continue;
+
+		if (vm->phys_addr > paddr ||
+			paddr + size - 1 > vm->phys_addr + vm->size - 1)
+			continue;
+
+		return svm;
+	}
+
+	return NULL;
+}
+
+struct static_vm *find_static_vm_vaddr(void *vaddr)
+{
+	struct static_vm *svm;
+	struct vm_struct *vm;
+
+	list_for_each_entry(svm, &static_vmlist, list) {
+		vm = &svm->vm;
+
+		/* static_vmlist is ascending order */
+		if (vm->addr > vaddr)
+			break;
+
+		if (vm->addr <= vaddr && vm->addr + vm->size > vaddr)
+			return svm;
+	}
+
+	return NULL;
+}
+
+void __init add_static_vm_early(struct static_vm *svm)
+{
+	struct static_vm *curr_svm;
+	struct vm_struct *vm;
+	void *vaddr;
+
+	vm = &svm->vm;
+	vm_area_add_early(vm);
+	vaddr = vm->addr;
+
+	list_for_each_entry(curr_svm, &static_vmlist, list) {
+		vm = &curr_svm->vm;
+
+		if (vm->addr > vaddr)
+			break;
+	}
+	list_add_tail(&svm->list, &curr_svm->list);
+}
+
 int ioremap_page(unsigned long virt, unsigned long phys,
 		 const struct mem_type *mtype)
 {
diff --git a/arch/arm/mm/mm.h b/arch/arm/mm/mm.h
index a8ee92d..d5a4e9a 100644
--- a/arch/arm/mm/mm.h
+++ b/arch/arm/mm/mm.h
@@ -1,4 +1,6 @@
 #ifdef CONFIG_MMU
+#include <linux/list.h>
+#include <linux/vmalloc.h>
 
 /* the upper-most page table pointer */
 extern pmd_t *top_pmd;
@@ -65,6 +67,16 @@ extern void __flush_dcache_page(struct address_space *mapping, struct page *page
 /* consistent regions used by dma_alloc_attrs() */
 #define VM_ARM_DMA_CONSISTENT	0x20000000
 
+
+struct static_vm {
+	struct vm_struct vm;
+	struct list_head list;
+};
+
+extern struct list_head static_vmlist;
+extern struct static_vm *find_static_vm_vaddr(void *vaddr);
+extern __init void add_static_vm_early(struct static_vm *svm);
+
 #endif
 
 #ifdef CONFIG_ZONE_DMA
-- 
1.7.9.5

[PATCH v5 3/3] ARM: mm: use static_vm for managing static mapped areas

[Joonsoo Kim]

A static mapped area is ARM-specific, so it is better not to use
generic vmalloc data structure, that is, vmlist and vmlist_lock
for managing static mapped area. And it causes some needless overhead and
reducing this overhead is better idea.

Now, we have newly introduced static_vm infrastructure.
With it, we don't need to iterate all mapped areas. Instead, we just
iterate static mapped areas. It helps to reduce an overhead of finding
matched area. And architecture dependency on vmalloc layer is removed,
so it will help to maintainability for vmalloc layer.

Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
index 904c15e..c7fef4b 100644
--- a/arch/arm/mm/ioremap.c
+++ b/arch/arm/mm/ioremap.c
@@ -261,13 +261,14 @@ void __iomem * __arm_ioremap_pfn_caller(unsigned long pfn,
 	const struct mem_type *type;
 	int err;
 	unsigned long addr;
- 	struct vm_struct * area;
+	struct vm_struct *area;
+	phys_addr_t paddr = __pfn_to_phys(pfn);
 
 #ifndef CONFIG_ARM_LPAE
 	/*
 	 * High mappings must be supersection aligned
 	 */
-	if (pfn >= 0x100000 && (__pfn_to_phys(pfn) & ~SUPERSECTION_MASK))
+	if (pfn >= 0x100000 && (paddr & ~SUPERSECTION_MASK))
 		return NULL;
 #endif
 
@@ -283,24 +284,16 @@ void __iomem * __arm_ioremap_pfn_caller(unsigned long pfn,
 	/*
 	 * Try to reuse one of the static mapping whenever possible.
 	 */
-	read_lock(&vmlist_lock);
-	for (area = vmlist; area; area = area->next) {
-		if (!size || (sizeof(phys_addr_t) == 4 && pfn >= 0x100000))
-			break;
-		if (!(area->flags & VM_ARM_STATIC_MAPPING))
-			continue;
-		if ((area->flags & VM_ARM_MTYPE_MASK) != VM_ARM_MTYPE(mtype))
-			continue;
-		if (__phys_to_pfn(area->phys_addr) > pfn ||
-		    __pfn_to_phys(pfn) + size-1 > area->phys_addr + area->size-1)
-			continue;
-		/* we can drop the lock here as we know *area is static */
-		read_unlock(&vmlist_lock);
-		addr = (unsigned long)area->addr;
-		addr += __pfn_to_phys(pfn) - area->phys_addr;
-		return (void __iomem *) (offset + addr);
+	if (size && !((sizeof(phys_addr_t) == 4 && pfn >= 0x100000))) {
+		struct static_vm *svm;
+
+		svm = find_static_vm_paddr(paddr, size, mtype);
+		if (svm) {
+			addr = (unsigned long)svm->vm.addr;
+			addr += paddr - svm->vm.phys_addr;
+			return (void __iomem *) (offset + addr);
+		}
 	}
-	read_unlock(&vmlist_lock);
 
 	/*
 	 * Don't allow RAM to be mapped - this causes problems with ARMv6+
@@ -312,21 +305,21 @@ void __iomem * __arm_ioremap_pfn_caller(unsigned long pfn,
  	if (!area)
  		return NULL;
  	addr = (unsigned long)area->addr;
-	area->phys_addr = __pfn_to_phys(pfn);
+	area->phys_addr = paddr;
 
 #if !defined(CONFIG_SMP) && !defined(CONFIG_ARM_LPAE)
 	if (DOMAIN_IO == 0 &&
 	    (((cpu_architecture() >= CPU_ARCH_ARMv6) && (get_cr() & CR_XP)) ||
 	       cpu_is_xsc3()) && pfn >= 0x100000 &&
-	       !((__pfn_to_phys(pfn) | size | addr) & ~SUPERSECTION_MASK)) {
+	       !((paddr | size | addr) & ~SUPERSECTION_MASK)) {
 		area->flags |= VM_ARM_SECTION_MAPPING;
 		err = remap_area_supersections(addr, pfn, size, type);
-	} else if (!((__pfn_to_phys(pfn) | size | addr) & ~PMD_MASK)) {
+	} else if (!((paddr | size | addr) & ~PMD_MASK)) {
 		area->flags |= VM_ARM_SECTION_MAPPING;
 		err = remap_area_sections(addr, pfn, size, type);
 	} else
 #endif
-		err = ioremap_page_range(addr, addr + size, __pfn_to_phys(pfn),
+		err = ioremap_page_range(addr, addr + size, paddr,
 					 __pgprot(type->prot_pte));
 
 	if (err) {
@@ -410,34 +403,28 @@ __arm_ioremap_exec(unsigned long phys_addr, size_t size, bool cached)
 void __iounmap(volatile void __iomem *io_addr)
 {
 	void *addr = (void *)(PAGE_MASK & (unsigned long)io_addr);
-	struct vm_struct *vm;
+	struct static_vm *svm;
+
+	/* If this is a static mapping, we must leave it alone */
+	svm = find_static_vm_vaddr(addr);
+	if (svm)
+		return;
 
-	read_lock(&vmlist_lock);
-	for (vm = vmlist; vm; vm = vm->next) {
-		if (vm->addr > addr)
-			break;
-		if (!(vm->flags & VM_IOREMAP))
-			continue;
-		/* If this is a static mapping we must leave it alone */
-		if ((vm->flags & VM_ARM_STATIC_MAPPING) &&
-		    (vm->addr <= addr) && (vm->addr + vm->size > addr)) {
-			read_unlock(&vmlist_lock);
-			return;
-		}
 #if !defined(CONFIG_SMP) && !defined(CONFIG_ARM_LPAE)
+	{
+		struct vm_struct *vm;
+
+		vm = find_vm_area(addr);
+
 		/*
 		 * If this is a section based mapping we need to handle it
 		 * specially as the VM subsystem does not know how to handle
 		 * such a beast.
 		 */
-		if ((vm->addr == addr) &&
-		    (vm->flags & VM_ARM_SECTION_MAPPING)) {
+		if (vm && (vm->flags & VM_ARM_SECTION_MAPPING))
 			unmap_area_sections((unsigned long)vm->addr, vm->size);
-			break;
-		}
-#endif
 	}
-	read_unlock(&vmlist_lock);
+#endif
 
 	vunmap(addr);
 }
diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index ce328c7..b2c0356 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -757,21 +757,24 @@ void __init iotable_init(struct map_desc *io_desc, int nr)
 {
 	struct map_desc *md;
 	struct vm_struct *vm;
+	struct static_vm *svm;
 
 	if (!nr)
 		return;
 
-	vm = early_alloc_aligned(sizeof(*vm) * nr, __alignof__(*vm));
+	svm = early_alloc_aligned(sizeof(*svm) * nr, __alignof__(*svm));
 
 	for (md = io_desc; nr; md++, nr--) {
 		create_mapping(md);
+
+		vm = &svm->vm;
 		vm->addr = (void *)(md->virtual & PAGE_MASK);
 		vm->size = PAGE_ALIGN(md->length + (md->virtual & ~PAGE_MASK));
 		vm->phys_addr = __pfn_to_phys(md->pfn);
 		vm->flags = VM_IOREMAP | VM_ARM_STATIC_MAPPING;
 		vm->flags |= VM_ARM_MTYPE(md->type);
 		vm->caller = iotable_init;
-		vm_area_add_early(vm++);
+		add_static_vm_early(svm++);
 	}
 }
 
@@ -779,13 +782,16 @@ void __init vm_reserve_area_early(unsigned long addr, unsigned long size,
 				  void *caller)
 {
 	struct vm_struct *vm;
+	struct static_vm *svm;
+
+	svm = early_alloc_aligned(sizeof(*svm), __alignof__(*svm));
 
-	vm = early_alloc_aligned(sizeof(*vm), __alignof__(*vm));
+	vm = &svm->vm;
 	vm->addr = (void *)addr;
 	vm->size = size;
 	vm->flags = VM_IOREMAP | VM_ARM_EMPTY_MAPPING;
 	vm->caller = caller;
-	vm_area_add_early(vm);
+	add_static_vm_early(svm);
 }
 
 #ifndef CONFIG_ARM_LPAE
@@ -810,14 +816,13 @@ static void __init pmd_empty_section_gap(unsigned long addr)
 
 static void __init fill_pmd_gaps(void)
 {
+	struct static_vm *svm;
 	struct vm_struct *vm;
 	unsigned long addr, next = 0;
 	pmd_t *pmd;
 
-	/* we're still single threaded hence no lock needed here */
-	for (vm = vmlist; vm; vm = vm->next) {
-		if (!(vm->flags & (VM_ARM_STATIC_MAPPING | VM_ARM_EMPTY_MAPPING)))
-			continue;
+	list_for_each_entry(svm, &static_vmlist, list) {
+		vm = &svm->vm;
 		addr = (unsigned long)vm->addr;
 		if (addr < next)
 			continue;
@@ -859,17 +864,12 @@ static void __init pci_reserve_io(void)
 {
 	struct vm_struct *vm;
 	unsigned long addr;
+	struct static_vm *svm;
 
-	/* we're still single threaded hence no lock needed here */
-	for (vm = vmlist; vm; vm = vm->next) {
-		if (!(vm->flags & VM_ARM_STATIC_MAPPING))
-			continue;
-		addr = (unsigned long)vm->addr;
-		addr &= ~(SZ_2M - 1);
-		if (addr == PCI_IO_VIRT_BASE)
-			return;
+	svm = find_static_vm_vaddr((void *)PCI_IO_VIRT_BASE);
+	if (svm)
+		return;
 
-	}
 	vm_reserve_area_early(PCI_IO_VIRT_BASE, SZ_2M, pci_reserve_io);
 }
 #else
-- 
1.7.9.5

Re: [PATCH v5 2/3] ARM: ioremap: introduce an infrastructure for static mapped area

[Nicolas Pitre]

On Tue, 5 Feb 2013, Joonsoo Kim wrote:

> In current implementation, we used ARM-specific flag, that is,
> VM_ARM_STATIC_MAPPING, for distinguishing ARM specific static mapped area.
> The purpose of static mapped area is to re-use static mapped area when
> entire physical address range of the ioremap request can be covered
> by this area.
> 
> This implementation causes needless overhead for some cases.
> For example, assume that there is only one static mapped area and
> vmlist has 300 areas. Every time we call ioremap, we check 300 areas for
> deciding whether it is matched or not. Moreover, even if there is
> no static mapped area and vmlist has 300 areas, every time we call
> ioremap, we check 300 areas in now.
> 
> If we construct a extra list for static mapped area, we can eliminate
> above mentioned overhead.
> With a extra list, if there is one static mapped area,
> we just check only one area and proceed next operation quickly.
> 
> In fact, it is not a critical problem, because ioremap is not frequently
> used. But reducing overhead is better idea.
> 
> Another reason for doing this work is for removing architecture dependency
> on vmalloc layer. I think that vmlist and vmlist_lock is internal data
> structure for vmalloc layer. Some codes for debugging and stat inevitably
> use vmlist and vmlist_lock. But it is preferable that they are used
> as least as possible in outside of vmalloc.c
> 
> Now, I introduce an ARM-specific infrastructure for static mapped area. In
> the following patch, we will use this and resolve above mentioned problem.
> 
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

Reviewed-by: Nicolas Pitre <nico@linaro.org>


> 
> diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
> index 88fd86c..904c15e 100644
> --- a/arch/arm/mm/ioremap.c
> +++ b/arch/arm/mm/ioremap.c
> @@ -39,6 +39,70 @@
>  #include <asm/mach/pci.h>
>  #include "mm.h"
>  
> +
> +LIST_HEAD(static_vmlist);
> +
> +static struct static_vm *find_static_vm_paddr(phys_addr_t paddr,
> +			size_t size, unsigned int mtype)
> +{
> +	struct static_vm *svm;
> +	struct vm_struct *vm;
> +
> +	list_for_each_entry(svm, &static_vmlist, list) {
> +		vm = &svm->vm;
> +		if (!(vm->flags & VM_ARM_STATIC_MAPPING))
> +			continue;
> +		if ((vm->flags & VM_ARM_MTYPE_MASK) != VM_ARM_MTYPE(mtype))
> +			continue;
> +
> +		if (vm->phys_addr > paddr ||
> +			paddr + size - 1 > vm->phys_addr + vm->size - 1)
> +			continue;
> +
> +		return svm;
> +	}
> +
> +	return NULL;
> +}
> +
> +struct static_vm *find_static_vm_vaddr(void *vaddr)
> +{
> +	struct static_vm *svm;
> +	struct vm_struct *vm;
> +
> +	list_for_each_entry(svm, &static_vmlist, list) {
> +		vm = &svm->vm;
> +
> +		/* static_vmlist is ascending order */
> +		if (vm->addr > vaddr)
> +			break;
> +
> +		if (vm->addr <= vaddr && vm->addr + vm->size > vaddr)
> +			return svm;
> +	}
> +
> +	return NULL;
> +}
> +
> +void __init add_static_vm_early(struct static_vm *svm)
> +{
> +	struct static_vm *curr_svm;
> +	struct vm_struct *vm;
> +	void *vaddr;
> +
> +	vm = &svm->vm;
> +	vm_area_add_early(vm);
> +	vaddr = vm->addr;
> +
> +	list_for_each_entry(curr_svm, &static_vmlist, list) {
> +		vm = &curr_svm->vm;
> +
> +		if (vm->addr > vaddr)
> +			break;
> +	}
> +	list_add_tail(&svm->list, &curr_svm->list);
> +}
> +
>  int ioremap_page(unsigned long virt, unsigned long phys,
>  		 const struct mem_type *mtype)
>  {
> diff --git a/arch/arm/mm/mm.h b/arch/arm/mm/mm.h
> index a8ee92d..d5a4e9a 100644
> --- a/arch/arm/mm/mm.h
> +++ b/arch/arm/mm/mm.h
> @@ -1,4 +1,6 @@
>  #ifdef CONFIG_MMU
> +#include <linux/list.h>
> +#include <linux/vmalloc.h>
>  
>  /* the upper-most page table pointer */
>  extern pmd_t *top_pmd;
> @@ -65,6 +67,16 @@ extern void __flush_dcache_page(struct address_space *mapping, struct page *page
>  /* consistent regions used by dma_alloc_attrs() */
>  #define VM_ARM_DMA_CONSISTENT	0x20000000
>  
> +
> +struct static_vm {
> +	struct vm_struct vm;
> +	struct list_head list;
> +};
> +
> +extern struct list_head static_vmlist;
> +extern struct static_vm *find_static_vm_vaddr(void *vaddr);
> +extern __init void add_static_vm_early(struct static_vm *svm);
> +
>  #endif
>  
>  #ifdef CONFIG_ZONE_DMA
> -- 
> 1.7.9.5
> 

Re: [PATCH v5 3/3] ARM: mm: use static_vm for managing static mapped areas

[Nicolas Pitre]

On Tue, 5 Feb 2013, Joonsoo Kim wrote:

> A static mapped area is ARM-specific, so it is better not to use
> generic vmalloc data structure, that is, vmlist and vmlist_lock
> for managing static mapped area. And it causes some needless overhead and
> reducing this overhead is better idea.
> 
> Now, we have newly introduced static_vm infrastructure.
> With it, we don't need to iterate all mapped areas. Instead, we just
> iterate static mapped areas. It helps to reduce an overhead of finding
> matched area. And architecture dependency on vmalloc layer is removed,
> so it will help to maintainability for vmalloc layer.
> 
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

Some comments below.

> diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
> index 904c15e..c7fef4b 100644
> --- a/arch/arm/mm/ioremap.c
> +++ b/arch/arm/mm/ioremap.c
> @@ -261,13 +261,14 @@ void __iomem * __arm_ioremap_pfn_caller(unsigned long pfn,
>  	const struct mem_type *type;
>  	int err;
>  	unsigned long addr;
> - 	struct vm_struct * area;
> +	struct vm_struct *area;
> +	phys_addr_t paddr = __pfn_to_phys(pfn);
>  
>  #ifndef CONFIG_ARM_LPAE
>  	/*
>  	 * High mappings must be supersection aligned
>  	 */
> -	if (pfn >= 0x100000 && (__pfn_to_phys(pfn) & ~SUPERSECTION_MASK))
> +	if (pfn >= 0x100000 && (paddr & ~SUPERSECTION_MASK))
>  		return NULL;
>  #endif
>  
> @@ -283,24 +284,16 @@ void __iomem * __arm_ioremap_pfn_caller(unsigned long pfn,
>  	/*
>  	 * Try to reuse one of the static mapping whenever possible.
>  	 */
> -	read_lock(&vmlist_lock);
> -	for (area = vmlist; area; area = area->next) {
> -		if (!size || (sizeof(phys_addr_t) == 4 && pfn >= 0x100000))
> -			break;
> -		if (!(area->flags & VM_ARM_STATIC_MAPPING))
> -			continue;
> -		if ((area->flags & VM_ARM_MTYPE_MASK) != VM_ARM_MTYPE(mtype))
> -			continue;
> -		if (__phys_to_pfn(area->phys_addr) > pfn ||
> -		    __pfn_to_phys(pfn) + size-1 > area->phys_addr + area->size-1)
> -			continue;
> -		/* we can drop the lock here as we know *area is static */
> -		read_unlock(&vmlist_lock);
> -		addr = (unsigned long)area->addr;
> -		addr += __pfn_to_phys(pfn) - area->phys_addr;
> -		return (void __iomem *) (offset + addr);
> +	if (size && !((sizeof(phys_addr_t) == 4 && pfn >= 0x100000))) {
                     ^                                             ^
You have a needless extra set of parents here.

[...]

> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> index ce328c7..b2c0356 100644
> --- a/arch/arm/mm/mmu.c
> +++ b/arch/arm/mm/mmu.c
> @@ -757,21 +757,24 @@ void __init iotable_init(struct map_desc *io_desc, int nr)
>  {
>  	struct map_desc *md;
>  	struct vm_struct *vm;
> +	struct static_vm *svm;
>  
>  	if (!nr)
>  		return;
>  
> -	vm = early_alloc_aligned(sizeof(*vm) * nr, __alignof__(*vm));
> +	svm = early_alloc_aligned(sizeof(*svm) * nr, __alignof__(*svm));
>  
>  	for (md = io_desc; nr; md++, nr--) {
>  		create_mapping(md);
> +
> +		vm = &svm->vm;
>  		vm->addr = (void *)(md->virtual & PAGE_MASK);
>  		vm->size = PAGE_ALIGN(md->length + (md->virtual & ~PAGE_MASK));
>  		vm->phys_addr = __pfn_to_phys(md->pfn);
>  		vm->flags = VM_IOREMAP | VM_ARM_STATIC_MAPPING;
>  		vm->flags |= VM_ARM_MTYPE(md->type);
>  		vm->caller = iotable_init;
> -		vm_area_add_early(vm++);
> +		add_static_vm_early(svm++);
>  	}
>  }
>  
> @@ -779,13 +782,16 @@ void __init vm_reserve_area_early(unsigned long addr, unsigned long size,
>  				  void *caller)
>  {
>  	struct vm_struct *vm;
> +	struct static_vm *svm;
> +
> +	svm = early_alloc_aligned(sizeof(*svm), __alignof__(*svm));
>  
> -	vm = early_alloc_aligned(sizeof(*vm), __alignof__(*vm));
> +	vm = &svm->vm;
>  	vm->addr = (void *)addr;
>  	vm->size = size;
>  	vm->flags = VM_IOREMAP | VM_ARM_EMPTY_MAPPING;
>  	vm->caller = caller;
> -	vm_area_add_early(vm);
> +	add_static_vm_early(svm);
>  }
>  
>  #ifndef CONFIG_ARM_LPAE
> @@ -810,14 +816,13 @@ static void __init pmd_empty_section_gap(unsigned long addr)
>  
>  static void __init fill_pmd_gaps(void)
>  {
> +	struct static_vm *svm;
>  	struct vm_struct *vm;
>  	unsigned long addr, next = 0;
>  	pmd_t *pmd;
>  
> -	/* we're still single threaded hence no lock needed here */
> -	for (vm = vmlist; vm; vm = vm->next) {
> -		if (!(vm->flags & (VM_ARM_STATIC_MAPPING | VM_ARM_EMPTY_MAPPING)))
> -			continue;
> +	list_for_each_entry(svm, &static_vmlist, list) {
> +		vm = &svm->vm;
>  		addr = (unsigned long)vm->addr;
>  		if (addr < next)
>  			continue;
> @@ -859,17 +864,12 @@ static void __init pci_reserve_io(void)
>  {
>  	struct vm_struct *vm;
>  	unsigned long addr;
> +	struct static_vm *svm;
>  
> -	/* we're still single threaded hence no lock needed here */
> -	for (vm = vmlist; vm; vm = vm->next) {
> -		if (!(vm->flags & VM_ARM_STATIC_MAPPING))
> -			continue;
> -		addr = (unsigned long)vm->addr;
> -		addr &= ~(SZ_2M - 1);
> -		if (addr == PCI_IO_VIRT_BASE)
> -			return;
> +	svm = find_static_vm_vaddr((void *)PCI_IO_VIRT_BASE);
> +	if (svm)
> +		return;
> 
> -	}
>  
>  	vm_reserve_area_early(PCI_IO_VIRT_BASE, SZ_2M, pci_reserve_io);
>  }

The replacement code is not equivalent.  I can't recall why the original 
is as it is, but it doesn't look right to me.  The 2MB round down 
certainly looks suspicious.

The replacement code should be better.  However I'd like you to get an 
ACK from Rob Herring as well for this patch.

Once that is sorted out, you can add

Reviewed-by: Nicolas Pitre <nico@linaro.org>


Nicolas

Re: [PATCH v5 1/3] ARM: vmregion: remove vmregion code entirely

[Santosh Shilimkar]

On Tuesday 05 February 2013 06:01 AM, Joonsoo Kim wrote:
> Now, there is no user for vmregion.
> So remove it.
>
> Acked-by: Nicolas Pitre <nico@linaro.org>
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>
> diff --git a/arch/arm/mm/Makefile b/arch/arm/mm/Makefile
> index 8a9c4cb..4e333fa 100644
> --- a/arch/arm/mm/Makefile
> +++ b/arch/arm/mm/Makefile
> @@ -6,7 +6,7 @@ obj-y				:= dma-mapping.o extable.o fault.o init.o \
>   				   iomap.o
>
>   obj-$(CONFIG_MMU)		+= fault-armv.o flush.o idmap.o ioremap.o \
> -				   mmap.o pgd.o mmu.o vmregion.o
> +				   mmap.o pgd.o mmu.o
>
>   ifneq ($(CONFIG_MMU),y)
>   obj-y				+= nommu.o
> diff --git a/arch/arm/mm/vmregion.c b/arch/arm/mm/vmregion.c
> deleted file mode 100644
> index a631016..0000000
> --- a/arch/arm/mm/vmregion.c
> +++ /dev/null
> @@ -1,205 +0,0 @@
You might want to use 'git format-patch -D'
which will just generate one line for a deleted file.

Regards,
Santosh

Re: [PATCH v5 0/3] introduce static_vm for ARM-specific static mapped area

[Santosh Shilimkar]

On Tuesday 05 February 2013 06:01 AM, Joonsoo Kim wrote:
> In current implementation, we used ARM-specific flag, that is,
> VM_ARM_STATIC_MAPPING, for distinguishing ARM specific static mapped area.
> The purpose of static mapped area is to re-use static mapped area when
> entire physical address range of the ioremap request can be covered
> by this area.
>
> This implementation causes needless overhead for some cases.
> For example, assume that there is only one static mapped area and
> vmlist has 300 areas. Every time we call ioremap, we check 300 areas for
> deciding whether it is matched or not. Moreover, even if there is
> no static mapped area and vmlist has 300 areas, every time we call
> ioremap, we check 300 areas in now.
>
> If we construct a extra list for static mapped area, we can eliminate
> above mentioned overhead.
> With a extra list, if there is one static mapped area,
> we just check only one area and proceed next operation quickly.
>
> In fact, it is not a critical problem, because ioremap is not frequently
> used. But reducing overhead is better idea.
>
> Another reason for doing this work is for removing vm_struct list management,
> entirely. For more information, look at the following link.
> http://lkml.org/lkml/2012/12/6/184
>

[..]

>
> Joonsoo Kim (3):
>    ARM: vmregion: remove vmregion code entirely
>    ARM: ioremap: introduce an infrastructure for static mapped area
>    ARM: mm: use static_vm for managing static mapped areas
>
>   arch/arm/mm/Makefile   |    2 +-
>   arch/arm/mm/ioremap.c  |  135 +++++++++++++++++++++----------
>   arch/arm/mm/mm.h       |   12 +++
>   arch/arm/mm/mmu.c      |   34 ++++----
>   arch/arm/mm/vmregion.c |  205 ------------------------------------------------
>   arch/arm/mm/vmregion.h |   31 --------
>   6 files changed, 123 insertions(+), 296 deletions(-)
>   delete mode 100644 arch/arm/mm/vmregion.c
>   delete mode 100644 arch/arm/mm/vmregion.h
>
Nice Clean-up. I tested this series on OMAP which uses few static
mappings. Feel free to add,

Tested-by: Santosh Shilimkar<santosh.shilimkar@ti.com>

Re: [PATCH v5 3/3] ARM: mm: use static_vm for managing static mapped areas

[Rob Herring]

On 02/04/2013 10:44 PM, Nicolas Pitre wrote:
> On Tue, 5 Feb 2013, Joonsoo Kim wrote:
> 
>> A static mapped area is ARM-specific, so it is better not to use
>> generic vmalloc data structure, that is, vmlist and vmlist_lock
>> for managing static mapped area. And it causes some needless overhead and
>> reducing this overhead is better idea.
>>
>> Now, we have newly introduced static_vm infrastructure.
>> With it, we don't need to iterate all mapped areas. Instead, we just
>> iterate static mapped areas. It helps to reduce an overhead of finding
>> matched area. And architecture dependency on vmalloc layer is removed,
>> so it will help to maintainability for vmalloc layer.
>>
>> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

[snip]

>> @@ -859,17 +864,12 @@ static void __init pci_reserve_io(void)
>>  {
>>  	struct vm_struct *vm;
>>  	unsigned long addr;
>> +	struct static_vm *svm;
>>  
>> -	/* we're still single threaded hence no lock needed here */
>> -	for (vm = vmlist; vm; vm = vm->next) {
>> -		if (!(vm->flags & VM_ARM_STATIC_MAPPING))
>> -			continue;
>> -		addr = (unsigned long)vm->addr;
>> -		addr &= ~(SZ_2M - 1);
>> -		if (addr == PCI_IO_VIRT_BASE)
>> -			return;
>> +	svm = find_static_vm_vaddr((void *)PCI_IO_VIRT_BASE);
>> +	if (svm)
>> +		return;
>>
>> -	}
>>  
>>  	vm_reserve_area_early(PCI_IO_VIRT_BASE, SZ_2M, pci_reserve_io);
>>  }
> 
> The replacement code is not equivalent.  I can't recall why the original 
> is as it is, but it doesn't look right to me.  The 2MB round down 
> certainly looks suspicious.

The PCI mapping is at a fixed, aligned 2MB mapping. If we find any
virtual address within that region already mapped, it is an error. We
probably should have had a WARN here.

> 
> The replacement code should be better.  However I'd like you to get an 
> ACK from Rob Herring as well for this patch.

It doesn't appear to me the above case is handled. The virt addr is
checked whether it is within an existing mapping, but not whether the
new mapping would overlap an existing mapping. It would be good to check
for this generically rather than specifically for the PCI i/o mapping.

Rob

Re: [PATCH v5 3/3] ARM: mm: use static_vm for managing static mapped areas

[Nicolas Pitre]

On Tue, 5 Feb 2013, Rob Herring wrote:

> On 02/04/2013 10:44 PM, Nicolas Pitre wrote:
> > On Tue, 5 Feb 2013, Joonsoo Kim wrote:
> > 
> >> A static mapped area is ARM-specific, so it is better not to use
> >> generic vmalloc data structure, that is, vmlist and vmlist_lock
> >> for managing static mapped area. And it causes some needless overhead and
> >> reducing this overhead is better idea.
> >>
> >> Now, we have newly introduced static_vm infrastructure.
> >> With it, we don't need to iterate all mapped areas. Instead, we just
> >> iterate static mapped areas. It helps to reduce an overhead of finding
> >> matched area. And architecture dependency on vmalloc layer is removed,
> >> so it will help to maintainability for vmalloc layer.
> >>
> >> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> 
> [snip]
> 
> >> @@ -859,17 +864,12 @@ static void __init pci_reserve_io(void)
> >>  {
> >>  	struct vm_struct *vm;
> >>  	unsigned long addr;
> >> +	struct static_vm *svm;
> >>  
> >> -	/* we're still single threaded hence no lock needed here */
> >> -	for (vm = vmlist; vm; vm = vm->next) {
> >> -		if (!(vm->flags & VM_ARM_STATIC_MAPPING))
> >> -			continue;
> >> -		addr = (unsigned long)vm->addr;
> >> -		addr &= ~(SZ_2M - 1);
> >> -		if (addr == PCI_IO_VIRT_BASE)
> >> -			return;
> >> +	svm = find_static_vm_vaddr((void *)PCI_IO_VIRT_BASE);
> >> +	if (svm)
> >> +		return;
> >>
> >> -	}
> >>  
> >>  	vm_reserve_area_early(PCI_IO_VIRT_BASE, SZ_2M, pci_reserve_io);
> >>  }
> > 
> > The replacement code is not equivalent.  I can't recall why the original 
> > is as it is, but it doesn't look right to me.  The 2MB round down 
> > certainly looks suspicious.
> 
> The PCI mapping is at a fixed, aligned 2MB mapping. If we find any
> virtual address within that region already mapped, it is an error.

Ah, OK.  This wasn't clear looking at the code.

> We probably should have had a WARN here.

Indeed.

> > 
> > The replacement code should be better.  However I'd like you to get an 
> > ACK from Rob Herring as well for this patch.
> 
> It doesn't appear to me the above case is handled. The virt addr is
> checked whether it is within an existing mapping, but not whether the
> new mapping would overlap an existing mapping. It would be good to check
> for this generically rather than specifically for the PCI i/o mapping.

Agreed.  However that is checked already in vm_area_add_early().  
Therefore the overlap test here is redundant.


Nicolas

Re: [PATCH v5 3/3] ARM: mm: use static_vm for managing static mapped areas

[Rob Herring]

On 02/05/2013 12:13 PM, Nicolas Pitre wrote:
> On Tue, 5 Feb 2013, Rob Herring wrote:
> 
>> On 02/04/2013 10:44 PM, Nicolas Pitre wrote:
>>> On Tue, 5 Feb 2013, Joonsoo Kim wrote:
>>>
>>>> A static mapped area is ARM-specific, so it is better not to use
>>>> generic vmalloc data structure, that is, vmlist and vmlist_lock
>>>> for managing static mapped area. And it causes some needless overhead and
>>>> reducing this overhead is better idea.
>>>>
>>>> Now, we have newly introduced static_vm infrastructure.
>>>> With it, we don't need to iterate all mapped areas. Instead, we just
>>>> iterate static mapped areas. It helps to reduce an overhead of finding
>>>> matched area. And architecture dependency on vmalloc layer is removed,
>>>> so it will help to maintainability for vmalloc layer.
>>>>
>>>> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>>
>> [snip]
>>
>>>> @@ -859,17 +864,12 @@ static void __init pci_reserve_io(void)
>>>>  {
>>>>  	struct vm_struct *vm;
>>>>  	unsigned long addr;
>>>> +	struct static_vm *svm;
>>>>  
>>>> -	/* we're still single threaded hence no lock needed here */
>>>> -	for (vm = vmlist; vm; vm = vm->next) {
>>>> -		if (!(vm->flags & VM_ARM_STATIC_MAPPING))
>>>> -			continue;
>>>> -		addr = (unsigned long)vm->addr;
>>>> -		addr &= ~(SZ_2M - 1);
>>>> -		if (addr == PCI_IO_VIRT_BASE)
>>>> -			return;
>>>> +	svm = find_static_vm_vaddr((void *)PCI_IO_VIRT_BASE);
>>>> +	if (svm)
>>>> +		return;
>>>>
>>>> -	}
>>>>  
>>>>  	vm_reserve_area_early(PCI_IO_VIRT_BASE, SZ_2M, pci_reserve_io);
>>>>  }
>>>
>>> The replacement code is not equivalent.  I can't recall why the original 
>>> is as it is, but it doesn't look right to me.  The 2MB round down 
>>> certainly looks suspicious.
>>
>> The PCI mapping is at a fixed, aligned 2MB mapping. If we find any
>> virtual address within that region already mapped, it is an error.
> 
> Ah, OK.  This wasn't clear looking at the code.
> 
>> We probably should have had a WARN here.
> 
> Indeed.
> 
>>>
>>> The replacement code should be better.  However I'd like you to get an 
>>> ACK from Rob Herring as well for this patch.
>>
>> It doesn't appear to me the above case is handled. The virt addr is
>> checked whether it is within an existing mapping, but not whether the
>> new mapping would overlap an existing mapping. It would be good to check
>> for this generically rather than specifically for the PCI i/o mapping.
> 
> Agreed.  However that is checked already in vm_area_add_early().  
> Therefore the overlap test here is redundant.

Ah, right. In that case:

Acked-by: Rob Herring <rob.herring@calxeda.com>

Rob

Re: [PATCH v5 3/3] ARM: mm: use static_vm for managing static mapped areas

[Joonsoo Kim]

Hello, Nicolas.

On Mon, Feb 04, 2013 at 11:44:16PM -0500, Nicolas Pitre wrote:
> On Tue, 5 Feb 2013, Joonsoo Kim wrote:
> 
> > A static mapped area is ARM-specific, so it is better not to use
> > generic vmalloc data structure, that is, vmlist and vmlist_lock
> > for managing static mapped area. And it causes some needless overhead and
> > reducing this overhead is better idea.
> > 
> > Now, we have newly introduced static_vm infrastructure.
> > With it, we don't need to iterate all mapped areas. Instead, we just
> > iterate static mapped areas. It helps to reduce an overhead of finding
> > matched area. And architecture dependency on vmalloc layer is removed,
> > so it will help to maintainability for vmalloc layer.
> > 
> > Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> 
> Some comments below.
> 
> > diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
> > index 904c15e..c7fef4b 100644
> > --- a/arch/arm/mm/ioremap.c
> > +++ b/arch/arm/mm/ioremap.c
> > @@ -261,13 +261,14 @@ void __iomem * __arm_ioremap_pfn_caller(unsigned long pfn,
> >  	const struct mem_type *type;
> >  	int err;
> >  	unsigned long addr;
> > - 	struct vm_struct * area;
> > +	struct vm_struct *area;
> > +	phys_addr_t paddr = __pfn_to_phys(pfn);
> >  
> >  #ifndef CONFIG_ARM_LPAE
> >  	/*
> >  	 * High mappings must be supersection aligned
> >  	 */
> > -	if (pfn >= 0x100000 && (__pfn_to_phys(pfn) & ~SUPERSECTION_MASK))
> > +	if (pfn >= 0x100000 && (paddr & ~SUPERSECTION_MASK))
> >  		return NULL;
> >  #endif
> >  
> > @@ -283,24 +284,16 @@ void __iomem * __arm_ioremap_pfn_caller(unsigned long pfn,
> >  	/*
> >  	 * Try to reuse one of the static mapping whenever possible.
> >  	 */
> > -	read_lock(&vmlist_lock);
> > -	for (area = vmlist; area; area = area->next) {
> > -		if (!size || (sizeof(phys_addr_t) == 4 && pfn >= 0x100000))
> > -			break;
> > -		if (!(area->flags & VM_ARM_STATIC_MAPPING))
> > -			continue;
> > -		if ((area->flags & VM_ARM_MTYPE_MASK) != VM_ARM_MTYPE(mtype))
> > -			continue;
> > -		if (__phys_to_pfn(area->phys_addr) > pfn ||
> > -		    __pfn_to_phys(pfn) + size-1 > area->phys_addr + area->size-1)
> > -			continue;
> > -		/* we can drop the lock here as we know *area is static */
> > -		read_unlock(&vmlist_lock);
> > -		addr = (unsigned long)area->addr;
> > -		addr += __pfn_to_phys(pfn) - area->phys_addr;
> > -		return (void __iomem *) (offset + addr);
> > +	if (size && !((sizeof(phys_addr_t) == 4 && pfn >= 0x100000))) {
>                      ^                                             ^
> You have a needless extra set of parents here.
 
Okay.

> [...]
> 
> > diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> > index ce328c7..b2c0356 100644
> > --- a/arch/arm/mm/mmu.c
> > +++ b/arch/arm/mm/mmu.c
> > @@ -757,21 +757,24 @@ void __init iotable_init(struct map_desc *io_desc, int nr)
> >  {
> >  	struct map_desc *md;
> >  	struct vm_struct *vm;
> > +	struct static_vm *svm;
> >  
> >  	if (!nr)
> >  		return;
> >  
> > -	vm = early_alloc_aligned(sizeof(*vm) * nr, __alignof__(*vm));
> > +	svm = early_alloc_aligned(sizeof(*svm) * nr, __alignof__(*svm));
> >  
> >  	for (md = io_desc; nr; md++, nr--) {
> >  		create_mapping(md);
> > +
> > +		vm = &svm->vm;
> >  		vm->addr = (void *)(md->virtual & PAGE_MASK);
> >  		vm->size = PAGE_ALIGN(md->length + (md->virtual & ~PAGE_MASK));
> >  		vm->phys_addr = __pfn_to_phys(md->pfn);
> >  		vm->flags = VM_IOREMAP | VM_ARM_STATIC_MAPPING;
> >  		vm->flags |= VM_ARM_MTYPE(md->type);
> >  		vm->caller = iotable_init;
> > -		vm_area_add_early(vm++);
> > +		add_static_vm_early(svm++);
> >  	}
> >  }
> >  
> > @@ -779,13 +782,16 @@ void __init vm_reserve_area_early(unsigned long addr, unsigned long size,
> >  				  void *caller)
> >  {
> >  	struct vm_struct *vm;
> > +	struct static_vm *svm;
> > +
> > +	svm = early_alloc_aligned(sizeof(*svm), __alignof__(*svm));
> >  
> > -	vm = early_alloc_aligned(sizeof(*vm), __alignof__(*vm));
> > +	vm = &svm->vm;
> >  	vm->addr = (void *)addr;
> >  	vm->size = size;
> >  	vm->flags = VM_IOREMAP | VM_ARM_EMPTY_MAPPING;
> >  	vm->caller = caller;
> > -	vm_area_add_early(vm);
> > +	add_static_vm_early(svm);
> >  }
> >  
> >  #ifndef CONFIG_ARM_LPAE
> > @@ -810,14 +816,13 @@ static void __init pmd_empty_section_gap(unsigned long addr)
> >  
> >  static void __init fill_pmd_gaps(void)
> >  {
> > +	struct static_vm *svm;
> >  	struct vm_struct *vm;
> >  	unsigned long addr, next = 0;
> >  	pmd_t *pmd;
> >  
> > -	/* we're still single threaded hence no lock needed here */
> > -	for (vm = vmlist; vm; vm = vm->next) {
> > -		if (!(vm->flags & (VM_ARM_STATIC_MAPPING | VM_ARM_EMPTY_MAPPING)))
> > -			continue;
> > +	list_for_each_entry(svm, &static_vmlist, list) {
> > +		vm = &svm->vm;
> >  		addr = (unsigned long)vm->addr;
> >  		if (addr < next)
> >  			continue;
> > @@ -859,17 +864,12 @@ static void __init pci_reserve_io(void)
> >  {
> >  	struct vm_struct *vm;
> >  	unsigned long addr;
> > +	struct static_vm *svm;
> >  
> > -	/* we're still single threaded hence no lock needed here */
> > -	for (vm = vmlist; vm; vm = vm->next) {
> > -		if (!(vm->flags & VM_ARM_STATIC_MAPPING))
> > -			continue;
> > -		addr = (unsigned long)vm->addr;
> > -		addr &= ~(SZ_2M - 1);
> > -		if (addr == PCI_IO_VIRT_BASE)
> > -			return;
> > +	svm = find_static_vm_vaddr((void *)PCI_IO_VIRT_BASE);
> > +	if (svm)
> > +		return;
> > 
> > -	}
> >  
> >  	vm_reserve_area_early(PCI_IO_VIRT_BASE, SZ_2M, pci_reserve_io);
> >  }
> 
> The replacement code is not equivalent.  I can't recall why the original 
> is as it is, but it doesn't look right to me.  The 2MB round down 
> certainly looks suspicious.
> 
> The replacement code should be better.  However I'd like you to get an 
> ACK from Rob Herring as well for this patch.
> 
> Once that is sorted out, you can add
> 
> Reviewed-by: Nicolas Pitre <nico@linaro.org>

Okay. I will fix this and re-send it with your "Reviewed-by".

Thanks.

> 
> Nicolas
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 3/3] ARM: mm: use static_vm for managing static mapped areas

[Joonsoo Kim]

Hello, Rob.

On Tue, Feb 05, 2013 at 01:12:51PM -0600, Rob Herring wrote:
> On 02/05/2013 12:13 PM, Nicolas Pitre wrote:
> > On Tue, 5 Feb 2013, Rob Herring wrote:
> > 
> >> On 02/04/2013 10:44 PM, Nicolas Pitre wrote:
> >>> On Tue, 5 Feb 2013, Joonsoo Kim wrote:
> >>>
> >>>> A static mapped area is ARM-specific, so it is better not to use
> >>>> generic vmalloc data structure, that is, vmlist and vmlist_lock
> >>>> for managing static mapped area. And it causes some needless overhead and
> >>>> reducing this overhead is better idea.
> >>>>
> >>>> Now, we have newly introduced static_vm infrastructure.
> >>>> With it, we don't need to iterate all mapped areas. Instead, we just
> >>>> iterate static mapped areas. It helps to reduce an overhead of finding
> >>>> matched area. And architecture dependency on vmalloc layer is removed,
> >>>> so it will help to maintainability for vmalloc layer.
> >>>>
> >>>> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> >>
> >> [snip]
> >>
> >>>> @@ -859,17 +864,12 @@ static void __init pci_reserve_io(void)
> >>>>  {
> >>>>  	struct vm_struct *vm;
> >>>>  	unsigned long addr;
> >>>> +	struct static_vm *svm;
> >>>>  
> >>>> -	/* we're still single threaded hence no lock needed here */
> >>>> -	for (vm = vmlist; vm; vm = vm->next) {
> >>>> -		if (!(vm->flags & VM_ARM_STATIC_MAPPING))
> >>>> -			continue;
> >>>> -		addr = (unsigned long)vm->addr;
> >>>> -		addr &= ~(SZ_2M - 1);
> >>>> -		if (addr == PCI_IO_VIRT_BASE)
> >>>> -			return;
> >>>> +	svm = find_static_vm_vaddr((void *)PCI_IO_VIRT_BASE);
> >>>> +	if (svm)
> >>>> +		return;
> >>>>
> >>>> -	}
> >>>>  
> >>>>  	vm_reserve_area_early(PCI_IO_VIRT_BASE, SZ_2M, pci_reserve_io);
> >>>>  }
> >>>
> >>> The replacement code is not equivalent.  I can't recall why the original 
> >>> is as it is, but it doesn't look right to me.  The 2MB round down 
> >>> certainly looks suspicious.
> >>
> >> The PCI mapping is at a fixed, aligned 2MB mapping. If we find any
> >> virtual address within that region already mapped, it is an error.
> > Ah, OK.  This wasn't clear looking at the code.
> >> We probably should have had a WARN here.
> > 
> > Indeed.
> > 

Okay.
I should fix it to find any mapping within PCI reserved region.
But, I think that it is not an error.
Now, I see your original commit 'c2794437091a4fda72c4a4f3567dd728dcc0c3c9'
and find below message.

"Platforms which need early i/o mapping (e.g. for vga console) can call
pci_map_io_early in their .map_io function."

Therfore, for some platform, it is possible that there is a mapping within
PCI reserved range.

So, I will not add WARN here.

I will fix and re-send v6 with your ACK.

Thanks for review.

> >>>
> >>> The replacement code should be better.  However I'd like you to get an 
> >>> ACK from Rob Herring as well for this patch.
> >>
> >> It doesn't appear to me the above case is handled. The virt addr is
> >> checked whether it is within an existing mapping, but not whether the
> >> new mapping would overlap an existing mapping. It would be good to check
> >> for this generically rather than specifically for the PCI i/o mapping.
> > 
> > Agreed.  However that is checked already in vm_area_add_early().  
> > Therefore the overlap test here is redundant.
> 
> Ah, right. In that case:
> 
> Acked-by: Rob Herring <rob.herring@calxeda.com>
> 
> Rob
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 1/3] ARM: vmregion: remove vmregion code entirely

[Joonsoo Kim]

Hello, Santosh.

On Tue, Feb 05, 2013 at 02:22:39PM +0530, Santosh Shilimkar wrote:
> On Tuesday 05 February 2013 06:01 AM, Joonsoo Kim wrote:
> >Now, there is no user for vmregion.
> >So remove it.
> >
> >Acked-by: Nicolas Pitre <nico@linaro.org>
> >Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> >
> >diff --git a/arch/arm/mm/Makefile b/arch/arm/mm/Makefile
> >index 8a9c4cb..4e333fa 100644
> >--- a/arch/arm/mm/Makefile
> >+++ b/arch/arm/mm/Makefile
> >@@ -6,7 +6,7 @@ obj-y				:= dma-mapping.o extable.o fault.o init.o \
> >  				   iomap.o
> >
> >  obj-$(CONFIG_MMU)		+= fault-armv.o flush.o idmap.o ioremap.o \
> >-				   mmap.o pgd.o mmu.o vmregion.o
> >+				   mmap.o pgd.o mmu.o
> >
> >  ifneq ($(CONFIG_MMU),y)
> >  obj-y				+= nommu.o
> >diff --git a/arch/arm/mm/vmregion.c b/arch/arm/mm/vmregion.c
> >deleted file mode 100644
> >index a631016..0000000
> >--- a/arch/arm/mm/vmregion.c
> >+++ /dev/null
> >@@ -1,205 +0,0 @@
> You might want to use 'git format-patch -D'
> which will just generate one line for a deleted file.

Nice tip!
Thanks.

> Regards,
> Santosh
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 0/3] introduce static_vm for ARM-specific static mapped area

[Joonsoo Kim]

Hello, Santosh.

On Tue, Feb 05, 2013 at 02:32:06PM +0530, Santosh Shilimkar wrote:
> On Tuesday 05 February 2013 06:01 AM, Joonsoo Kim wrote:
> >In current implementation, we used ARM-specific flag, that is,
> >VM_ARM_STATIC_MAPPING, for distinguishing ARM specific static mapped area.
> >The purpose of static mapped area is to re-use static mapped area when
> >entire physical address range of the ioremap request can be covered
> >by this area.
> >
> >This implementation causes needless overhead for some cases.
> >For example, assume that there is only one static mapped area and
> >vmlist has 300 areas. Every time we call ioremap, we check 300 areas for
> >deciding whether it is matched or not. Moreover, even if there is
> >no static mapped area and vmlist has 300 areas, every time we call
> >ioremap, we check 300 areas in now.
> >
> >If we construct a extra list for static mapped area, we can eliminate
> >above mentioned overhead.
> >With a extra list, if there is one static mapped area,
> >we just check only one area and proceed next operation quickly.
> >
> >In fact, it is not a critical problem, because ioremap is not frequently
> >used. But reducing overhead is better idea.
> >
> >Another reason for doing this work is for removing vm_struct list management,
> >entirely. For more information, look at the following link.
> >http://lkml.org/lkml/2012/12/6/184
> >
> 
> [..]
> 
> >
> >Joonsoo Kim (3):
> >   ARM: vmregion: remove vmregion code entirely
> >   ARM: ioremap: introduce an infrastructure for static mapped area
> >   ARM: mm: use static_vm for managing static mapped areas
> >
> >  arch/arm/mm/Makefile   |    2 +-
> >  arch/arm/mm/ioremap.c  |  135 +++++++++++++++++++++----------
> >  arch/arm/mm/mm.h       |   12 +++
> >  arch/arm/mm/mmu.c      |   34 ++++----
> >  arch/arm/mm/vmregion.c |  205 ------------------------------------------------
> >  arch/arm/mm/vmregion.h |   31 --------
> >  6 files changed, 123 insertions(+), 296 deletions(-)
> >  delete mode 100644 arch/arm/mm/vmregion.c
> >  delete mode 100644 arch/arm/mm/vmregion.h
> >
> Nice Clean-up. I tested this series on OMAP which uses few static
> mappings. Feel free to add,
> 
> Tested-by: Santosh Shilimkar<santosh.shilimkar@ti.com>

I will re-send v6 with your Tested-by.

Thanks for testing this.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 3/3] ARM: mm: use static_vm for managing static mapped areas

[Joonsoo Kim]

On Wed, Feb 06, 2013 at 11:07:07AM +0900, Joonsoo Kim wrote:
> Hello, Rob.
> 
> On Tue, Feb 05, 2013 at 01:12:51PM -0600, Rob Herring wrote:
> > On 02/05/2013 12:13 PM, Nicolas Pitre wrote:
> > > On Tue, 5 Feb 2013, Rob Herring wrote:
> > > 
> > >> On 02/04/2013 10:44 PM, Nicolas Pitre wrote:
> > >>> On Tue, 5 Feb 2013, Joonsoo Kim wrote:
> > >>>
> > >>>> A static mapped area is ARM-specific, so it is better not to use
> > >>>> generic vmalloc data structure, that is, vmlist and vmlist_lock
> > >>>> for managing static mapped area. And it causes some needless overhead and
> > >>>> reducing this overhead is better idea.
> > >>>>
> > >>>> Now, we have newly introduced static_vm infrastructure.
> > >>>> With it, we don't need to iterate all mapped areas. Instead, we just
> > >>>> iterate static mapped areas. It helps to reduce an overhead of finding
> > >>>> matched area. And architecture dependency on vmalloc layer is removed,
> > >>>> so it will help to maintainability for vmalloc layer.
> > >>>>
> > >>>> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > >>
> > >> [snip]
> > >>
> > >>>> @@ -859,17 +864,12 @@ static void __init pci_reserve_io(void)
> > >>>>  {
> > >>>>  	struct vm_struct *vm;
> > >>>>  	unsigned long addr;
> > >>>> +	struct static_vm *svm;
> > >>>>  
> > >>>> -	/* we're still single threaded hence no lock needed here */
> > >>>> -	for (vm = vmlist; vm; vm = vm->next) {
> > >>>> -		if (!(vm->flags & VM_ARM_STATIC_MAPPING))
> > >>>> -			continue;
> > >>>> -		addr = (unsigned long)vm->addr;
> > >>>> -		addr &= ~(SZ_2M - 1);
> > >>>> -		if (addr == PCI_IO_VIRT_BASE)
> > >>>> -			return;
> > >>>> +	svm = find_static_vm_vaddr((void *)PCI_IO_VIRT_BASE);
> > >>>> +	if (svm)
> > >>>> +		return;
> > >>>>
> > >>>> -	}
> > >>>>  
> > >>>>  	vm_reserve_area_early(PCI_IO_VIRT_BASE, SZ_2M, pci_reserve_io);
> > >>>>  }
> > >>>
> > >>> The replacement code is not equivalent.  I can't recall why the original 
> > >>> is as it is, but it doesn't look right to me.  The 2MB round down 
> > >>> certainly looks suspicious.
> > >>
> > >> The PCI mapping is at a fixed, aligned 2MB mapping. If we find any
> > >> virtual address within that region already mapped, it is an error.
> > > Ah, OK.  This wasn't clear looking at the code.
> > >> We probably should have had a WARN here.
> > > 
> > > Indeed.
> > > 
> 
> Okay.
> I should fix it to find any mapping within PCI reserved region.

Ah...
Above comment is my mistake.

If there is a region already mapped within PCI reserved region and
it is not found by find_static_vm_vaddr(), vm_area_add_early() hit BUG_ON().
So, to leave find_static_vm_vaddr() is safe.

> But, I think that it is not an error.
> Now, I see your original commit 'c2794437091a4fda72c4a4f3567dd728dcc0c3c9'
> and find below message.
> 
> "Platforms which need early i/o mapping (e.g. for vga console) can call
> pci_map_io_early in their .map_io function."
> 
> Therfore, for some platform, it is possible that there is a mapping within
> PCI reserved range.
> 
> So, I will not add WARN here.
> 
> I will fix and re-send v6 with your ACK.
> 
> Thanks for review.
> 
> > >>>
> > >>> The replacement code should be better.  However I'd like you to get an 
> > >>> ACK from Rob Herring as well for this patch.
> > >>
> > >> It doesn't appear to me the above case is handled. The virt addr is
> > >> checked whether it is within an existing mapping, but not whether the
> > >> new mapping would overlap an existing mapping. It would be good to check
> > >> for this generically rather than specifically for the PCI i/o mapping.
> > > 
> > > Agreed.  However that is checked already in vm_area_add_early().  
> > > Therefore the overlap test here is redundant.
> > 
> > Ah, right. In that case:
> > 
> > Acked-by: Rob Herring <rob.herring@calxeda.com>
> > 
> > Rob
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 3/3] ARM: mm: use static_vm for managing static mapped areas

[Nicolas Pitre]

On Wed, 6 Feb 2013, Joonsoo Kim wrote:

> On Wed, Feb 06, 2013 at 11:07:07AM +0900, Joonsoo Kim wrote:
> > Hello, Rob.
> > 
> > On Tue, Feb 05, 2013 at 01:12:51PM -0600, Rob Herring wrote:
> > > On 02/05/2013 12:13 PM, Nicolas Pitre wrote:
> > > > On Tue, 5 Feb 2013, Rob Herring wrote:
> > > > 
> > > >> On 02/04/2013 10:44 PM, Nicolas Pitre wrote:
> > > >>> On Tue, 5 Feb 2013, Joonsoo Kim wrote:
> > > >>>
> > > >>>> A static mapped area is ARM-specific, so it is better not to use
> > > >>>> generic vmalloc data structure, that is, vmlist and vmlist_lock
> > > >>>> for managing static mapped area. And it causes some needless overhead and
> > > >>>> reducing this overhead is better idea.
> > > >>>>
> > > >>>> Now, we have newly introduced static_vm infrastructure.
> > > >>>> With it, we don't need to iterate all mapped areas. Instead, we just
> > > >>>> iterate static mapped areas. It helps to reduce an overhead of finding
> > > >>>> matched area. And architecture dependency on vmalloc layer is removed,
> > > >>>> so it will help to maintainability for vmalloc layer.
> > > >>>>
> > > >>>> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > > >>
> > > >> [snip]
> > > >>
> > > >>>> @@ -859,17 +864,12 @@ static void __init pci_reserve_io(void)
> > > >>>>  {
> > > >>>>  	struct vm_struct *vm;
> > > >>>>  	unsigned long addr;
> > > >>>> +	struct static_vm *svm;
> > > >>>>  
> > > >>>> -	/* we're still single threaded hence no lock needed here */
> > > >>>> -	for (vm = vmlist; vm; vm = vm->next) {
> > > >>>> -		if (!(vm->flags & VM_ARM_STATIC_MAPPING))
> > > >>>> -			continue;
> > > >>>> -		addr = (unsigned long)vm->addr;
> > > >>>> -		addr &= ~(SZ_2M - 1);
> > > >>>> -		if (addr == PCI_IO_VIRT_BASE)
> > > >>>> -			return;
> > > >>>> +	svm = find_static_vm_vaddr((void *)PCI_IO_VIRT_BASE);
> > > >>>> +	if (svm)
> > > >>>> +		return;
> > > >>>>
> > > >>>> -	}
> > > >>>>  
> > > >>>>  	vm_reserve_area_early(PCI_IO_VIRT_BASE, SZ_2M, pci_reserve_io);
> > > >>>>  }
> > > >>>
> > > >>> The replacement code is not equivalent.  I can't recall why the original 
> > > >>> is as it is, but it doesn't look right to me.  The 2MB round down 
> > > >>> certainly looks suspicious.
> > > >>
> > > >> The PCI mapping is at a fixed, aligned 2MB mapping. If we find any
> > > >> virtual address within that region already mapped, it is an error.
> > > > Ah, OK.  This wasn't clear looking at the code.
> > > >> We probably should have had a WARN here.
> > > > 
> > > > Indeed.
> > > > 
> > 
> > Okay.
> > I should fix it to find any mapping within PCI reserved region.
> 
> Ah...
> Above comment is my mistake.
> 
> If there is a region already mapped within PCI reserved region and
> it is not found by find_static_vm_vaddr(), vm_area_add_early() hit BUG_ON().
> So, to leave find_static_vm_vaddr() is safe.

Yes.  In conclusion, your patch was fine.  You may remove the redundant 
parents and send the whole set to Russell.


Nicolas