vmalloc: simplify vread()/vwrite()

RFC PATCH In review Wu Fengguang 3 participants 9 messages Jan 7, 2010

Subsystems (2)

Memory management vmalloc

lore

Share & tools

lore

Wu Fengguang

author

2010-01-07 01:25 UTC #0 lore raw

RFC PATCH 1/1 vmalloc: simplify vread()/vwrite()

vread()/vwrite() is only called from kcore/kmem to access one page at a time. So the logic can be vastly simplified.

The changes are:
- remove the vmlist walk and rely solely on vmalloc_to_page()
- replace the VM_IOREMAP check with (page && page_is_ram(pfn))

The VM_IOREMAP check is introduced in commit d0107eb07320b for per-cpu alloc. Kame, would you double check if this change is OK for that purpose?

The page_is_ram() check is necessary because kmap_atomic() is not designed to work with non-RAM pages.

Even for a RAM page, we don't own the page, and cannot assume it's a _PAGE_CACHE_WB page. So I wonder whether it's necessary to do another patch to call reserve_memtype() before kmap_atomic() to ensure cache consistency?

TODO: update comments accordingly

CC Tejun Heo <[email protected]>

CC Ingo Molnar <[email protected]>

CC Nick Piggin <[email protected]>

CC Andi Kleen <[email protected]>

CC Hugh Dickins <[email protected]>

CC Christoph Lameter <[email protected]>

CC KAMEZAWA Hiroyuki <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

---
 mm/vmalloc.c |  196 ++++++++++---------------------------------------
 1 file changed, 40 insertions(+), 156 deletions(-)

mm/vmalloc.c +40 ~~-156~~

@@ -1646,87 +1646,6 @@ void *vmalloc_32_user(unsigned long size
 }
 EXPORT_SYMBOL(vmalloc_32_user);
 
-/*
- * small helper routine , copy contents to buf from addr.
- * If the page is not present, fill zero.
- */
-
-static int aligned_vread(char *buf, char *addr, unsigned long count)
-{
-	struct page *p;
-	int copied = 0;
-
-	while (count) {
-		unsigned long offset, length;
-
-		offset = (unsigned long)addr & ~PAGE_MASK;
-		length = PAGE_SIZE - offset;
-		if (length > count)
-			length = count;
-		p = vmalloc_to_page(addr);
-		/*
-		 * To do safe access to this _mapped_ area, we need
-		 * lock. But adding lock here means that we need to add
-		 * overhead of vmalloc()/vfree() calles for this _debug_
-		 * interface, rarely used. Instead of that, we'll use
-		 * kmap() and get small overhead in this access function.
-		 */
-		if (p) {
-			/*
-			 * we can expect USER0 is not used (see vread/vwrite's
-			 * function description)
-			 */
-			void *map = kmap_atomic(p, KM_USER0);
-			memcpy(buf, map + offset, length);
-			kunmap_atomic(map, KM_USER0);
-		} else
-			memset(buf, 0, length);
-
-		addr += length;
-		buf += length;
-		copied += length;
-		count -= length;
-	}
-	return copied;
-}
-
-static int aligned_vwrite(char *buf, char *addr, unsigned long count)
-{
-	struct page *p;
-	int copied = 0;
-
-	while (count) {
-		unsigned long offset, length;
-
-		offset = (unsigned long)addr & ~PAGE_MASK;
-		length = PAGE_SIZE - offset;
-		if (length > count)
-			length = count;
-		p = vmalloc_to_page(addr);
-		/*
-		 * To do safe access to this _mapped_ area, we need
-		 * lock. But adding lock here means that we need to add
-		 * overhead of vmalloc()/vfree() calles for this _debug_
-		 * interface, rarely used. Instead of that, we'll use
-		 * kmap() and get small overhead in this access function.
-		 */
-		if (p) {
-			/*
-			 * we can expect USER0 is not used (see vread/vwrite's
-			 * function description)
-			 */
-			void *map = kmap_atomic(p, KM_USER0);
-			memcpy(map + offset, buf, length);
-			kunmap_atomic(map, KM_USER0);
-		}
-		addr += length;
-		buf += length;
-		copied += length;
-		count -= length;
-	}
-	return copied;
-}
-
 /**
  *	vread() -  read vmalloc area in a safe way.
  *	@buf:		buffer for reading data

@@ -1757,49 +1676,34 @@ static int aligned_vwrite(char *buf, cha
 
 long vread(char *buf, char *addr, unsigned long count)
 {
-	struct vm_struct *tmp;
-	char *vaddr, *buf_start = buf;
-	unsigned long buflen = count;
-	unsigned long n;
-
-	/* Don't allow overflow */
-	if ((unsigned long) addr + count < count)
-		count = -(unsigned long) addr;
+	struct page *p;
+	void *map;
+	int offset = (unsigned long)addr & (PAGE_SIZE - 1);
 
