linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/3] introduce static_vm for ARM-specific static mapped area
@ 2013-01-24  1:28 Joonsoo Kim
  2013-01-24  1:28 ` [PATCH v3 1/3] ARM: vmregion: remove vmregion code entirely Joonsoo Kim
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Joonsoo Kim @ 2013-01-24  1:28 UTC (permalink / raw)
  To: Russell King
  Cc: linux-arm-kernel, linux-kernel, js1304, Will Deacon,
	Nicolas Pitre, Catalin Marinas, 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
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: static_vm: introduce an infrastructure for static mapped area
  ARM: mm: use static_vm for managing static mapped areas

 arch/arm/include/asm/mach/static_vm.h |   51 ++++++++
 arch/arm/mm/Makefile                  |    2 +-
 arch/arm/mm/ioremap.c                 |   69 ++++-------
 arch/arm/mm/mm.h                      |   10 --
 arch/arm/mm/mmu.c                     |   54 +++++----
 arch/arm/mm/static_vm.c               |   94 +++++++++++++++
 arch/arm/mm/vmregion.c                |  205 ---------------------------------
 arch/arm/mm/vmregion.h                |   31 -----
 8 files changed, 204 insertions(+), 312 deletions(-)
 create mode 100644 arch/arm/include/asm/mach/static_vm.h
 create mode 100644 arch/arm/mm/static_vm.c
 delete mode 100644 arch/arm/mm/vmregion.c
 delete mode 100644 arch/arm/mm/vmregion.h

-- 
1.7.9.5


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v3 1/3] ARM: vmregion: remove vmregion code entirely
  2013-01-24  1:28 [PATCH v3 0/3] introduce static_vm for ARM-specific static mapped area Joonsoo Kim
@ 2013-01-24  1:28 ` Joonsoo Kim
  2013-01-29 23:36   ` Nicolas Pitre
  2013-01-24  1:28 ` [PATCH v3 2/3] ARM: static_vm: introduce an infrastructure for static mapped area Joonsoo Kim
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 12+ messages in thread
From: Joonsoo Kim @ 2013-01-24  1:28 UTC (permalink / raw)
  To: Russell King
  Cc: linux-arm-kernel, linux-kernel, js1304, Will Deacon,
	Nicolas Pitre, Catalin Marinas, Joonsoo Kim

From: Joonsoo Kim <js1304@gmail.com>

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

