vm_ops.page_mkwrite() fails with vmalloc on 2.6.23

Hi,

i have a problem with vmalloc() and vm_ops.page_mkwrite().

ReadOnly access works, but on a write access the VM will endless invoke the vm_ops.page_mkwrite() handler.

I tracked down the problem to the
struct page.mapping pointer,
which is NULL.

The problem original occurs with the fb_defio driver (driver/video/fb_defio.c). This driver use the vm_ops.page_mkwrite() handler for tracking the modified pages, which will be in an extra thread handled, to perform the IO and clean and write protect all pages with page_clean().

I am not sure if the is a feature of the new rmap code or a bug.

Is there an way to get a similar functionality? Currently, i have no idea
how to get the ptep from a page alloced with vmalloc().

Greetings,
Stefani

Here is a small sample driver:

#include <linux/module.h>
#include <linux/errno.h>
#include <linux/string.h>
#include <linux/mm.h>
#include <linux/vmalloc.h>
#include <linux/fs.h>

#define DEVICE_NAME "mydrv"
#define DEVICE_MAJOR 240

static u8 *mydrv_memory;
static const u_long mydrv_memory_size=1024*1024; // 1 Megabyte

#ifdef MODULE
static u_long vmas=0;
#endif

static void mydrv_vma_open(struct vm_area_struct *vma) {

#ifdef MODULE
	if (vmas++==0)
		try_module_get(THIS_MODULE);

#endif
}

static void mydrv_vma_close(struct vm_area_struct *vma) {

#ifdef MODULE
	if (--vmas==0)
		module_put(THIS_MODULE);

#endif
}

struct page *mydrv_vma_nopage(struct vm_area_struct *vma,unsigned long address,int *type)

{
	unsigned long offset;
	struct page *page;

	offset = (address - vma->vm_start) + (vma->vm_pgoff << PAGE_SHIFT);
	printk("--> mydrv_vma_nopage:%lu(%08lx)\n",offset,offset);

	if (offset >= mydrv_memory_size)
		return NOPAGE_SIGBUS;

	page = vmalloc_to_page(mydrv_memory + offset);
	if (!page)
		return NOPAGE_OOM;

	get_page(page);
	if (type)
		*type = VM_FAULT_MINOR;
	return page;
}

int mydrv_mkwrite(struct vm_area_struct *vma,struct page *page)

{
	printk("--> mydrv_mkwrite:%p\n",page->mapping);

	return 0;
}

struct vm_operations_struct mydrv_vm_ops = {
.open = mydrv_vma_open,
.close = mydrv_vma_close,
.nopage = mydrv_vma_nopage,
.page_mkwrite = mydrv_mkwrite,
};

static int mydrv_mmap(struct file *file, struct vm_area_struct *vma)

{
	vma->vm_ops = &mydrv_vm_ops;
	vma->vm_flags |= ( VM_IO | VM_RESERVED | VM_DONTEXPAND );
	vma->vm_private_data = 0;
	mydrv_vma_open(vma);
	return 0;
}

int mydrv_open(struct inode *inode, struct file *file)

{
	int		minor;

	minor=MINOR(file->f_dentry->d_inode->i_rdev);
	printk("--> mydrv_open called for minor: %d\n", minor);

	if (minor>0) {
		printk("--> mydrv_open minor %d failed\n",minor);
		return -ENODEV;
	}

	printk("--> mydrv_open o.K.\n");
	return 0;
}

int mydrv_close(struct inode *inode, struct file *file)

{
	int		minor;

	minor=MINOR(file->f_dentry->d_inode->i_rdev);
	printk("--> mydrv_close for minor: %d\n", minor);
	return 0;
}

struct file_operations mydrv_fops = {
.owner=THIS_MODULE,
.mmap=mydrv_mmap,
.open=mydrv_open,
.release=mydrv_close,
};

int init_module(void)

{
	printk("--> init_module called\n");

	if (!(mydrv_memory = vmalloc(mydrv_memory_size)))
		return -ENOMEM;

	if (register_chrdev(DEVICE_MAJOR, DEVICE_NAME, &mydrv_fops) < 0) {
		printk("--> Error registering driver.\n");
		return -ENODEV;
	}

	memset(mydrv_memory, 0, mydrv_memory_size);
	printk("--> init_module done\n");
	return 0;
}

void cleanup_module(void)

{
	printk("--> cleanup_module called\n");
	unregister_chrdev(DEVICE_MAJOR,DEVICE_NAME);
	vfree(mydrv_memory);
	printk("--> cleanup_module done\n");
}

MODULE_LICENSE("GPL");
MODULE_AUTHOR("Stefan Seibold <[email protected]>"); MODULE_DESCRIPTION("Test Driver");

Quoted 6 lines · On Mon, 22 Oct 2007 16:40:57 +0200 Stefani Seibold <[email protected]> wrote:

The problem original occurs with the fb_defio driver (driver/video/fb_defio.c). This driver use the vm_ops.page_mkwrite() handler for tracking the modified pages, which will be in an extra thread handled, to perform the IO and clean and write protect all pages with page_clean().

Quoted 7 lines · Andrew Morton

Quoted 6 lines · On Mon, 22 Oct 2007 16:40:57 +0200 Stefani Seibold <[email protected]> wrote:

The problem original occurs with the fb_defio driver (driver/video/fb_defio.c). This driver use the vm_ops.page_mkwrite() handler for tracking the modified pages, which will be in an extra thread handled, to perform the IO and clean and write protect all pages with page_clean().

Hi,

An aside, I just tested that deferred IO works fine on 2.6.22.10/pxa255.

I understood from the thread that PeterZ is looking into page_mkclean changes which I guess went into 2.6.23. I'm also happy to help in any way if the way we're doing fb_defio needs to change.

Quoted 9 lines · Jaya Kumar

Quoted 7 lines · Andrew Morton

Quoted 6 lines · On Mon, 22 Oct 2007 16:40:57 +0200 Stefani Seibold <[email protected]> wrote:

The problem original occurs with the fb_defio driver (driver/video/fb_defio.c). This driver use the vm_ops.page_mkwrite() handler for tracking the modified pages, which will be in an extra thread handled, to perform the IO and clean and write protect all pages with page_clean().

Quoted 7 lines

Hi,

An aside, I just tested that deferred IO works fine on 2.6.22.10/pxa255.

I understood from the thread that PeterZ is looking into page_mkclean changes which I guess went into 2.6.23. I'm also happy to help in any way if the way we're doing fb_defio needs to change.

Yeah, its the truncate race stuff introduced by Nick in
d0217ac04ca6591841e5665f518e38064f4e65bd

I'm a bit at a loss on how to go around fixing this. One ugly idea I had was to check page->mapping before going into page_mkwrite() and when that is null, don't bother with the truncate check.

Quoted 3 lines · Peter Zijlstra

Quoted 2 lines · Jaya Kumar

Quoted 1 line · Andrew Morton

On Mon, 22 Oct 2007 16:40:57 +0200 Stefani Seibold

Quoted 15 lines

Quoted 14 lines

Quoted 5 lines

Quoted 5 lines

The problem original occurs with the fb_defio driver (driver/video/fb_defio.c). This driver use the vm_ops.page_mkwrite() handler for tracking the modified pages, which will be in an extra thread handled, to perform the IO and clean and write protect all pages with page_clean().

Hi,

An aside, I just tested that deferred IO works fine on 2.6.22.10/pxa255.

I understood from the thread that PeterZ is looking into page_mkclean changes which I guess went into 2.6.23. I'm also happy to help in any way if the way we're doing fb_defio needs to change.

Quoted 6 lines

Yeah, its the truncate race stuff introduced by Nick in
d0217ac04ca6591841e5665f518e38064f4e65bd

I'm a bit at a loss on how to go around fixing this. One ugly idea I had was to check page->mapping before going into page_mkwrite() and when that is null, don't bother with the truncate check.

Something like this

---
 mm/memory.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Index: linux-2.6/mm/memory.c
===================================================================

mm/memory.c +2 -0