-	read_lock(&vmlist_lock);
-	for (tmp = vmlist; count && tmp; tmp = tmp->next) {
-		vaddr = (char *) tmp->addr;
-		if (addr >= vaddr + tmp->size - PAGE_SIZE)
-			continue;
-		while (addr < vaddr) {
-			if (count == 0)
-				goto finished;
-			*buf = '\0';
-			buf++;
-			addr++;
-			count--;
-		}
-		n = vaddr + tmp->size - PAGE_SIZE - addr;
-		if (n > count)
-			n = count;
-		if (!(tmp->flags & VM_IOREMAP))
-			aligned_vread(buf, addr, n);
-		else /* IOREMAP area is treated as memory hole */
-			memset(buf, 0, n);
-		buf += n;
-		addr += n;
-		count -= n;
-	}
-finished:
-	read_unlock(&vmlist_lock);
+	/* Assume subpage access */
+	BUG_ON(count > PAGE_SIZE - offset);
 
-	if (buf == buf_start)
+	p = vmalloc_to_page(addr);
+	if (!p || !page_is_ram(page_to_pfn(p))) {
+		memset(buf, 0, count);
 		return 0;
-	/* zero-fill memory holes */
-	if (buf != buf_start + buflen)
-		memset(buf, 0, buflen - (buf - buf_start));
+	}
 
-	return buflen;
+	/*
+	 * To do safe access to this _mapped_ area, we need
+	 * lock. But adding lock here means that we need to add
+	 * overhead of vmalloc()/vfree() calles for this _debug_
+	 * interface, rarely used. Instead of that, we'll use
+	 * kmap() and get small overhead in this access function.
+	 *
+	 * we can expect USER0 is not used (see vread/vwrite's
+	 * function description)
+	 */
+	map = kmap_atomic(p, KM_USER0);
+	memcpy(buf, map + offset, count);
+	kunmap_atomic(map, KM_USER0);
+
+	return count;
 }
 
 /**

@@ -1834,44 +1738,24 @@ finished:
 
 long vwrite(char *buf, char *addr, unsigned long count)
 {
-	struct vm_struct *tmp;
-	char *vaddr;
-	unsigned long n, buflen;
-	int copied = 0;
-
-	/* Don't allow overflow */
-	if ((unsigned long) addr + count < count)
-		count = -(unsigned long) addr;
-	buflen = count;
+	struct page *p;
+	void *map;
+	int offset = (unsigned long)addr & (PAGE_SIZE - 1);
 
-	read_lock(&vmlist_lock);
-	for (tmp = vmlist; count && tmp; tmp = tmp->next) {
-		vaddr = (char *) tmp->addr;
-		if (addr >= vaddr + tmp->size - PAGE_SIZE)
-			continue;
-		while (addr < vaddr) {
-			if (count == 0)
-				goto finished;
-			buf++;
-			addr++;
-			count--;
-		}
-		n = vaddr + tmp->size - PAGE_SIZE - addr;
-		if (n > count)
-			n = count;
-		if (!(tmp->flags & VM_IOREMAP)) {
-			aligned_vwrite(buf, addr, n);
-			copied++;
-		}
-		buf += n;
-		addr += n;
-		count -= n;
-	}
-finished:
-	read_unlock(&vmlist_lock);
-	if (!copied)
+	/* Assume subpage access */
+	BUG_ON(count > PAGE_SIZE - offset);
+
+	p = vmalloc_to_page(addr);
+	if (!p)
+		return 0;
+	if (!page_is_ram(page_to_pfn(p)))
 		return 0;