Signed-off-by: Joonsoo Kim <js1304@gmail.com>
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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v3 2/3] ARM: static_vm: introduce an infrastructure for static mapped area
  2013-01-24  1:28 [PATCH v3 0/3] introduce static_vm for ARM-specific static mapped area Joonsoo Kim
  2013-01-24  1:28 ` [PATCH v3 1/3] ARM: vmregion: remove vmregion code entirely Joonsoo Kim
@ 2013-01-24  1:28 ` Joonsoo Kim
  2013-01-30  0:05   ` Nicolas Pitre
  2013-01-24  1:28 ` [PATCH v3 3/3] ARM: mm: use static_vm for managing static mapped areas Joonsoo Kim
  2013-01-28 17:51 ` [PATCH v3 0/3] introduce static_vm for ARM-specific static mapped area Will Deacon
  3 siblings, 1 reply; 12+ messages in thread
From: Joonsoo Kim @ 2013-01-24  1:28 UTC (permalink / raw)
  To: Russell King
  Cc: linux-arm-kernel, linux-kernel, js1304, Will Deacon,
	Nicolas Pitre, Catalin Marinas, Joonsoo Kim

From: Joonsoo Kim <js1304@gmail.com>

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 <js1304@gmail.com>
Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

diff --git a/arch/arm/include/asm/mach/static_vm.h b/arch/arm/include/asm/mach/static_vm.h
new file mode 100644
index 0000000..72c8339
--- /dev/null
+++ b/arch/arm/include/asm/mach/static_vm.h
@@ -0,0 +1,45 @@
+/*
+ * arch/arm/include/asm/mach/static_vm.h
+ *
+ * Copyright (C) 2012 LG Electronics, Joonsoo Kim <iamjoonsoo.kim@lge.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#ifndef _ASM_MACH_STATIC_VM_H
+#define _ASM_MACH_STATIC_VM_H
+
+#include <linux/types.h>
+#include <linux/vmalloc.h>
+
+struct static_vm {
+	struct static_vm	*next;
+	void			*vaddr;
+	unsigned long		size;
+	unsigned long		flags;
+	phys_addr_t		paddr;
+	const void		*caller;
+};
+
+extern struct static_vm *static_vmlist;
+extern spinlock_t static_vmlist_lock;
+
+extern struct static_vm *find_static_vm_paddr(phys_addr_t paddr,
+			size_t size, unsigned long flags);
+extern struct static_vm *find_static_vm_vaddr(void *vaddr);
+extern void init_static_vm(struct static_vm *static_vm,
+			struct vm_struct *vm, unsigned long flags);
+extern void insert_static_vm(struct static_vm *vm);
+
+#endif /* _ASM_MACH_STATIC_VM_H */
diff --git a/arch/arm/mm/Makefile b/arch/arm/mm/Makefile
index 4e333fa..57b329a 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
+				   mmap.o pgd.o mmu.o static_vm.o
 
 ifneq ($(CONFIG_MMU),y)
 obj-y				+= nommu.o
diff --git a/arch/arm/mm/static_vm.c b/arch/arm/mm/static_vm.c
new file mode 100644
index 0000000..265d8e9
--- /dev/null
+++ b/arch/arm/mm/static_vm.c
@@ -0,0 +1,94 @@
+/*
+ * arch/arm/mm/static_vm.c
+ *
+ * Copyright (C) 2012 LG Electronics, Joonsoo Kim <iamjoonsoo.kim@lge.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#include <linux/spinlock.h>
+
+#include <asm/mach/static_vm.h>
+
+struct static_vm *static_vmlist;
+DEFINE_SPINLOCK(static_vmlist_lock);
+
+struct static_vm *find_static_vm_paddr(phys_addr_t paddr,
+			size_t size, unsigned long flags)
+{
+	struct static_vm *area;
+
+	spin_lock(&static_vmlist_lock);
+	for (area = static_vmlist; area; area = area->next) {
+		if ((area->flags & flags) != flags)
+			continue;
+
+		if (area->paddr > paddr ||
+			paddr + size - 1 > area->paddr + area->size - 1)
+			continue;
+
+		spin_unlock(&static_vmlist_lock);
+		return area;
+	}
+	spin_unlock(&static_vmlist_lock);
+
+	return NULL;
+}
+
+struct static_vm *find_static_vm_vaddr(void *vaddr)
+{
+	struct static_vm *area;
+
+	spin_lock(&static_vmlist_lock);
+	for (area = static_vmlist; area; area = area->next) {
+		/* static_vmlist is ascending order */
+		if (area->vaddr > vaddr)
+			break;
+
+		if (area->vaddr <= vaddr && area->vaddr + area->size > vaddr) {
+			spin_unlock(&static_vmlist_lock);
+			return area;
+		}
+	}
+	spin_unlock(&static_vmlist_lock);
+
+	return NULL;
+}
+
+void init_static_vm(struct static_vm *static_vm,
+				struct vm_struct *vm, unsigned long flags)
+{
+	static_vm->vaddr = vm->addr;
+	static_vm->size = vm->size;
+	static_vm->paddr = vm->phys_addr;
+	static_vm->caller = vm->caller;
+	static_vm->flags = flags;
+}
+
+void insert_static_vm(struct static_vm *vm)
+{
+	struct static_vm *tmp, **p;
+
+	spin_lock(&static_vmlist_lock);
+	for (p = &static_vmlist; (tmp = *p) != NULL; p = &tmp->next) {
+		if (tmp->vaddr >= vm->vaddr) {
+			BUG_ON(tmp->vaddr < vm->vaddr + vm->size);
+			break;
+		} else
+			BUG_ON(tmp->vaddr + tmp->size > vm->vaddr);
+	}
+	vm->next = *p;
+	*p = vm;
+	spin_unlock(&static_vmlist_lock);
+}
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v3 3/3] ARM: mm: use static_vm for managing static mapped areas
  2013-01-24  1:28 [PATCH v3 0/3] introduce static_vm for ARM-specific static mapped area Joonsoo Kim
  2013-01-24  1:28 ` [PATCH v3 1/3] ARM: vmregion: remove vmregion code entirely Joonsoo Kim
  2013-01-24  1:28 ` [PATCH v3 2/3] ARM: static_vm: introduce an infrastructure for static mapped area Joonsoo Kim
@ 2013-01-24  1:28 ` Joonsoo Kim
  2013-01-30  0:06   ` Nicolas Pitre
  2013-01-28 17:51 ` [PATCH v3 0/3] introduce static_vm for ARM-specific static mapped area Will Deacon
  3 siblings, 1 reply; 12+ messages in thread
From: Joonsoo Kim @ 2013-01-24  1:28 UTC (permalink / raw)
  To: Russell King
  Cc: linux-arm-kernel, linux-kernel, js1304, Will Deacon,
	Nicolas Pitre, Catalin Marinas, Joonsoo Kim

From: Joonsoo Kim <js1304@gmail.com>

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 <js1304@gmail.com>
Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

diff --git a/arch/arm/include/asm/mach/static_vm.h b/arch/arm/include/asm/mach/static_vm.h
index 72c8339..24672b1 100644
--- a/arch/arm/include/asm/mach/static_vm.h
+++ b/arch/arm/include/asm/mach/static_vm.h
@@ -32,6 +32,12 @@ struct static_vm {
 	const void		*caller;
 };
 
+#define STATIC_VM_MEM		0x00000001
+#define STATIC_VM_EMPTY		0x00000002
+
+/* mtype should be less than 28 */
+#define STATIC_VM_MTYPE(mt)	(1UL << ((mt) + 4))
+
 extern struct static_vm *static_vmlist;
 extern spinlock_t static_vmlist_lock;
 
diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
index 88fd86c..2c0d3a1 100644
--- a/arch/arm/mm/ioremap.c
+++ b/arch/arm/mm/ioremap.c
@@ -36,6 +36,7 @@
 #include <asm/system_info.h>
 
 #include <asm/mach/map.h>
+#include <asm/mach/static_vm.h>
 #include <asm/mach/pci.h>
 #include "mm.h"
 
