Messages in this thread | | | Date | Fri, 1 Feb 2013 23:54:02 +0900 | Subject | Re: [PATCH v4 2/3] ARM: ioremap: introduce an infrastructure for static mapped area | From | JoonSoo Kim <> |
| |
Hello, Nicolas.
2013/2/1 Nicolas Pitre <nicolas.pitre@linaro.org>: > On Thu, 31 Jan 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> > > Much better. Comments below.
Thanks.
>> diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c >> index 88fd86c..ceb34ae 100644 >> --- a/arch/arm/mm/ioremap.c >> +++ b/arch/arm/mm/ioremap.c >> @@ -39,6 +39,78 @@ >> #include <asm/mach/pci.h> >> #include "mm.h" >> >> + >> +LIST_HEAD(static_vmlist); >> +static DEFINE_RWLOCK(static_vmlist_lock); > > In fact you don't need a lock at all. The only writer is > add_static_vm_early() and we know it is only used during boot when the > kernel is still single-threaded.
Yes!
>> + >> +static struct static_vm *find_static_vm_paddr(phys_addr_t paddr, >> + size_t size, unsigned long flags) >> +{ >> + struct static_vm *svm; >> + struct vm_struct *vm; >> + >> + read_lock(&static_vmlist_lock); >> + list_for_each_entry(svm, &static_vmlist, list) { >> + if (svm->flags != flags) >> + continue; >> + >> + vm = &svm->vm; >> + if (vm->phys_addr > paddr || >> + paddr + size - 1 > vm->phys_addr + vm->size - 1) >> + continue; >> + >> + read_unlock(&static_vmlist_lock); >> + return svm; >> + } >> + >> + return NULL; >> +} >> + >> +struct static_vm *find_static_vm_vaddr(void *vaddr) >> +{ >> + struct static_vm *svm; >> + struct vm_struct *vm; >> + >> + read_lock(&static_vmlist_lock); >> + 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) { >> + read_unlock(&static_vmlist_lock); >> + return svm; >> + } >> + } >> + read_unlock(&static_vmlist_lock); >> + >> + return NULL; >> +} >> + >> +void add_static_vm_early(struct static_vm *svm, unsigned long flags) > > This should be marked with __init. This way, it is less likely to be > used after boot, especially with no locking. And vm_area_add_early() is > valid only if !vmap_initialized anyway, and also __init.
Okay.
>> +{ >> + struct static_vm *curr_svm; >> + struct vm_struct *vm; >> + void *vaddr; >> + >> + vm_area_add_early(&svm->vm); >> + >> + vaddr = svm->vm.addr; >> + svm->flags = flags; >> + >> + write_lock(&static_vmlist_lock); >> + 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); >> + write_unlock(&static_vmlist_lock); >> +} >> + >> 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..fb45c79 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,24 @@ 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 >> >> + >> +/* ARM specific static_vm->flags bits */ >> +#define STATIC_VM_MEM 0x00000001 >> +#define STATIC_VM_EMPTY 0x00000002 >> +#define STATIC_VM_MTYPE(mtype) ((mtype) << 20) >> + >> +#define STATIC_VM_TYPE(type, mtype) (type | STATIC_VM_MTYPE(mtype)) >> + >> +struct static_vm { >> + struct vm_struct vm; >> + struct list_head list; >> + unsigned long flags; >> +}; > > What is your motivation for having separate flags instead of simply > keeping the current vm->flags usage? >
Keeping the current vm->flags is better idea. I will re-work about all your comments.
Thanks.
| |