-	return buflen;
+
+	map = kmap_atomic(p, KM_USER0);
+	memcpy(map + offset, buf, count);
+	kunmap_atomic(map, KM_USER0);
+
+	return count;
 }
 
 /**

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

KAMEZAWA Hiroyuki

2010-01-07 01:41 UTC #1 lore raw

Quoted 3 lines · On Thu, 7 Jan 2010 09:24:59 +0800 Wu Fengguang <[email protected]> wrote:

vread()/vwrite() is only called from kcore/kmem to access one page at a time. So the logic can be vastly simplified.

I recommend you to rename the function because safety of function is changed and you can show what callers are influenced.

Quoted 8 lines

The changes are:
- remove the vmlist walk and rely solely on vmalloc_to_page()
- replace the VM_IOREMAP check with (page && page_is_ram(pfn))
The VM_IOREMAP check is introduced in commit d0107eb07320b for per-cpu alloc. Kame, would you double check if this change is OK for that purpose?

I think VM_IOREMAP is for avoiding access to device configuration area and unexpected breakage in device. Then, VM_IOREMAP are should be skipped by the caller. (My patch _just_ moves the avoidance of callers to vread()/vwrite())

Quoted 3 lines

The page_is_ram() check is necessary because kmap_atomic() is not designed to work with non-RAM pages.

I think page_is_ram() is not a complete method...on x86, it just check e820's memory range. checking VM_IOREMAP is better, I think.

Quoted 7 lines

Even for a RAM page, we don't own the page, and cannot assume it's a _PAGE_CACHE_WB page. So I wonder whether it's necessary to do another patch to call reserve_memtype() before kmap_atomic() to ensure cache consistency?
TODO: update comments accordingly

BTW, f->f_pos problem on 64bit machine still exists and this patch is still hard to test. I stopped that because anyone doesn't show any interests.

I have no objection to your direction.

but please rewrite the function explanation as
"addr" should be page alinged and bufsize should be multiple of page size." and change the function names.

Thanks,
-Kame

Quoted 228 lines

CC Tejun Heo <[email protected]>

CC Ingo Molnar <[email protected]>

CC Nick Piggin <[email protected]>

CC Andi Kleen <[email protected]>

CC Hugh Dickins <[email protected]>

CC Christoph Lameter <[email protected]>

CC KAMEZAWA Hiroyuki <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

---
 mm/vmalloc.c |  196 ++++++++++---------------------------------------
 1 file changed, 40 insertions(+), 156 deletions(-)

mm/vmalloc.c +34 ~~-155~~

@@ -1646,87 +1646,6 @@ void *vmalloc_32_user(unsigned long size
 }
 EXPORT_SYMBOL(vmalloc_32_user);
 
-/*
- * small helper routine , copy contents to buf from addr.
- * If the page is not present, fill zero.
- */
-
-static int aligned_vread(char *buf, char *addr, unsigned long count)
-{
-	struct page *p;
-	int copied = 0;
-
-	while (count) {
-		unsigned long offset, length;
-
-		offset = (unsigned long)addr & ~PAGE_MASK;
-		length = PAGE_SIZE - offset;
-		if (length > count)
-			length = count;
-		p = vmalloc_to_page(addr);
-		/*
-		 * To do safe access to this _mapped_ area, we need
-		 * lock. But adding lock here means that we need to add
-		 * overhead of vmalloc()/vfree() calles for this _debug_
-		 * interface, rarely used. Instead of that, we'll use
-		 * kmap() and get small overhead in this access function.
-		 */
-		if (p) {
-			/*
-			 * we can expect USER0 is not used (see vread/vwrite's
-			 * function description)
-			 */
-			void *map = kmap_atomic(p, KM_USER0);
-			memcpy(buf, map + offset, length);
-			kunmap_atomic(map, KM_USER0);
-		} else
-			memset(buf, 0, length);
-
-		addr += length;
-		buf += length;
-		copied += length;
-		count -= length;
-	}
-	return copied;
-}
-
-static int aligned_vwrite(char *buf, char *addr, unsigned long count)
-{
-	struct page *p;
-	int copied = 0;
-
-	while (count) {
-		unsigned long offset, length;
-
-		offset = (unsigned long)addr & ~PAGE_MASK;
-		length = PAGE_SIZE - offset;
-		if (length > count)
-			length = count;
-		p = vmalloc_to_page(addr);
-		/*
-		 * To do safe access to this _mapped_ area, we need
-		 * lock. But adding lock here means that we need to add
-		 * overhead of vmalloc()/vfree() calles for this _debug_
-		 * interface, rarely used. Instead of that, we'll use
-		 * kmap() and get small overhead in this access function.
-		 */
-		if (p) {
-			/*
-			 * we can expect USER0 is not used (see vread/vwrite's
-			 * function description)
-			 */
-			void *map = kmap_atomic(p, KM_USER0);
-			memcpy(map + offset, buf, length);
-			kunmap_atomic(map, KM_USER0);
-		}
-		addr += length;
-		buf += length;
-		copied += length;
-		count -= length;
-	}
-	return copied;
-}
-
 /**
  *	vread() -  read vmalloc area in a safe way.
  *	@buf:		buffer for reading data

@@ -1757,49 +1676,34 @@ static int aligned_vwrite(char *buf, cha
 
 long vread(char *buf, char *addr, unsigned long count)
 {
-	struct vm_struct *tmp;
-	char *vaddr, *buf_start = buf;
-	unsigned long buflen = count;
-	unsigned long n;
-
-	/* Don't allow overflow */
-	if ((unsigned long) addr + count < count)
-		count = -(unsigned long) addr;
+	struct page *p;
+	void *map;
+	int offset = (unsigned long)addr & (PAGE_SIZE - 1);
 