@@ -197,7 +198,8 @@ 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
 	/*
@@ -219,24 +221,17 @@ 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 *static_vm;
+
+		static_vm = find_static_vm_paddr(__pfn_to_phys(pfn), size,
+				STATIC_VM_MEM | STATIC_VM_MTYPE(mtype));
+		if (static_vm) {
+			addr = (unsigned long)static_vm->vaddr;
+			addr += paddr - static_vm->paddr;
+			return (void __iomem *) (offset + addr);
+		}
 	}
-	read_unlock(&vmlist_lock);
 
 	/*
 	 * Don't allow RAM to be mapped - this causes problems with ARMv6+
@@ -248,7 +243,7 @@ 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 &&
@@ -346,34 +341,20 @@ __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;
-
-	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;
-		}
+	struct static_vm *static_vm;
+
+	static_vm = find_static_vm_vaddr(addr);
+	if (static_vm)
+		return;
+
 #if !defined(CONFIG_SMP) && !defined(CONFIG_ARM_LPAE)
-		/*
-		 * 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)) {
+	{
+		struct vm_struct *vm;
+		vm = find_vm_area(addr);
+		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/mm.h b/arch/arm/mm/mm.h
index a8ee92d..3ae75e5 100644
--- a/arch/arm/mm/mm.h
+++ b/arch/arm/mm/mm.h
@@ -52,16 +52,6 @@ extern void __flush_dcache_page(struct address_space *mapping, struct page *page
 /* (super)section-mapped I/O regions used by ioremap()/iounmap() */
 #define VM_ARM_SECTION_MAPPING	0x80000000
 
-/* permanent static mappings from iotable_init() */
-#define VM_ARM_STATIC_MAPPING	0x40000000
-
-/* empty mapping */
-#define VM_ARM_EMPTY_MAPPING	0x20000000
-
-/* mapping type (attributes) for permanent static mappings */
-#define VM_ARM_MTYPE(mt)		((mt) << 20)
-#define VM_ARM_MTYPE_MASK	(0x1f << 20)
-
 /* consistent regions used by dma_alloc_attrs() */
 #define VM_ARM_DMA_CONSISTENT	0x20000000
 
diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index 9f06102..b799b69 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -31,6 +31,7 @@
 
 #include <asm/mach/arch.h>
 #include <asm/mach/map.h>
+#include <asm/mach/static_vm.h>
 #include <asm/mach/pci.h>
 
 #include "mm.h"
@@ -757,21 +758,28 @@ void __init iotable_init(struct map_desc *io_desc, int nr)
 {
 	struct map_desc *md;
 	struct vm_struct *vm;
+	struct static_vm *static_vm;
 
 	if (!nr)
 		return;
 
 	vm = early_alloc_aligned(sizeof(*vm) * nr, __alignof__(*vm));
+	static_vm = early_alloc_aligned(sizeof(*static_vm) * nr,
+						__alignof__(*static_vm));
 
 	for (md = io_desc; nr; md++, nr--) {
 		create_mapping(md);
+
 		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->flags = VM_IOREMAP;
 		vm->caller = iotable_init;
+
+		init_static_vm(static_vm, vm, STATIC_VM_MEM |
+						STATIC_VM_MTYPE(md->type));
 		vm_area_add_early(vm++);
+		insert_static_vm(static_vm++);
 	}
 }
 
@@ -779,13 +787,20 @@ void __init vm_reserve_area_early(unsigned long addr, unsigned long size,
 				  void *caller)
 {
 	struct vm_struct *vm;
+	struct static_vm *static_vm;
 
 	vm = early_alloc_aligned(sizeof(*vm), __alignof__(*vm));
+	static_vm = early_alloc_aligned(sizeof(*static_vm),
+					__alignof__(*static_vm));
+
 	vm->addr = (void *)addr;
 	vm->size = size;
-	vm->flags = VM_IOREMAP | VM_ARM_EMPTY_MAPPING;
+	vm->flags = VM_IOREMAP;
 	vm->caller = caller;
+
+	init_static_vm(static_vm, vm, STATIC_VM_EMPTY);
 	vm_area_add_early(vm);
+	insert_static_vm(static_vm);
 }
 
 #ifndef CONFIG_ARM_LPAE
