Add IO mapping space reused support
Subsystems (3)
v2 cover Add IO mapping space reused support
Changes in V2:
- Uses the atomic_t instead.
- Revises some comment message.
Richard Lee (2):
mm/vmalloc: Add IO mapping space reused interface support.
ARM: ioremap: Add IO mapping space reused support.
arch/arm/mm/ioremap.c | 6 ++++ include/linux/vmalloc.h | 5 +++ mm/vmalloc.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 93 insertions(+)
Signature · 1 line
1.8.4
PATCH v2 1/2 mm/vmalloc: Add IO mapping space reused interface support.
For the IO mapping, the same physical address space maybe mapped more than one time, for example, in some SoCs:
- 0x20001000 ~ 0x20001400 --> 1KB for Dev1
- 0x20001400 ~ 0x20001800 --> 1KB for Dev2
and the page size is 4KB.
Then both Dev1 and Dev2 will do ioremap operations, and the IO vmalloc area's virtual address will be aligned down to 4KB, and the size will be aligned up to 4KB. That's to say, only one 4KB size's vmalloc area could contain Dev1 and Dev2 IO mapping area at the same time.
For this case, we can ioremap only one time, and the later ioremap operation will just return the exist vmalloc area.
--- include/linux/vmalloc.h | 5 +++ mm/vmalloc.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+)
include/linux/vmalloc.h +5 -0
@@ -1,6 +1,7 @@#ifndef _LINUX_VMALLOC_H#define _LINUX_VMALLOC_H +#include <linux/atomic.h> #include <linux/spinlock.h> #include <linux/init.h> #include <linux/list.h>
@@ -34,6 +35,7 @@ struct vm_struct {struct page **pages;unsigned int nr_pages;phys_addr_t phys_addr; +atomic_t used;const void *caller; };
@@ -100,6 +102,9 @@ static inline size_t get_vm_area_size(const struct vm_struct *area)return area->size -PAGE_SIZE ; } +extern struct vm_struct *find_vm_area_paddr (phys_addr_t paddr,size_t size, +unsigned long *offset, +unsigned long flags);extern struct vm_struct *get_vm_area (unsigned long size,unsigned long flags);extern struct vm_struct *get_vm_area_caller (unsigned long size,unsigned long flags,const void *caller);
mm/vmalloc.c +82 -0
100 lines not on this page · raw message
Signature · 1 line
1.8.4
Re: [PATCHv2 1/2] mm/vmalloc: Add IO mapping space reused interface support.
Adding Nico...
Quoted 6 lines · Richard Lee
For the IO mapping, the same physical address space maybe mapped more than one time, for example, in some SoCs:
-0x20001000~0x20001400--> 1KB for Dev1
-0x20001400~0x20001800--> 1KB for Dev2
and the page size is 4KB.
Quoted 8 lines
Then both Dev1 and Dev2 will do ioremap operations, and the IO vmalloc area's virtual address will be aligned down to 4KB, and the size will be aligned up to 4KB. That's to say, only one 4KB size's vmalloc area could contain Dev1 and Dev2 IO mapping area at the same time.
For this case, we can ioremap only one time, and the later ioremap operation will just return the exist vmalloc area.
We already have similar reuse tracking for the static mappings on ARM. I'm wondering if either that can be reused for ioremap as well or this can replace the static mapping tracking. It seems silly to have 2 lists to search for finding an existing mapping.
But there is a fundamental problem with your approach. You do not and cannot check the memory type of the mapping. If you look at the static mapping code, it only reuses mappings if the memory type match. While having aliases with different memory types is bad on ARMv6+, I assume there are some cases that do that given the static mapping code handles that case. We would at least want to detect and warn if we didn't want to allow different attributes rather than just return a mapping with whatever attributes the first mapping used.
Rob
Re: [PATCHv2 1/2] mm/vmalloc: Add IO mapping space reused interface support.
Quoted 15 lines · Rob Herring
Adding Nico...
Quoted 12 lines · Richard Lee
For the IO mapping, the same physical address space maybe mapped more than one time, for example, in some SoCs:
-0x20001000~0x20001400--> 1KB for Dev1
-0x20001400~0x20001800--> 1KB for Dev2
and the page size is 4KB.Then both Dev1 and Dev2 will do ioremap operations, and the IO vmalloc area's virtual address will be aligned down to 4KB, and the size will be aligned up to 4KB. That's to say, only one 4KB size's vmalloc area could contain Dev1 and Dev2 IO mapping area at the same time.
Quoted 8 lines
Quoted 2 lines
For this case, we can ioremap only one time, and the later ioremap operation will just return the exist vmalloc area.
We already have similar reuse tracking for the static mappings on ARM. I'm wondering if either that can be reused for ioremap as well or this can replace the static mapping tracking. It seems silly to have 2 lists to search for finding an existing mapping.
Yes, it is.
Maybe there is one way to reuse the ARM's static mapping tracking, or one new global way to replace it.
Quoted 9 lines
But there is a fundamental problem with your approach. You do not and cannot check the memory type of the mapping. If you look at the static mapping code, it only reuses mappings if the memory type match. While having aliases with different memory types is bad on ARMv6+, I assume there are some cases that do that given the static mapping code handles that case. We would at least want to detect and warn if we didn't want to allow different attributes rather than just return a mapping with whatever attributes the first mapping used.
You are right, maybe an alternative is defining one call back for each individual
ARCH or other ways.
Thanks very much for you comments, they are very important for me.
BRs,
Richard Lee
Quoted 1 line
Rob
Andrew Morton
maintainerRe: [PATCHv2 1/2] mm/vmalloc: Add IO mapping space reused interface support.
In reply to Richard LeeQuoted 11 lines · On Wed, 14 May 2014 16:18:51 +0800 Richard Lee <[email protected]> wrote:
For the IO mapping, the same physical address space maybe mapped more than one time, for example, in some SoCs:
-0x20001000~0x20001400--> 1KB for Dev1
-0x20001400~0x20001800--> 1KB for Dev2
and the page size is 4KB.Then both Dev1 and Dev2 will do ioremap operations, and the IO vmalloc area's virtual address will be aligned down to 4KB, and the size will be aligned up to 4KB. That's to say, only one 4KB size's vmalloc area could contain Dev1 and Dev2 IO mapping area at the same time.
Unclear. What happens when a caller does the two ioremaps at present? It fails? Returns the current mapping's address? Something else?
Quoted 2 lines
For this case, we can ioremap only one time, and the later ioremap operation will just return the exist vmalloc area.
I guess an alternative is to establish a new vmap pointing at the same physical address. How does this approach compare to refcounting the existing vmap?
Quoted 41 lines
include/linux/vmalloc.h +5
-0@@ -1,6 +1,7 @@#ifndef _LINUX_VMALLOC_H#define _LINUX_VMALLOC_H +#include <linux/atomic.h> #include <linux/spinlock.h> #include <linux/init.h> #include <linux/list.h> @@ -34,6 +35,7 @@ struct vm_struct {struct page **pages;unsigned int nr_pages;phys_addr_t phys_addr; +atomic_t used;const void *caller; };@@ -100,6 +102,9 @@ static inline size_t get_vm_area_size(const struct vm_struct *area)return area->size -PAGE_SIZE ; } +extern struct vm_struct *find_vm_area_paddr (phys_addr_t paddr,size_t size, +unsigned long *offset, +unsigned long flags);extern struct vm_struct *get_vm_area (unsigned long size,unsigned long flags);extern struct vm_struct *get_vm_area_caller (unsigned long size,unsigned long flags,const void *caller);mm/vmalloc.c +1
-0@@ -1293,6 +1293,7 @@ static void setup_vmalloc_vm(struct vm_struct *vm, struct vmap_area *va, vm->addr = (void *)va->va_start; vm->size = va->va_end - va->va_start; vm->caller = caller; +atomic_set (&vm->used,1 ); va->vm = vm; va->flags |=VM_VM_AREA ;spin_unlock (&vmap_area_lock);
mm/vmalloc.c struct vm_struct *get_vm_area_caller(unsigned long size, unsigned long flags, · line 1390
Quoted 7 lines
mm/vmalloc.c +4
-0@@ -1383,6 +1384,84 @@ struct vm_struct *get_vm_area_caller(unsigned long size, unsigned long flags, NUMA_NO_NODE, GFP_KERNEL, caller); } +static int vm_area_used_inc (struct vm_struct *area) +{ +if (!(area->flags &VM_IOREMAP )) +return -EINVAL ;
afaict this can never happen?
mm/vmalloc.c struct vm_struct *get_vm_area_caller(unsigned long size, unsigned long flags, · line 1394
Quoted 3 lines
mm/vmalloc.c +3
-0+atomic_add (1 , &area->used); + +return atomic_read (&va->vm->used);
atomic_add_return() is neater. But the return value is in fact never used so it could return void.
Quoted 6 lines
mm/vmalloc.c +6
-0+} + +static int vm_area_used_dec (const void *addr) +{ +struct vmap_area *va; +
mm/vmalloc.c struct vm_struct *get_vm_area_caller(unsigned long size, unsigned long flags, · line 1410
Quoted 10 lines
mm/vmalloc.c +10
-0+ va =find_vmap_area ((unsigned long )addr); +if (!va || !(va->flags &VM_VM_AREA )) +return 0 ; + +if (!(va->vm->flags &VM_IOREMAP )) +return 0 ; + +atomic_sub (1 , &va->vm->used); + +return atomic_read (&va->vm->used);
atomic_sub_return()
Quoted 58 lines
mm/vmalloc.c +54
-0+} + +/** +* find_vm_area_paddr - find a continuous kernel virtual area using the +* physical addreess. +* @paddr: base physical address +* @size: size of the physical area range +* @offset: the start offset of the vm area +* @flags: %VM_IOREMAP for I/O mappings +* +* Search for the kernel VM area, whoes physical address starting at +* @paddr, and if the exsit VM area's size is large enough, then return +* it with increasing the 'used' counter, or return NULL. +*/ +struct vm_struct *find_vm_area_paddr (phys_addr_t paddr,size_t size, +unsigned long *offset, +unsigned long flags) +{ +struct vmap_area *va; +int off; + +if (!(flags &VM_IOREMAP )) +return NULL ; + + size =PAGE_ALIGN ((paddr & ~PAGE_MASK ) + size); + +rcu_read_lock (); +list_for_each_entry_rcu (va, &vmap_area_list, list) { +phys_addr_t phys_addr; + +if (!va || !(va->flags &VM_VM_AREA ) || !va->vm) +continue ; + +if (!(va->vm->flags &VM_IOREMAP )) +continue ; + + phys_addr = va->vm->phys_addr; + + off = (paddr &PAGE_MASK ) - (phys_addr &PAGE_MASK ); +if (off <0 ) +continue ; + +if (off + size <= va->vm->size -PAGE_SIZE ) { + *offset = off + (paddr & ~PAGE_MASK ); +vm_area_used_inc (va->vm); +rcu_read_unlock (); +return va->vm; + } + } +rcu_read_unlock (); + +return NULL ; +} +/** * find_vm_area - find a continuous kernel virtual area * @addr: base address
mm/vmalloc.c static void __vunmap(const void *addr, int deallocate_pages) · line 1526
Quoted 5 lines
mm/vmalloc.c +2
-0@@ -1443,6 +1522,9 @@ static void __vunmap(const void *addr, int deallocate_pages) addr))return ; +if (vm_area_used_dec (addr)) +return ;
This could do with a comment explaining why we return - ie, document the overall concept/design.
Also, what prevents races here? Some other thread comes in and grabs a new reference just after this thread has decided to nuke the vmap?
If there's locking here which I failed to notice then some code commentary which explains the locking rules would also be nice.
Quoted 3 lines
mm/vmalloc.c +0
-0area =remove_vm_area (addr);if (unlikely (!area)) { WARN(1, KERN_ERR "Trying to vfree() nonexistent vm area (%p)\n",
Re: [PATCHv2 1/2] mm/vmalloc: Add IO mapping space reused interface support.
Quoted 8 lines · Andrew Morton
Quoted 6 lines · On Wed, 14 May 2014 16:18:51 +0800 Richard Lee <[email protected]> wrote:
For the IO mapping, the same physical address space maybe mapped more than one time, for example, in some SoCs:
-0x20001000~0x20001400--> 1KB for Dev1
-0x20001400~0x20001800--> 1KB for Dev2
and the page size is 4KB.
Quoted 9 lines
Quoted 5 lines
Then both Dev1 and Dev2 will do ioremap operations, and the IO vmalloc area's virtual address will be aligned down to 4KB, and the size will be aligned up to 4KB. That's to say, only one 4KB size's vmalloc area could contain Dev1 and Dev2 IO mapping area at the same time.
Unclear. What happens when a caller does the two ioremaps at present? It fails? Returns the current mapping's address? Something else?
For this case, should the later one wait ?
Maybe this patch hasn't consider about this.
Quoted 7 lines
Quoted 2 lines
For this case, we can ioremap only one time, and the later ioremap operation will just return the exist vmalloc area.
I guess an alternative is to establish a new vmap pointing at the same physical address. How does this approach compare to refcounting the existing vmap?
Yes, I'm also thinking to estabish one new vmap.
Quoted 44 lines
Quoted 44 lines
include/linux/vmalloc.h +5
-0@@ -1,6 +1,7 @@#ifndef _LINUX_VMALLOC_H#define _LINUX_VMALLOC_H +#include <linux/atomic.h> #include <linux/spinlock.h> #include <linux/init.h> #include <linux/list.h> @@ -34,6 +35,7 @@ struct vm_struct {struct page **pages;unsigned int nr_pages;phys_addr_t phys_addr; +atomic_t used;const void *caller; };@@ -100,6 +102,9 @@ static inline size_t get_vm_area_size(const struct vm_struct *area)return area->size -PAGE_SIZE ; } +extern struct vm_struct *find_vm_area_paddr (phys_addr_t paddr,size_t size, +unsigned long *offset, +unsigned long flags);extern struct vm_struct *get_vm_area (unsigned long size,unsigned long flags);extern struct vm_struct *get_vm_area_caller (unsigned long size,unsigned long flags,const void *caller);mm/vmalloc.c +1
-0@@ -1293,6 +1293,7 @@ static void setup_vmalloc_vm(struct vm_struct *vm, struct vmap_area *va, vm->addr = (void *)va->va_start; vm->size = va->va_end - va->va_start; vm->caller = caller; +atomic_set (&vm->used,1 ); va->vm = vm; va->flags |=VM_VM_AREA ;spin_unlock (&vmap_area_lock);@@ -1383,6 +1384,84 @@ struct vm_struct *get_vm_area_caller(unsigned long size, unsigned long flags, NUMA_NO_NODE, GFP_KERNEL, caller); }
Quoted 7 lines
mm/vmalloc.c struct vm_struct *get_vm_area_caller(unsigned long size, unsigned long flags, · line 1390
Quoted 4 lines
mm/vmalloc.c +4
-0+static int vm_area_used_inc (struct vm_struct *area) +{ +if (!(area->flags &VM_IOREMAP )) +return -EINVAL ;afaict this can never happen?
Yes, it is for now.
Quoted 7 lines
mm/vmalloc.c struct vm_struct *get_vm_area_caller(unsigned long size, unsigned long flags, · line 1394
Quoted 3 lines
mm/vmalloc.c +3
-0+atomic_add (1 , &area->used); + +return atomic_read (&va->vm->used);
atomic_add_return()is neater. But the return value is in fact never used so it could return void.
yes, that' fine.
Quoted 10 lines
Quoted 10 lines
mm/vmalloc.c +10
-0+} + +static int vm_area_used_dec (const void *addr) +{ +struct vmap_area *va; + + va =find_vmap_area ((unsigned long )addr); +if (!va || !(va->flags &VM_VM_AREA )) +return 0 ; +
Quoted 9 lines
mm/vmalloc.c struct vm_struct *get_vm_area_caller(unsigned long size, unsigned long flags, · line 1410
Quoted 6 lines
mm/vmalloc.c +6
-0+if (!(va->vm->flags &VM_IOREMAP )) +return 0 ; + +atomic_sub (1 , &va->vm->used); + +return atomic_read (&va->vm->used);
atomic_sub_return()
yes,
Quoted 67 lines
Quoted 58 lines
mm/vmalloc.c +54
-0+} + +/** +* find_vm_area_paddr - find a continuous kernel virtual area using the +* physical addreess. +* @paddr: base physical address +* @size: size of the physical area range +* @offset: the start offset of the vm area +* @flags: %VM_IOREMAP for I/O mappings +* +* Search for the kernel VM area, whoes physical address starting at +* @paddr, and if the exsit VM area's size is large enough, then return +* it with increasing the 'used' counter, or return NULL. +*/ +struct vm_struct *find_vm_area_paddr (phys_addr_t paddr,size_t size, +unsigned long *offset, +unsigned long flags) +{ +struct vmap_area *va; +int off; + +if (!(flags &VM_IOREMAP )) +return NULL ; + + size =PAGE_ALIGN ((paddr & ~PAGE_MASK ) + size); + +rcu_read_lock (); +list_for_each_entry_rcu (va, &vmap_area_list, list) { +phys_addr_t phys_addr; + +if (!va || !(va->flags &VM_VM_AREA ) || !va->vm) +continue ; + +if (!(va->vm->flags &VM_IOREMAP )) +continue ; + + phys_addr = va->vm->phys_addr; + + off = (paddr &PAGE_MASK ) - (phys_addr &PAGE_MASK ); +if (off <0 ) +continue ; + +if (off + size <= va->vm->size -PAGE_SIZE ) { + *offset = off + (paddr & ~PAGE_MASK ); +vm_area_used_inc (va->vm); +rcu_read_unlock (); +return va->vm; + } + } +rcu_read_unlock (); + +return NULL ; +} +/** * find_vm_area - find a continuous kernel virtual area * @addr: base address mm/vmalloc.c static void __vunmap(const void *addr, int deallocate_pages) · line 1526
Quoted 5 lines
mm/vmalloc.c +2
-0@@ -1443,6 +1522,9 @@ static void __vunmap(const void *addr, int deallocate_pages) addr))return ; +if (vm_area_used_dec (addr)) +return ;This could do with a comment explaining why we return - ie, document the overall concept/design.
Quoted 6 lines
Also, what prevents races here? Some other thread comes in and grabs a new reference just after this thread has decided to nuke the vmap?
If there's locking here which I failed to notice then some code commentary which explains the locking rules would also be nice.
I will try to revise this.
Actually, I'm thinking about adding a new rb tree for the ioremap vmalloc area sorted by physical address ? Then this will be more efficient for searching.
Thanks very much for you comments.
BRs
Richard Lee
Quoted 4 lines
Quoted 3 lines
mm/vmalloc.c +0
-0area =remove_vm_area (addr);if (unlikely (!area)) { WARN(1, KERN_ERR "Trying to vfree() nonexistent vm area (%p)\n",
PATCH v2 2/2 ARM: ioremap: Add IO mapping space reused support.
In reply to Richard LeeFor the IO mapping, the same physical address space maybe mapped more than one time, for example, in some SoCs:
- 0x20001000 ~ 0x20001400 --> 1KB for Dev1
- 0x20001400 ~ 0x20001800 --> 1KB for Dev2
and the page size is 4KB.
Then both Dev1 and Dev2 will do ioremap operations, and the IO vmalloc area's virtual address will be aligned down to 4KB, and the size will be aligned up to 4KB. That's to say, only one 4KB size's vmalloc area could contain Dev1 and Dev2 IO mapping area at the same time.
For this case, we can ioremap only one time, and the later ioremap operation will just return the exist vmalloc area.
This patch add IO mapping space reused support.
--- arch/arm/mm/ioremap.c | 6 ++++++ 1 file changed, 6 insertions(+)
arch/arm/mm/ioremap.c +6 -0
@@ -301,6 +301,12 @@ void __iomem * __arm_ioremap_pfn_caller(unsigned long pfn,if (WARN_ON (pfn_valid (pfn)))return NULL ; + area =find_vm_area_paddr (paddr, size, &offset,VM_IOREMAP ); +if (area) { + addr = (unsigned long )area->addr; +return (void __iomem *)(offset + addr); + } + area =get_vm_area_caller (size,VM_IOREMAP , caller);if (!area)return NULL ;
Signature · 1 line
1.8.4