-	read_lock(&vmlist_lock);
-	for (tmp = vmlist; count && tmp; tmp = tmp->next) {
-		vaddr = (char *) tmp->addr;
-		if (addr >= vaddr + tmp->size - PAGE_SIZE)
-			continue;
-		while (addr < vaddr) {
-			if (count == 0)
-				goto finished;
-			*buf = '\0';
-			buf++;
-			addr++;
-			count--;
-		}
-		n = vaddr + tmp->size - PAGE_SIZE - addr;
-		if (n > count)
-			n = count;
-		if (!(tmp->flags & VM_IOREMAP))
-			aligned_vread(buf, addr, n);
-		else /* IOREMAP area is treated as memory hole */
-			memset(buf, 0, n);
-		buf += n;
-		addr += n;
-		count -= n;
-	}
-finished:
-	read_unlock(&vmlist_lock);
+	/* Assume subpage access */
+	BUG_ON(count > PAGE_SIZE - offset);
 
-	if (buf == buf_start)
+	p = vmalloc_to_page(addr);
+	if (!p || !page_is_ram(page_to_pfn(p))) {
+		memset(buf, 0, count);
 		return 0;
-	/* zero-fill memory holes */
-	if (buf != buf_start + buflen)
-		memset(buf, 0, buflen - (buf - buf_start));
+	}
 
-	return buflen;
+	/*
+	 * To do safe access to this _mapped_ area, we need
+	 * lock. But adding lock here means that we need to add
+	 * overhead of vmalloc()/vfree() calles for this _debug_
+	 * interface, rarely used. Instead of that, we'll use
+	 * kmap() and get small overhead in this access function.
+	 *
+	 * we can expect USER0 is not used (see vread/vwrite's
+	 * function description)
+	 */
+	map = kmap_atomic(p, KM_USER0);
+	memcpy(buf, map + offset, count);
+	kunmap_atomic(map, KM_USER0);
+
+	return count;
 }
 
 /**

@@ -1834,44 +1738,24 @@ finished:
 
 long vwrite(char *buf, char *addr, unsigned long count)
 {
-	struct vm_struct *tmp;
-	char *vaddr;
-	unsigned long n, buflen;
-	int copied = 0;
-
-	/* Don't allow overflow */
-	if ((unsigned long) addr + count < count)
-		count = -(unsigned long) addr;
-	buflen = count;
+	struct page *p;
+	void *map;
+	int offset = (unsigned long)addr & (PAGE_SIZE - 1);
 
-	read_lock(&vmlist_lock);
-	for (tmp = vmlist; count && tmp; tmp = tmp->next) {
-		vaddr = (char *) tmp->addr;
-		if (addr >= vaddr + tmp->size - PAGE_SIZE)
-			continue;
-		while (addr < vaddr) {
-			if (count == 0)
-				goto finished;
-			buf++;
-			addr++;
-			count--;
-		}
-		n = vaddr + tmp->size - PAGE_SIZE - addr;
-		if (n > count)
-			n = count;
-		if (!(tmp->flags & VM_IOREMAP)) {
-			aligned_vwrite(buf, addr, n);
-			copied++;
-		}
-		buf += n;
-		addr += n;
-		count -= n;
-	}
-finished:
-	read_unlock(&vmlist_lock);
-	if (!copied)
+	/* Assume subpage access */
+	BUG_ON(count > PAGE_SIZE - offset);
+
+	p = vmalloc_to_page(addr);
+	if (!p)
+		return 0;
+	if (!page_is_ram(page_to_pfn(p)))
 		return 0;