@@ -810,15 +825,19 @@ static void __init pmd_empty_section_gap(unsigned long addr)
 
 static void __init fill_pmd_gaps(void)
 {
-	struct vm_struct *vm;
+	struct static_vm *area;
 	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;
-		addr = (unsigned long)vm->addr;
+	/*
+	 * We should not take a lock here, because pmd_empty_section_gap()
+	 * invokes vm_reserve_area_early(), and then it call insert_static_vm()
+	 * which try to take a lock.
+	 * We're still single thread, so traverse whole list without a lock
+	 * is safe for now. And inserting new entry is also safe.
+	 */
+	for (area = static_vmlist; area; area = area->next) {
+		addr = (unsigned long)area->vaddr;
 		if (addr < next)
 			continue;
 
@@ -838,7 +857,7 @@ static void __init fill_pmd_gaps(void)
 		 * If so and the second section entry for this PMD is empty
 		 * then we block the corresponding virtual address.
 		 */
-		addr += vm->size;
+		addr += area->size;
 		if ((addr & ~PMD_MASK) == SECTION_SIZE) {
 			pmd = pmd_off_k(addr) + 1;
 			if (pmd_none(*pmd))
@@ -857,19 +876,12 @@ static void __init fill_pmd_gaps(void)
 #if defined(CONFIG_PCI) && !defined(CONFIG_NEED_MACH_IO_H)
 static void __init pci_reserve_io(void)
 {
-	struct vm_struct *vm;
-	unsigned long addr;
+	struct static_vm *static_vm;
 
-	/* 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;
+	static_vm = find_static_vm_vaddr((void *)PCI_IO_VIRT_BASE);
+	if (static_vm)
+		return;
 
-	}
 	vm_reserve_area_early(PCI_IO_VIRT_BASE, SZ_2M, pci_reserve_io);
 }
 #else
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 0/3] introduce static_vm for ARM-specific static mapped area
  2013-01-24  1:28 [PATCH v3 0/3] introduce static_vm for ARM-specific static mapped area Joonsoo Kim
                   ` (2 preceding siblings ...)
  2013-01-24  1:28 ` [PATCH v3 3/3] ARM: mm: use static_vm for managing static mapped areas Joonsoo Kim
@ 2013-01-28 17:51 ` Will Deacon
  2013-01-28 18:04   ` Nicolas Pitre
  3 siblings, 1 reply; 12+ messages in thread
From: Will Deacon @ 2013-01-28 17:51 UTC (permalink / raw)
  To: Joonsoo Kim
  Cc: Russell King, js1304, Nicolas Pitre, Catalin Marinas,
	linux-kernel, linux-arm-kernel

Hello,

On Thu, Jan 24, 2013 at 01:28:51AM +0000, 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

First patch looks good (removing the unused vmregion stuff) but I'm not so
sure about the rest of it. If you really care about ioremap performance,
perhaps it would be better to have a container struct around the vm_struct
for static mappings and then stick them in an augmented rbtree so you can
efficiently find the mapping encompassing a particular physical address?

Will

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 0/3] introduce static_vm for ARM-specific static mapped area
  2013-01-28 17:51 ` [PATCH v3 0/3] introduce static_vm for ARM-specific static mapped area Will Deacon
@ 2013-01-28 18:04   ` Nicolas Pitre
  2013-01-29  6:56     ` Joonsoo Kim
  0 siblings, 1 reply; 12+ messages in thread
From: Nicolas Pitre @ 2013-01-28 18:04 UTC (permalink / raw)
  To: Will Deacon
  Cc: Joonsoo Kim, Russell King, js1304, Catalin Marinas, linux-kernel,
	linux-arm-kernel

On Mon, 28 Jan 2013, Will Deacon wrote:

> Hello,
> 
> On Thu, Jan 24, 2013 at 01:28:51AM +0000, 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
> 
> First patch looks good (removing the unused vmregion stuff) but I'm not so
> sure about the rest of it. If you really care about ioremap performance,
> perhaps it would be better to have a container struct around the vm_struct
> for static mappings and then stick them in an augmented rbtree so you can
> efficiently find the mapping encompassing a particular physical address?

How can ioremap performance be a problem is the question I had since the 
beginning.

Firstly, ioremap is _not_ meant to be used in performance critical 
paths.

Secondly, there shouldn't be _that_ many entries on the vmlist such as 
300.  That sounds a bit excessive.

So please, can we discuss the reasons that motivated those patches in 
the first place?  Maybe that's where the actual problem is.


Nicolas

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 0/3] introduce static_vm for ARM-specific static mapped area
  2013-01-28 18:04   ` Nicolas Pitre
@ 2013-01-29  6:56     ` Joonsoo Kim
  2013-01-29 23:35       ` Nicolas Pitre
  0 siblings, 1 reply; 12+ messages in thread
From: Joonsoo Kim @ 2013-01-29  6:56 UTC (permalink / raw)
  To: Nicolas Pitre
  Cc: Will Deacon, Russell King, Catalin Marinas, linux-kernel,
	linux-arm-kernel

On Mon, Jan 28, 2013 at 01:04:24PM -0500, Nicolas Pitre wrote:
> On Mon, 28 Jan 2013, Will Deacon wrote:
> 
> > Hello,
> > 
> > On Thu, Jan 24, 2013 at 01:28:51AM +0000, 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
> > 
> > First patch looks good (removing the unused vmregion stuff) but I'm not so
> > sure about the rest of it. If you really care about ioremap performance,
> > perhaps it would be better to have a container struct around the vm_struct
> > for static mappings and then stick them in an augmented rbtree so you can
> > efficiently find the mapping encompassing a particular physical address?
> 
> How can ioremap performance be a problem is the question I had since the 
> beginning.
> 
> Firstly, ioremap is _not_ meant to be used in performance critical 
> paths.
> 
> Secondly, there shouldn't be _that_ many entries on the vmlist such as 
> 300.  That sounds a bit excessive.
> 
> So please, can we discuss the reasons that motivated those patches in 
> the first place?  Maybe that's where the actual problem is.

Hello, Wiil and Nicolas.
First of all, thanks for reviewing.

There is another reason for doing this work.
As mentioned above, I try to remove list management for vm_struct(vmlist),
entirely. For that purpose, removing architecture dependency against vmlist
is needed. Below link is for my RFC patch trying to remove list management
for vm_struct.

http://lkml.org/lkml/2012/12/6/184

Removing dependency for other architectures is rather trivial, but for ARM,
it is not trivial case. So I prepared this patchset.
My description makes you missleading possibly.
Sorry for this.

Answer for your other questions is below.

I know ioremap is _not_ meant to be used in performance critical paths, and
I mentioned it earlier.
"In fact, it is not a critical problem, because ioremap is not frequently used.
But reducing overhead is better idea."

And, there are many entries on the vmlist for my test devices(Android phone).
I saw more than 300 entries in former days, but today, I re-check it and
find 230~250 entries.

Thanks.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 0/3] introduce static_vm for ARM-specific static mapped area
  2013-01-29  6:56     ` Joonsoo Kim
@ 2013-01-29 23:35       ` Nicolas Pitre
  0 siblings, 0 replies; 12+ messages in thread
From: Nicolas Pitre @ 2013-01-29 23:35 UTC (permalink / raw)
  To: Joonsoo Kim
  Cc: Will Deacon, Russell King, Catalin Marinas, linux-kernel,
	linux-arm-kernel

On Tue, 29 Jan 2013, Joonsoo Kim wrote:

> On Mon, Jan 28, 2013 at 01:04:24PM -0500, Nicolas Pitre wrote:
> > On Mon, 28 Jan 2013, Will Deacon wrote:
> > 
> > > Hello,
> > > 
> > > On Thu, Jan 24, 2013 at 01:28:51AM +0000, 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
> > > 
> > > First patch looks good (removing the unused vmregion stuff) but I'm not so
> > > sure about the rest of it. If you really care about ioremap performance,
> > > perhaps it would be better to have a container struct around the vm_struct
> > > for static mappings and then stick them in an augmented rbtree so you can
> > > efficiently find the mapping encompassing a particular physical address?
> > 
> > How can ioremap performance be a problem is the question I had since the 
> > beginning.
> > 
> > Firstly, ioremap is _not_ meant to be used in performance critical 
> > paths.
> > 
> > Secondly, there shouldn't be _that_ many entries on the vmlist such as 
> > 300.  That sounds a bit excessive.
> > 
> > So please, can we discuss the reasons that motivated those patches in 
> > the first place?  Maybe that's where the actual problem is.
> 
> Hello, Wiil and Nicolas.
> First of all, thanks for reviewing.
> 
> There is another reason for doing this work.
> As mentioned above, I try to remove list management for vm_struct(vmlist),
> entirely. For that purpose, removing architecture dependency against vmlist
> is needed. Below link is for my RFC patch trying to remove list management
> for vm_struct.
> 
> http://lkml.org/lkml/2012/12/6/184

OK,  I get it now.

I do have comments on your patches.  I'll provide them as a reply to 
them.


Nicolas

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/3] ARM: vmregion: remove vmregion code entirely
  2013-01-24  1:28 ` [PATCH v3 1/3] ARM: vmregion: remove vmregion code entirely Joonsoo Kim
@ 2013-01-29 23:36   ` Nicolas Pitre
  0 siblings, 0 replies; 12+ messages in thread
From: Nicolas Pitre @ 2013-01-29 23:36 UTC (permalink / raw)
  To: Joonsoo Kim
  Cc: Russell King, linux-arm-kernel, linux-kernel, js1304,
	Will Deacon, Catalin Marinas

On Thu, 24 Jan 2013, Joonsoo Kim wrote:

> From: Joonsoo Kim <js1304@gmail.com>
> 
> Now, there is no user for vmregion.
> So remove it.
> 
> Signed-off-by: Joonsoo Kim <js1304@gmail.com>
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

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


> 
> 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
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 2/3] ARM: static_vm: introduce an infrastructure for static mapped area
  2013-01-24  1:28 ` [PATCH v3 2/3] ARM: static_vm: introduce an infrastructure for static mapped area Joonsoo Kim
@ 2013-01-30  0:05   ` Nicolas Pitre
  2013-01-30  7:36     ` Joonsoo Kim
  0 siblings, 1 reply; 12+ messages in thread
From: Nicolas Pitre @ 2013-01-30  0:05 UTC (permalink / raw)
  To: Joonsoo Kim
  Cc: Russell King, linux-arm-kernel, linux-kernel, js1304,
	Will Deacon, Catalin Marinas

On Thu, 24 Jan 2013, Joonsoo Kim wrote:

> From: Joonsoo Kim <js1304@gmail.com>
> 
> 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 <js1304@gmail.com>
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

First of all, I don't think you really need a new file with a global 
scope header file.  Given that this code is meant to be used only for 
ioremap optimization  on ARM, it is probably a better idea to simply put 
it all into arch/arm/mm/ioremap.c instead.  The only function that needs 
to be exported out of ioremap.c is insert_static_vm(), and only for the 
benefit of arch/arm/mm/mmu.c, therefore this function prototype may as 
well just be added to arch/arm/mm/mm.h.

More comments below.

> diff --git a/arch/arm/include/asm/mach/static_vm.h b/arch/arm/include/asm/mach/static_vm.h
> new file mode 100644
> index 0000000..72c8339
> --- /dev/null
> +++ b/arch/arm/include/asm/mach/static_vm.h
> @@ -0,0 +1,45 @@
> +/*
> + * arch/arm/include/asm/mach/static_vm.h
> + *
> + * Copyright (C) 2012 LG Electronics, Joonsoo Kim <iamjoonsoo.kim@lge.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + */
> +
> +#ifndef _ASM_MACH_STATIC_VM_H
> +#define _ASM_MACH_STATIC_VM_H
> +
> +#include <linux/types.h>
> +#include <linux/vmalloc.h>
> +
> +struct static_vm {
> +	struct static_vm	*next;
> +	void			*vaddr;
> +	unsigned long		size;
> +	unsigned long		flags;
> +	phys_addr_t		paddr;
> +	const void		*caller;
> +};

Here you're duplicating most of the vm_struct content for no obvious 
reasons.  Patch #3 even allocates both a vm_struct and a static_vm 
instance in parallel for each mapping.  Instead, you should consider 
something like this:

struct static_vm {
	struct static_vm *next;
	struct vm_struct vm;
};

This way, you only need to allocate one structure:

	struct static_vm *svm = early_alloc(...);
	...
	svm->vm.addr = addr;
	...
	vm_area_add_early(&svm->vm);
	insert_static_vm(svm);

And then, it would make sense for the insert_static_vm() to do the 
vm_area_add_early() call itself as well.

Maybe rename insert_static_vm() to static_vm_area_add_early() to better 
identify its purpose as well.  It shouldn't be used for any other 
purpose anyway.

> +
> +extern struct static_vm *static_vmlist;
> +extern spinlock_t static_vmlist_lock;

Your patch is providing the proper accessors to manipulate those.  They 
therefore should not be exported globally.

> +
> +extern struct static_vm *find_static_vm_paddr(phys_addr_t paddr,
> +			size_t size, unsigned long flags);
> +extern struct static_vm *find_static_vm_vaddr(void *vaddr);
> +extern void init_static_vm(struct static_vm *static_vm,
> +			struct vm_struct *vm, unsigned long flags);

Since those are only used in ioremap.c, and because I suggested their 
implementation be moved there as well, you shouldn't need prototype 
declarations anymore.  And init_static_vm() would be useless with my 
previous suggestions.


Nicolas

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 3/3] ARM: mm: use static_vm for managing static mapped areas
  2013-01-24  1:28 ` [PATCH v3 3/3] ARM: mm: use static_vm for managing static mapped areas Joonsoo Kim
@ 2013-01-30  0:06   ` Nicolas Pitre
  0 siblings, 0 replies; 12+ messages in thread
From: Nicolas Pitre @ 2013-01-30  0:06 UTC (permalink / raw)
  To: Joonsoo Kim
  Cc: Russell King, linux-arm-kernel, linux-kernel, js1304,
	Will Deacon, Catalin Marinas

On Thu, 24 Jan 2013, Joonsoo Kim wrote:

> From: Joonsoo Kim <js1304@gmail.com>
> 
> 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 <js1304@gmail.com>
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

Given my comments to patch #2, the content of this patch should be 
reworked accordingly.



> 
> diff --git a/arch/arm/include/asm/mach/static_vm.h b/arch/arm/include/asm/mach/static_vm.h
> index 72c8339..24672b1 100644
> --- a/arch/arm/include/asm/mach/static_vm.h
> +++ b/arch/arm/include/asm/mach/static_vm.h
> @@ -32,6 +32,12 @@ struct static_vm {
>  	const void		*caller;
>  };
>  
> +#define STATIC_VM_MEM		0x00000001
> +#define STATIC_VM_EMPTY		0x00000002
> +
> +/* mtype should be less than 28 */
> +#define STATIC_VM_MTYPE(mt)	(1UL << ((mt) + 4))
> +
>  extern struct static_vm *static_vmlist;
>  extern spinlock_t static_vmlist_lock;
>  
> diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
> index 88fd86c..2c0d3a1 100644
> --- a/arch/arm/mm/ioremap.c
> +++ b/arch/arm/mm/ioremap.c
> @@ -36,6 +36,7 @@
>  #include <asm/system_info.h>
>  
>  #include <asm/mach/map.h>
> +#include <asm/mach/static_vm.h>
>  #include <asm/mach/pci.h>
>  #include "mm.h"
>  
> @@ -197,7 +198,8 @@ 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
>  	/*
> @@ -219,24 +221,17 @@ 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 *static_vm;
> +
> +		static_vm = find_static_vm_paddr(__pfn_to_phys(pfn), size,
> +				STATIC_VM_MEM | STATIC_VM_MTYPE(mtype));
> +		if (static_vm) {
> +			addr = (unsigned long)static_vm->vaddr;
> +			addr += paddr - static_vm->paddr;
> +			return (void __iomem *) (offset + addr);
> +		}
>  	}
> -	read_unlock(&vmlist_lock);
>  
>  	/*
>  	 * Don't allow RAM to be mapped - this causes problems with ARMv6+
> @@ -248,7 +243,7 @@ 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 &&
> @@ -346,34 +341,20 @@ __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;
> -
> -	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;
> -		}
> +	struct static_vm *static_vm;
> +
> +	static_vm = find_static_vm_vaddr(addr);
> +	if (static_vm)
> +		return;
> +
>  #if !defined(CONFIG_SMP) && !defined(CONFIG_ARM_LPAE)
> -		/*
> -		 * 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)) {
> +	{
> +		struct vm_struct *vm;
> +		vm = find_vm_area(addr);
> +		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/mm.h b/arch/arm/mm/mm.h
> index a8ee92d..3ae75e5 100644
> --- a/arch/arm/mm/mm.h
> +++ b/arch/arm/mm/mm.h
> @@ -52,16 +52,6 @@ extern void __flush_dcache_page(struct address_space *mapping, struct page *page
>  /* (super)section-mapped I/O regions used by ioremap()/iounmap() */
>  #define VM_ARM_SECTION_MAPPING	0x80000000
>  
> -/* permanent static mappings from iotable_init() */
> -#define VM_ARM_STATIC_MAPPING	0x40000000
> -
> -/* empty mapping */
> -#define VM_ARM_EMPTY_MAPPING	0x20000000
> -
> -/* mapping type (attributes) for permanent static mappings */
> -#define VM_ARM_MTYPE(mt)		((mt) << 20)
> -#define VM_ARM_MTYPE_MASK	(0x1f << 20)
> -
>  /* consistent regions used by dma_alloc_attrs() */
>  #define VM_ARM_DMA_CONSISTENT	0x20000000
>  
> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> index 9f06102..b799b69 100644
> --- a/arch/arm/mm/mmu.c
> +++ b/arch/arm/mm/mmu.c
> @@ -31,6 +31,7 @@
>  
>  #include <asm/mach/arch.h>
>  #include <asm/mach/map.h>
> +#include <asm/mach/static_vm.h>
>  #include <asm/mach/pci.h>
>  
>  #include "mm.h"
> @@ -757,21 +758,28 @@ void __init iotable_init(struct map_desc *io_desc, int nr)
>  {
>  	struct map_desc *md;
>  	struct vm_struct *vm;
> +	struct static_vm *static_vm;
>  
>  	if (!nr)
>  		return;
>  
>  	vm = early_alloc_aligned(sizeof(*vm) * nr, __alignof__(*vm));
> +	static_vm = early_alloc_aligned(sizeof(*static_vm) * nr,
> +						__alignof__(*static_vm));
>  
>  	for (md = io_desc; nr; md++, nr--) {
>  		create_mapping(md);
> +
>  		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->flags = VM_IOREMAP;
>  		vm->caller = iotable_init;
> +
> +		init_static_vm(static_vm, vm, STATIC_VM_MEM |
> +						STATIC_VM_MTYPE(md->type));
>  		vm_area_add_early(vm++);
> +		insert_static_vm(static_vm++);
>  	}
>  }
>  
> @@ -779,13 +787,20 @@ void __init vm_reserve_area_early(unsigned long addr, unsigned long size,
>  				  void *caller)
>  {
>  	struct vm_struct *vm;
> +	struct static_vm *static_vm;
>  
>  	vm = early_alloc_aligned(sizeof(*vm), __alignof__(*vm));
> +	static_vm = early_alloc_aligned(sizeof(*static_vm),
> +					__alignof__(*static_vm));
> +
>  	vm->addr = (void *)addr;
>  	vm->size = size;
> -	vm->flags = VM_IOREMAP | VM_ARM_EMPTY_MAPPING;
> +	vm->flags = VM_IOREMAP;
>  	vm->caller = caller;
> +
> +	init_static_vm(static_vm, vm, STATIC_VM_EMPTY);
>  	vm_area_add_early(vm);
> +	insert_static_vm(static_vm);
>  }
>  
>  #ifndef CONFIG_ARM_LPAE
> @@ -810,15 +825,19 @@ static void __init pmd_empty_section_gap(unsigned long addr)
>  
>  static void __init fill_pmd_gaps(void)
>  {
> -	struct vm_struct *vm;
> +	struct static_vm *area;
>  	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;
> -		addr = (unsigned long)vm->addr;
> +	/*
> +	 * We should not take a lock here, because pmd_empty_section_gap()
> +	 * invokes vm_reserve_area_early(), and then it call insert_static_vm()
> +	 * which try to take a lock.
> +	 * We're still single thread, so traverse whole list without a lock
> +	 * is safe for now. And inserting new entry is also safe.
> +	 */
> +	for (area = static_vmlist; area; area = area->next) {
> +		addr = (unsigned long)area->vaddr;
>  		if (addr < next)
>  			continue;
>  
> @@ -838,7 +857,7 @@ static void __init fill_pmd_gaps(void)
>  		 * If so and the second section entry for this PMD is empty
>  		 * then we block the corresponding virtual address.
>  		 */
> -		addr += vm->size;
> +		addr += area->size;
>  		if ((addr & ~PMD_MASK) == SECTION_SIZE) {
>  			pmd = pmd_off_k(addr) + 1;
>  			if (pmd_none(*pmd))
> @@ -857,19 +876,12 @@ static void __init fill_pmd_gaps(void)
>  #if defined(CONFIG_PCI) && !defined(CONFIG_NEED_MACH_IO_H)
>  static void __init pci_reserve_io(void)
>  {
> -	struct vm_struct *vm;
> -	unsigned long addr;
> +	struct static_vm *static_vm;
>  
> -	/* 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;
> +	static_vm = find_static_vm_vaddr((void *)PCI_IO_VIRT_BASE);
> +	if (static_vm)
> +		return;
>  
> -	}
>  	vm_reserve_area_early(PCI_IO_VIRT_BASE, SZ_2M, pci_reserve_io);
>  }
>  #else
> -- 
> 1.7.9.5
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 2/3] ARM: static_vm: introduce an infrastructure for static mapped area
  2013-01-30  0:05   ` Nicolas Pitre
@ 2013-01-30  7:36     ` Joonsoo Kim
  0 siblings, 0 replies; 12+ messages in thread
From: Joonsoo Kim @ 2013-01-30  7:36 UTC (permalink / raw)
  To: Nicolas Pitre
  Cc: Russell King, linux-arm-kernel, linux-kernel, Will Deacon,
	Catalin Marinas

Hello, Nicolas.

On Tue, Jan 29, 2013 at 07:05:32PM -0500, Nicolas Pitre wrote:
> On Thu, 24 Jan 2013, Joonsoo Kim wrote:
> 
> > From: Joonsoo Kim <js1304@gmail.com>
> > 
> > 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 <js1304@gmail.com>
> > Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> 
> First of all, I don't think you really need a new file with a global 
> scope header file.  Given that this code is meant to be used only for 
> ioremap optimization  on ARM, it is probably a better idea to simply put 
> it all into arch/arm/mm/ioremap.c instead.  The only function that needs 
> to be exported out of ioremap.c is insert_static_vm(), and only for the 
> benefit of arch/arm/mm/mmu.c, therefore this function prototype may as 
> well just be added to arch/arm/mm/mm.h.

I agree with your all opinions.
I will re-work and will re-send v4 as soon as possible.
Thanks for review.

> More comments below.
> 
> > diff --git a/arch/arm/include/asm/mach/static_vm.h b/arch/arm/include/asm/mach/static_vm.h
> > new file mode 100644
> > index 0000000..72c8339
> > --- /dev/null
> > +++ b/arch/arm/include/asm/mach/static_vm.h
> > @@ -0,0 +1,45 @@
> > +/*
> > + * arch/arm/include/asm/mach/static_vm.h
> > + *
> > + * Copyright (C) 2012 LG Electronics, Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> > + */
> > +
> > +#ifndef _ASM_MACH_STATIC_VM_H
> > +#define _ASM_MACH_STATIC_VM_H
> > +
> > +#include <linux/types.h>
> > +#include <linux/vmalloc.h>
> > +
> > +struct static_vm {
> > +	struct static_vm	*next;
> > +	void			*vaddr;
> > +	unsigned long		size;
> > +	unsigned long		flags;
> > +	phys_addr_t		paddr;
> > +	const void		*caller;
> > +};
> 
> Here you're duplicating most of the vm_struct content for no obvious 
> reasons.  Patch #3 even allocates both a vm_struct and a static_vm 
> instance in parallel for each mapping.  Instead, you should consider 
> something like this:
> 
> struct static_vm {
> 	struct static_vm *next;
> 	struct vm_struct vm;
> };
> 
> This way, you only need to allocate one structure:
> 
> 	struct static_vm *svm = early_alloc(...);
> 	...
> 	svm->vm.addr = addr;
> 	...
> 	vm_area_add_early(&svm->vm);
> 	insert_static_vm(svm);

Yes!
It's good idea.

> And then, it would make sense for the insert_static_vm() to do the 
> vm_area_add_early() call itself as well.

Okay.

> Maybe rename insert_static_vm() to static_vm_area_add_early() to better 
> identify its purpose as well.  It shouldn't be used for any other 
> purpose anyway.

Okay.

> > +
> > +extern struct static_vm *static_vmlist;
> > +extern spinlock_t static_vmlist_lock;
> 
> Your patch is providing the proper accessors to manipulate those.  They 
> therefore should not be exported globally.

Okay.

> > +
> > +extern struct static_vm *find_static_vm_paddr(phys_addr_t paddr,
> > +			size_t size, unsigned long flags);
> > +extern struct static_vm *find_static_vm_vaddr(void *vaddr);
> > +extern void init_static_vm(struct static_vm *static_vm,
> > +			struct vm_struct *vm, unsigned long flags);
> 
> Since those are only used in ioremap.c, and because I suggested their 
> implementation be moved there as well, you shouldn't need prototype 
> declarations anymore.  And init_static_vm() would be useless with my 
> previous suggestions.

Yes.

> 
> 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/

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2013-01-30  7:36 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-01-24  1:28 [PATCH v3 0/3] introduce static_vm for ARM-specific static mapped area Joonsoo Kim
2013-01-24  1:28 ` [PATCH v3 1/3] ARM: vmregion: remove vmregion code entirely Joonsoo Kim
2013-01-29 23:36   ` Nicolas Pitre
2013-01-24  1:28 ` [PATCH v3 2/3] ARM: static_vm: introduce an infrastructure for static mapped area Joonsoo Kim
2013-01-30  0:05   ` Nicolas Pitre
2013-01-30  7:36     ` Joonsoo Kim
2013-01-24  1:28 ` [PATCH v3 3/3] ARM: mm: use static_vm for managing static mapped areas Joonsoo Kim
2013-01-30  0:06   ` Nicolas Pitre
2013-01-28 17:51 ` [PATCH v3 0/3] introduce static_vm for ARM-specific static mapped area Will Deacon
2013-01-28 18:04   ` Nicolas Pitre
2013-01-29  6:56     ` Joonsoo Kim
2013-01-29 23:35       ` Nicolas Pitre

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).