Date:	Thu, 31 Jan 2013 22:05:45 -0500 (EST)
From:	Nicolas Pitre <nicolas.pitre@...aro.org>
To:	Joonsoo Kim <iamjoonsoo.kim@....com>
cc:	Russell King <linux@....linux.org.uk>,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	js1304@...il.com, Will Deacon <will.deacon@....com>,
	Catalin Marinas <catalin.marinas@....com>
Subject: Re: [PATCH v4 2/3] ARM: ioremap: introduce an infrastructure for
 static mapped area

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@....com>

Much better.  Comments below.

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

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

> +{
> +	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?


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

Powered by blists - more mailing lists