mm/vmalloc.c finished: · line 1761

Quoted 10 lines

mm/vmalloc.c +6 -1

-	return buflen;
+
+	map = kmap_atomic(p, KM_USER0);
+	memcpy(map + offset, buf, count);
+	kunmap_atomic(map, KM_USER0);
+
+	return count;
 }
 
 /**

Wu Fengguang

author

2010-01-07 02:51 UTC #2 lore raw

Quoted 8 lines · KAMEZAWA Hiroyuki

Quoted 3 lines · On Thu, 7 Jan 2010 09:24:59 +0800 Wu Fengguang <[email protected]> wrote:
vread()/vwrite() is only called from kcore/kmem to access one page at a time. So the logic can be vastly simplified.
I recommend you to rename the function because safety of function is changed and you can show what callers are influenced.

OK.

Quoted 11 lines

Quoted 8 lines
The changes are:
- remove the vmlist walk and rely solely on vmalloc_to_page()
- replace the VM_IOREMAP check with (page && page_is_ram(pfn))
The VM_IOREMAP check is introduced in commit d0107eb07320b for per-cpu alloc. Kame, would you double check if this change is OK for that purpose?
I think VM_IOREMAP is for avoiding access to device configuration area and unexpected breakage in device. Then, VM_IOREMAP are should be skipped by the caller. (My patch _just_ moves the avoidance of callers to vread()/vwrite())

"device configuration area" is not RAM, so testing of RAM would be able to skip them?

Quoted 6 lines

Quoted 4 lines
The page_is_ram() check is necessary because kmap_atomic() is not designed to work with non-RAM pages.
I think page_is_ram() is not a complete method...on x86, it just check e820's memory range. checking VM_IOREMAP is better, I think.

(double check) Not complete or not safe?

EFI seems to not update e820 table by default. Ying, do you know why?

Quoted 10 lines

Quoted 7 lines
Even for a RAM page, we don't own the page, and cannot assume it's a _PAGE_CACHE_WB page. So I wonder whether it's necessary to do another patch to call reserve_memtype() before kmap_atomic() to ensure cache consistency?
TODO: update comments accordingly
BTW, f->f_pos problem on 64bit machine still exists and this patch is still hard to test. I stopped that because anyone doesn't show any interests.

I'm using your patch :)

I feel most inconfident on this patch, so submitted it for RFC first. I'll then submit a full patch series including your f_pos fix.

Quoted 5 lines

I have no objection to your direction.
but please rewrite the function explanation as
"addr" should be page alinged and bufsize should be multiple of page size." and change the function names.

OK, I'll rename it to vread_page().

Thanks,
Fengguang

KAMEZAWA Hiroyuki

2010-01-07 03:00 UTC #3 lore raw

Quoted 8 lines · On Thu, 7 Jan 2010 10:50:54 +0800 Wu Fengguang <[email protected]> wrote:

Quoted 8 lines
Quoted 8 lines
The changes are:
- remove the vmlist walk and rely solely on vmalloc_to_page()
- replace the VM_IOREMAP check with (page && page_is_ram(pfn))
The VM_IOREMAP check is introduced in commit d0107eb07320b for per-cpu alloc. Kame, would you double check if this change is OK for that purpose?

Quoted 7 lines

Quoted 3 lines
I think VM_IOREMAP is for avoiding access to device configuration area and unexpected breakage in device. Then, VM_IOREMAP are should be skipped by the caller. (My patch _just_ moves the avoidance of callers to vread()/vwrite())
"device configuration area" is not RAM, so testing of RAM would be able to skip them?

Sorry, that's an area what I'm not sure.
But, page_is_ram() implementation other than x86 seems not very safe... (And it seems that it's not defiend in some archs.)

Quoted 9 lines

Quoted 6 lines
Quoted 4 lines
The page_is_ram() check is necessary because kmap_atomic() is not designed to work with non-RAM pages.
I think page_is_ram() is not a complete method...on x86, it just check e820's memory range. checking VM_IOREMAP is better, I think.
(double check) Not complete or not safe?

I think not-safe because e820 doesn't seem to be updated.

Quoted 2 lines

EFI seems to not update e820 table by default. Ying, do you know why?

I hope all this kinds can be fixed by kernel/resource.c in generic way.... Now, each archs have its own.

Quoted 8 lines

Quoted 8 lines
Quoted 7 lines
Even for a RAM page, we don't own the page, and cannot assume it's a _PAGE_CACHE_WB page. So I wonder whether it's necessary to do another patch to call reserve_memtype() before kmap_atomic() to ensure cache consistency?
TODO: update comments accordingly

Quoted 8 lines

Quoted 2 lines
BTW, f->f_pos problem on 64bit machine still exists and this patch is still hard to test. I stopped that because anyone doesn't show any interests.
I'm using your patch :)
I feel most inconfident on this patch, so submitted it for RFC first. I'll then submit a full patch series including your f_pos fix.

Thank you, it's helpful.

Thanks,
-Kame

Wu Fengguang

author

2010-01-07 03:21 UTC #4 lore raw

Quoted 11 lines · KAMEZAWA Hiroyuki

Quoted 8 lines · On Thu, 7 Jan 2010 10:50:54 +0800 Wu Fengguang <[email protected]> wrote:
Quoted 8 lines
Quoted 8 lines
The changes are:
- remove the vmlist walk and rely solely on vmalloc_to_page()
- replace the VM_IOREMAP check with (page && page_is_ram(pfn))
The VM_IOREMAP check is introduced in commit d0107eb07320b for per-cpu alloc. Kame, would you double check if this change is OK for that purpose?

Quoted 8 lines

Quoted 7 lines
Quoted 3 lines
I think VM_IOREMAP is for avoiding access to device configuration area and unexpected breakage in device. Then, VM_IOREMAP are should be skipped by the caller. (My patch _just_ moves the avoidance of callers to vread()/vwrite())
"device configuration area" is not RAM, so testing of RAM would be able to skip them?
Sorry, that's an area what I'm not sure.

I believe the fundamental correctness requirement would be: to avoid accessing _PAGE_CACHE_UC/WC pages by building _PAGE_CACHE_WB mapping to them(which kmap_atomic do), whether it be physical RAM or device address area.

For example, /dev/mem allows access to
- device areas, with proper _PAGE_CACHE_* via ioremap_cache()
- _low_ physical RAM, which it directly reuse the 1:1 kernel mapping

Quoted 2 lines

But, page_is_ram() implementation other than x86 seems not very safe... (And it seems that it's not defiend in some archs.)

Ah didn't know archs other than x86. And hotplug won't update e820 as well..

Quoted 4 lines

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
The page_is_ram() check is necessary because kmap_atomic() is not designed to work with non-RAM pages.

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
I think page_is_ram() is not a complete method...on x86, it just check e820's memory range. checking VM_IOREMAP is better, I think.
(double check) Not complete or not safe?
I think not-safe because e820 doesn't seem to be updated.
Quoted 1 line
EFI seems to not update e820 table by default. Ying, do you know why?

Ying just confirmed that the kernel didn't update e820 with EFI memmap because the bootloader will fake one e820 table based on EFI memmap, in order to run legacy/unmodified kernel.

Quoted 3 lines

I hope all this kinds can be fixed by kernel/resource.c in generic way.... Now, each archs have its own.

Agreed.

Quoted 11 lines

Quoted 11 lines
Quoted 10 lines
Quoted 7 lines
Even for a RAM page, we don't own the page, and cannot assume it's a _PAGE_CACHE_WB page. So I wonder whether it's necessary to do another patch to call reserve_memtype() before kmap_atomic() to ensure cache consistency?
TODO: update comments accordingly
BTW, f->f_pos problem on 64bit machine still exists and this patch is still hard to test. I stopped that because anyone doesn't show any interests.

Quoted 6 lines

Quoted 5 lines
I'm using your patch :)
I feel most inconfident on this patch, so submitted it for RFC first. I'll then submit a full patch series including your f_pos fix.
Thank you, it's helpful.

Thanks,
Fengguang

Huang Ying

2010-01-07 03:15 UTC #5 lore raw

In reply to Wu Fengguang

Quoted 31 lines · Fengguang

Quoted 8 lines · KAMEZAWA Hiroyuki
Quoted 3 lines · On Thu, 7 Jan 2010 09:24:59 +0800 Wu Fengguang <[email protected]> wrote:
vread()/vwrite() is only called from kcore/kmem to access one page at a time. So the logic can be vastly simplified.
I recommend you to rename the function because safety of function is changed and you can show what callers are influenced.
OK.
Quoted 11 lines
Quoted 8 lines
The changes are:
- remove the vmlist walk and rely solely on vmalloc_to_page()
- replace the VM_IOREMAP check with (page && page_is_ram(pfn))
The VM_IOREMAP check is introduced in commit d0107eb07320b for per-cpu alloc. Kame, would you double check if this change is OK for that purpose?
I think VM_IOREMAP is for avoiding access to device configuration area and unexpected breakage in device. Then, VM_IOREMAP are should be skipped by the caller. (My patch _just_ moves the avoidance of callers to vread()/vwrite())
"device configuration area" is not RAM, so testing of RAM would be able to skip them?
Quoted 4 lines
Quoted 4 lines
The page_is_ram() check is necessary because kmap_atomic() is not designed to work with non-RAM pages.

Quoted 6 lines

Quoted 2 lines
I think page_is_ram() is not a complete method...on x86, it just check e820's memory range. checking VM_IOREMAP is better, I think.
(double check) Not complete or not safe?
EFI seems to not update e820 table by default. Ying, do you know why?

In EFI system, E820 table is constructed from EFI memory map in boot loader, so I think you can rely on E820 table.

Best Regards,
Huang Ying

KAMEZAWA Hiroyuki

2010-01-07 03:26 UTC #6 lore raw

Quoted 7 lines · On Thu, 07 Jan 2010 11:15:41 +0800 Huang Ying <[email protected]> wrote:

Quoted 7 lines
Quoted 6 lines
Quoted 4 lines
The page_is_ram() check is necessary because kmap_atomic() is not designed to work with non-RAM pages.
I think page_is_ram() is not a complete method...on x86, it just check e820's memory range. checking VM_IOREMAP is better, I think.

Quoted 7 lines

Quoted 3 lines
(double check) Not complete or not safe?
EFI seems to not update e820 table by default. Ying, do you know why?
In EFI system, E820 table is constructed from EFI memory map in boot loader, so I think you can rely on E820 table.

Yes, we can rely on. But concerns here is that we cannot get any information of ioremap via e820 map.

But yes,
== ioremap()
140 for (pfn = phys_addr >> PAGE_SHIFT;
141 (pfn << PAGE_SHIFT) < (last_addr & PAGE_MASK);
142 pfn++) {
143
144 int is_ram = page_is_ram(pfn);
145
146 if (is_ram && pfn_valid(pfn) && !PageReserved(pfn_to_page(pfn)))
147 return NULL;
148 WARN_ON_ONCE(is_ram);
149 }
==
you'll get warned before access if "ram" area is remapped...

But, about this patch, it seems that page_is_ram() is not free from architecture dependecy.

Thanks,
-Kame

Wu Fengguang

author

2010-01-07 05:24 UTC #7 lore raw

Quoted 20 lines · KAMEZAWA Hiroyuki

Quoted 7 lines · On Thu, 07 Jan 2010 11:15:41 +0800 Huang Ying <[email protected]> wrote:
Quoted 7 lines
Quoted 6 lines
Quoted 4 lines
The page_is_ram() check is necessary because kmap_atomic() is not designed to work with non-RAM pages.
I think page_is_ram() is not a complete method...on x86, it just check e820's memory range. checking VM_IOREMAP is better, I think.
Quoted 7 lines
Quoted 3 lines
(double check) Not complete or not safe?
EFI seems to not update e820 table by default. Ying, do you know why?
In EFI system, E820 table is constructed from EFI memory map in boot loader, so I think you can rely on E820 table.
Yes, we can rely on. But concerns here is that we cannot get any information of ioremap via e820 map.

Quoted 14 lines

But yes,
== ioremap()
140 for (pfn = phys_addr >> PAGE_SHIFT;
141 (pfn << PAGE_SHIFT) < (last_addr & PAGE_MASK);
142 pfn++) {
143
144 int is_ram = page_is_ram(pfn);
145
146 if (is_ram && pfn_valid(pfn) && !PageReserved(pfn_to_page(pfn)))
147 return NULL;
148 WARN_ON_ONCE(is_ram);
149 }
==
you'll get warned before access if "ram" area is remapped...

Right.

Quoted 2 lines

But, about this patch, it seems that page_is_ram() is not free from architecture dependecy.

Yes this is a problem. We can provide a generic page_is_ram() as below. And could further convert the existing x86 (and others) page_is_ram() to be resource-based -- since at least for now the e820 table won't be updated on memory hotplug.

Thanks,
Fengguang

---
 include/linux/ioport.h |    2 ++
 kernel/resource.c      |   18 ++++++++++++++++++
 2 files changed, 20 insertions(+)

kernel/resource.c +18 -0

@@ -297,6 +297,24 @@ int walk_system_ram_range(unsigned long 
 
 #endif
 
+static int __page_is_ram(unsigned long pfn, unsigned long nr_pages, void *arg)
+{
+	int *is_ram = arg;
+
+	*is_ram = 1;
+
+	return 1;
+}
+
+int __attribute__((weak)) page_is_ram(unsigned long pagenr)
+{
+	int is_ram = 0;
+
+	walk_system_ram_range(pagenr, 1, &is_ram, __page_is_ram);
+
+	return is_ram;
+}
+
 /*
  * Find empty slot in the resource tree given range and alignment.
  */