@@ -2300,6 +2300,8 @@ static int __do_fault(struct mm_struct *
 			 * to become writable
 			 */
 			if (vma->vm_ops->page_mkwrite) {
+				struct address_space *mapping = page->mapping;
+
 				unlock_page(page);
 				if (vma->vm_ops->page_mkwrite(vma, page) < 0) {
 					ret = VM_FAULT_SIGBUS;

Quoted 7 lines · Andrew Morton

Quoted 6 lines · On Mon, 22 Oct 2007 16:40:57 +0200 Stefani Seibold <[email protected]> wrote:

The problem original occurs with the fb_defio driver (driver/video/fb_defio.c). This driver use the vm_ops.page_mkwrite() handler for tracking the modified pages, which will be in an extra thread handled, to perform the IO and clean and write protect all pages with page_clean().

An aside, I just tested that deferred IO works fine on 2.6.22.10/pxa255.

I understood from the thread that PeterZ is looking into page_mkclean changes which I guess went into 2.6.23. I'm also happy to help in any way if the way we're doing fb_defio needs to change.

Quoted 5 lines · Jaya Kumar

An aside, I just tested that deferred IO works fine on 2.6.22.10/pxa255.

I understood from the thread that PeterZ is looking into page_mkclean changes which I guess went into 2.6.23. I'm also happy to help in any way if the way we're doing fb_defio needs to change.

OK, seems I can't read. Or at least, I missed a large part of the problem.

page_mkclean() hasn't changed, it was ->page_mkwrite() that changed. And looking at the fb_defio code, I'm not sure I understand how its page_mkclean() use could ever have worked.

The proposed patch [1] only fixes the issue of ->page_mkwrite() on vmalloc()'ed memory. Not page_mkclean(), and that has never worked from what I can make of it.

Jaya, could you shed some light on this? I presume you had your display working.

Quoted 14 lines · Peter Zijlstra

Quoted 5 lines · Jaya Kumar

An aside, I just tested that deferred IO works fine on 2.6.22.10/pxa255.

I understood from the thread that PeterZ is looking into page_mkclean changes which I guess went into 2.6.23. I'm also happy to help in any way if the way we're doing fb_defio needs to change.

OK, seems I can't read. Or at least, I missed a large part of the problem.

page_mkclean() hasn't changed, it was ->page_mkwrite() that changed. And looking at the fb_defio code, I'm not sure I understand how its page_mkclean() use could ever have worked.

Quoted 7 lines

The proposed patch [1] only fixes the issue of ->page_mkwrite() on vmalloc()'ed memory. Not page_mkclean(), and that has never worked from what I can make of it.

Jaya, could you shed some light on this? I presume you had your display working.

I thought I had it working. I saw the display update after each mmap/write sequence to the framebuffer. I need to check if there's an munmap or anything else going on in between write sequences that would cause it to behave like page_mkclean was working.

Is it correct to assume that page_mkclean should mark the pages read-only so that the next write would again trigger mkwrite?

Even if the page was from a vmalloc_to_page()?

Quoted 28 lines · Jaya Kumar

Quoted 14 lines · Peter Zijlstra

Quoted 5 lines · Jaya Kumar

An aside, I just tested that deferred IO works fine on 2.6.22.10/pxa255.

I understood from the thread that PeterZ is looking into page_mkclean changes which I guess went into 2.6.23. I'm also happy to help in any way if the way we're doing fb_defio needs to change.

OK, seems I can't read. Or at least, I missed a large part of the problem.

page_mkclean() hasn't changed, it was ->page_mkwrite() that changed. And looking at the fb_defio code, I'm not sure I understand how its page_mkclean() use could ever have worked.

Quoted 7 lines

The proposed patch [1] only fixes the issue of ->page_mkwrite() on vmalloc()'ed memory. Not page_mkclean(), and that has never worked from what I can make of it.

Jaya, could you shed some light on this? I presume you had your display working.

I thought I had it working. I saw the display update after each mmap/write sequence to the framebuffer. I need to check if there's an munmap or anything else going on in between write sequences that would cause it to behave like page_mkclean was working.

Quoted 2 lines

Is it correct to assume that page_mkclean should mark the pages read-only so that the next write would again trigger mkwrite?

Well, yes, that is the intended behaviour.

Quoted 1 line

Even if the page was from a vmalloc_to_page()?

That is the crux, I only ever implemented it for file pages.

[ also, remap_vmalloc_range() suffers similar issues, only file and anon
have proper rmap ]

I'm not sure we want full rmap for remap_pfn/vmalloc_range, but perhaps we could assist drivers in maintaining and using vma lists.

I think page_mkclean_one() would work if you'd manually set page->index and iterate the vmas yourself. Although atm I'm not sure of anything so don't pin me on it.

Quoted 10 lines · Peter Zijlstra

[ also, remap_vmalloc_range() suffers similar issues, only file and anon
have proper rmap ]

I'm not sure we want full rmap for remap_pfn/vmalloc_range, but perhaps we could assist drivers in maintaining and using vma lists.

I think page_mkclean_one() would work if you'd manually set page->index and iterate the vmas yourself. Although atm I'm not sure of anything so don't pin me on it.

:-) If it's anybody's fault, it's mine for not testing properly. My bad.

In the case of defio, I think it's no trouble to build a list of vmas at mmap time and then to iterate through them when it's ready for mkclean time as you suggested. I don't fully understand page->index yet. I had thought it was only used by swap cache or file map.

On an unrelated note, I was looking for somewhere to stuff a 16 bit offset (so that I have a cheap way to know which struct page corresponds to which framebuffer block or offset) for another driver. I had thought page->index was it but I think I am wrong now.

Quoted 19 lines · Peter Zijlstra

Quoted 10 lines

[ also, remap_vmalloc_range() suffers similar issues, only file and anon
have proper rmap ]

I'm not sure we want full rmap for remap_pfn/vmalloc_range, but perhaps we could assist drivers in maintaining and using vma lists.

I think page_mkclean_one() would work if you'd manually set page->index and iterate the vmas yourself. Although atm I'm not sure of anything so don't pin me on it.

:-) If it's anybody's fault, it's mine for not testing properly. My bad.

In the case of defio, I think it's no trouble to build a list of vmas at mmap time and then to iterate through them when it's ready for mkclean time as you suggested. I don't fully understand page->index yet. I had thought it was only used by swap cache or file map.

Quoted 4 lines

On an unrelated note, I was looking for somewhere to stuff a 16 bit offset (so that I have a cheap way to know which struct page corresponds to which framebuffer block or offset) for another driver. I had thought page->index was it but I think I am wrong now.

Yeah, page->index is used along with vma->vmpgoff and vma->vm_start to determine the address of the page in the given vma:

  address = vma->vm_start + ((page->index - vma->vm_pgoff) << PAGE_SHIFT);

and from that address the pte can be found by walking the vma->vm_mm page tables.

So page->index does what you want it to, identify which part of the framebuffer this particular page belongs to.

the question is how can i get all pte's from a vmalloc'ed memory. Due to the zeroed mapping pointer i dont see how to do this?

i have a problem with vmalloc() and vm_ops.page_mkwrite().

ReadOnly access works, but on a write access the VM will endless invoke the vm_ops.page_mkwrite() handler.

I tracked down the problem to the
struct page.mapping pointer,
which is NULL.

The problem original occurs with the fb_defio driver (driver/video/fb_defio.c). This driver use the vm_ops.page_mkwrite() handler for tracking the modified pages, which will be in an extra thread handled, to perform the IO and clean and write protect all pages with page_clean().

I am not sure if the is a feature of the new rmap code or a bug.

Is there an way to get a similar functionality? Currently, i have no idea
how to get the ptep from a page alloced with vmalloc().

Quoted 3 lines · Stefani Seibold

the question is how can i get all pte's from a vmalloc'ed memory. Due to the zeroed mapping pointer i dont see how to do this?

The mapping pointer is zeroed because you've done nothing to set it. Below is how I answered you a week ago. But this is new territory (extending page_mkclean to work on more than just pagecache pages), I'm still unsure what would be the safest way to do it.

So page->index does what you want it to, identify which part of the framebuffer this particular page belongs to.

Quoted 2 lines · Peter Zijlstra

So page->index does what you want it to, identify which part of the framebuffer this particular page belongs to.

Ok. I'm attempting to walk the code sequence. Here's what I think:

- driver loads
- driver vmalloc()s its fb
- this creates the necessary pte entries

then...
- app mmap(/dev/fb0)
- vma is created
- defio mmap adds this vma to private list (equivalent of address_space or anon_vma)

- app touches base + pixel(128,128) = base + 16k
- page fault
- defio nopage gets called
- defio nopage does vmalloc_to_page(base+16k)

- that finds the correct struct page corresponding to that vaddr. page->index has not been set by anyone so far, right?
* ah... i see, you are suggesting that this is where I could set the index since i know the offset i want it to represent. right?

- defio mkwrite get called. defio adds page to its list. schedules delayed work
- app keeps writing the page
- delayed work occurs
- foreach vma { foreach page { page_mkclean_one(page, vma) }

- cycle repeats...

On Tue, 2007-10-30 at 09:16 -0400, Jaya Kumar wrote:

Quoted 2 lines

- defio mmap adds this vma to private list (equivalent of address_space or anon_vma)

Quoted 1 line

- foreach vma { foreach page { page_mkclean_one(page, vma) }

Yeah, page_mkclean_one(page, vma) will use vma_address() to obtain an user-space address for the page in this vma using page->index and the formula from the last email, this address is then used to walk the page tables and obtain a pte.

Quoted 1 line · Peter Zijlstra

On Tue, 2007-10-30 at 09:16 -0400, Jaya Kumar wrote:

....

Quoted 2 lines

Quoted 2 lines

- defio mmap adds this vma to private list (equivalent of address_space or anon_vma)

....

Quoted 6 lines

Quoted 1 line

- foreach vma { foreach page { page_mkclean_one(page, vma) }

Yeah, page_mkclean_one(page, vma) will use vma_address() to obtain an user-space address for the page in this vma using page->index and the formula from the last email, this address is then used to walk the page tables and obtain a pte.

I don't understand why you suggested an anon_vma, nor why Jaya is suggesting a private list. All vmas mapping /dev/fb0 will be kept in the prio_tree rooted in its struct address_space (__vma_link_file in mm/mmap.c). And page_mkclean gets page_mkclean_file to walk that very tree. The missing part is just the setting of page->mapping to point to that struct address_space (and clearing it before finally freeing the pages), and the setting of page->index as you described. Isn't it?

I don't understand why you suggested an anon_vma, nor why Jaya is suggesting a private list. All vmas mapping /dev/fb0 will be kept in the prio_tree rooted in its struct address_space (__vma_link_file in mm/mmap.c). And page_mkclean gets page_mkclean_file to walk that very tree. The missing part is just the setting of page->mapping to point to that struct address_space (and clearing it before finally freeing the pages), and the setting of page->index as you described. Isn't it?