include/linux/ioport.h +2 -0

@@ -188,5 +188,7 @@ extern int
 walk_system_ram_range(unsigned long start_pfn, unsigned long nr_pages,
 		void *arg, int (*func)(unsigned long, unsigned long, void *));
 
+extern int page_is_ram(unsigned long pagenr);
+
 #endif /* __ASSEMBLY__ */
 #endif	/* _LINUX_IOPORT_H */

KAMEZAWA Hiroyuki

2010-01-07 05:42 UTC #8 lore raw

Quoted 41 lines · On Thu, 7 Jan 2010 13:24:03 +0800 Wu Fengguang <[email protected]> wrote:

Quoted 20 lines · KAMEZAWA Hiroyuki
Quoted 7 lines · On Thu, 07 Jan 2010 11:15:41 +0800 Huang Ying <[email protected]> wrote:
Quoted 7 lines
Quoted 6 lines
Quoted 4 lines
The page_is_ram() check is necessary because kmap_atomic() is not designed to work with non-RAM pages.
I think page_is_ram() is not a complete method...on x86, it just check e820's memory range. checking VM_IOREMAP is better, I think.
Quoted 7 lines
Quoted 3 lines
(double check) Not complete or not safe?
EFI seems to not update e820 table by default. Ying, do you know why?
In EFI system, E820 table is constructed from EFI memory map in boot loader, so I think you can rely on E820 table.
Yes, we can rely on. But concerns here is that we cannot get any information of ioremap via e820 map.
Quoted 14 lines
But yes,
== ioremap()
140 for (pfn = phys_addr >> PAGE_SHIFT;
141 (pfn << PAGE_SHIFT) < (last_addr & PAGE_MASK);
142 pfn++) {
143
144 int is_ram = page_is_ram(pfn);
145
146 if (is_ram && pfn_valid(pfn) && !PageReserved(pfn_to_page(pfn)))
147 return NULL;
148 WARN_ON_ONCE(is_ram);
149 }
==
you'll get warned before access if "ram" area is remapped...
Right.
Quoted 2 lines
But, about this patch, it seems that page_is_ram() is not free from architecture dependecy.

Quoted 7 lines

Yes this is a problem. We can provide a generic page_is_ram() as below. And could further convert the existing x86 (and others) page_is_ram() to be resource-based -- since at least for now the e820 table won't be updated on memory hotplug.
Thanks,
Fengguang

seems nice :)

Thanks,
-Kame

Quoted 36 lines

---
 include/linux/ioport.h |    2 ++
 kernel/resource.c      |   18 ++++++++++++++++++
 2 files changed, 20 insertions(+)

kernel/resource.c +18 -0

@@ -297,6 +297,24 @@ int walk_system_ram_range(unsigned long 
 
 #endif
 
+static int __page_is_ram(unsigned long pfn, unsigned long nr_pages, void *arg)
+{
+	int *is_ram = arg;
+
+	*is_ram = 1;
+
+	return 1;
+}
+
+int __attribute__((weak)) page_is_ram(unsigned long pagenr)
+{
+	int is_ram = 0;
+
+	walk_system_ram_range(pagenr, 1, &is_ram, __page_is_ram);
+
+	return is_ram;
+}
+
 /*
  * Find empty slot in the resource tree given range and alignment.
  */

include/linux/ioport.h extern int · line 194

Quoted 7 lines

include/linux/ioport.h +2 -0

@@ -188,5 +188,7 @@ extern int
 walk_system_ram_range(unsigned long start_pfn, unsigned long nr_pages,
 		void *arg, int (*func)(unsigned long, unsigned long, void *));
 
+extern int page_is_ram(unsigned long pagenr);
+
 #endif /* __ASSEMBLY__ */
 #endif	/* _LINUX_IOPORT_H */