mm: madvise(WILLNEED) for anonymous memory

Lennart asked for madvise(WILLNEED) to work on anonymous pages, he plans to use this to pre-fault pages. He currently uses: mlock/munlock for this purpose.

[ compile tested only ]

Signed-off-by Peter Zijlstra <[email protected]>

---

mm/madvise.c +0 -0

index 93ee375..eff60ce 100644

mm/madvise.c +2 -0

+}
+
 /*
  * Schedule all required I/O operations.  Do not wait for completion.
  */

Quoted 4 lines · Peter Zijlstra

Lennart asked for madvise(WILLNEED) to work on anonymous pages, he plans to use this to pre-fault pages. He currently uses: mlock/munlock for this purpose.

I certainly agree with this in principle: it just seems an unnecessary and surprising restriction to refuse on anonymous vmas; I guess the only reason for not adding this was not having anyone asking for it until now. Though, does Lennart realize he could use MAP_POPULATE in the mmap?

Quoted 11 lines

Signed-off-by Peter Zijlstra <[email protected]>

---

mm/madvise.c +0 -0

index 93ee375..eff60ce 100644
@@ -100,6 +100,24 @@ out:
 	return error;
 }
 

mm/madvise.c out: · line 105

Quoted 3 lines

mm/madvise.c +3 -0

+static long madvice_willneed_anon(struct vm_area_struct *vma,
+				  struct vm_area_struct **prev,
+				  unsigned long start, unsigned long end)

mavise.c uses "madvise_" rather than " madvice_" throughout, so please go with the flow.

mm/madvise.c out: · line 111

Quoted 6 lines

mm/madvise.c +6 -0

+{
+	int ret, len;
+
+	*prev = vma;
+	if (end > vma->vm_end)
+		end = vma->vm_end;

Please check, but I think the upper level ensures end is within range.

mm/madvise.c out: · line 118

Quoted 7 lines

mm/madvise.c +7 -0

+
+	len = end - start;
+	ret = get_user_pages(current, current->mm, start, len,
+			0, 0, NULL, NULL);
+	if (ret < 0)
+		return ret;
+	return ret == len ? 0 : -1;

It's not good to return -1 as an alternative to a real errno:
it'll look like -EPERM. If you copied that from somewhere, better send a patch to fix the somewhere! Ah, yes, make_pages_present: it happens that nobody is interested in its return value, so we could make it a void; but that'd just be a cleanup. What to do here if non-negative ret less than len? Oh, just return 0, that's good enough in this case (the file case always returns 0).

Hmm, might it be better to use make_pages_present itself, fixing its retval, rather than using get_user_pages directly? (I'd hope the caching makes its repeat of find_vma not an overhead.)

Interesting divergence: make_pages_present faults in writable pages in a writable vma, whereas the file case's force_page_cache_readahead doesn't even insert the pages into the mm.

Quoted 12 lines

mm/madvise.c +3 -1

+}
+
 /*
  * Schedule all required I/O operations.  Do not wait for completion.
  */

@@ -110,7 +128,7 @@ static long madvise_willneed(struct vm_area_struct * vma,
 	struct file *file = vma->vm_file;
 
 	if (!file)
-		return -EBADF;
+		return madvice_willneed_anon(vma, prev, start, end);
 

mm/madvise.c static long madvise_willneed(struct vm_area_struct * vma, · line 134

Quoted 2 lines

mm/madvise.c +0 -0

 	if (file->f_mapping->a_ops->get_xip_page) {
 		/* no bad return value, but ignore advice */

And there's a correctly invisible hunk to the patch too: this extension of MADV_WILLNEED also does not require down_write of mmap_sem, so madvise_need_mmap_write can remain unchanged.

Quoted 3 lines · Hugh Dickins

Interesting divergence: make_pages_present faults in writable pages in a writable vma, whereas the file case's force_page_cache_readahead doesn't even insert the pages into the mm.

Yeah, the find_vma and write fault thing are the reason I didn't use make_pages_present.

I had noticed the difference in pte population between force_page_cache_readahead and make_pages_present, but it seemed to me that writing a function to walk the page tables and populate the swapcache but not populate the ptes wasn't worth the effort.

Quoted 6 lines · Peter Zijlstra

Quoted 3 lines · Hugh Dickins

Interesting divergence: make_pages_present faults in writable pages in a writable vma, whereas the file case's force_page_cache_readahead doesn't even insert the pages into the mm.

Quoted 7 lines

Yeah, the find_vma and write fault thing are the reason I didn't use make_pages_present.

I had noticed the difference in pte population between force_page_cache_readahead and make_pages_present, but it seemed to me that writing a function to walk the page tables and populate the swapcache but not populate the ptes wasn't worth the effort.

Ah, another, more important difference:

force_page_cache_readahead will not wait for the read to complete, whereas get_user_pages() will be fully synchronous.

I think I'd better come up with something else then,..

Quoted 6 lines · Hugh Dickins

On Thu, 20 Dec 2007, Peter Zijlstra wrote:

I certainly agree with this in principle: it just seems an unnecessary and surprising restriction to refuse on anonymous vmas; I guess the only reason for not adding this was not having anyone asking for it until now. Though, does Lennart realize he could use MAP_POPULATE in the mmap?

I think he's trying to get his data swapped-in.

Quoted 8 lines

mm/madvise.c out:

Quoted 6 lines

mm/madvise.c +6 -0

+{
+	int ret, len;
+
+	*prev = vma;
+	if (end > vma->vm_end)
+		end = vma->vm_end;

Please check, but I think the upper level ensures end is within range.

It certainly looks like it, but I since the file case did this check I thought it prudent to also do it. I guess I might as well remove both.

Quoted 7 lines

Hmm, might it be better to use make_pages_present itself, fixing its retval, rather than using get_user_pages directly? (I'd hope the caching makes its repeat of find_vma not an overhead.)

Interesting divergence: make_pages_present faults in writable pages in a writable vma, whereas the file case's force_page_cache_readahead doesn't even insert the pages into the mm.

Yeah, the find_vma and write fault thing are the reason I didn't use make_pages_present.

I had noticed the difference in pte population between force_page_cache_readahead and make_pages_present, but it seemed to me that writing a function to walk the page tables and populate the swapcache but not populate the ptes wasn't worth the effort.

Ah, another, more important difference:

force_page_cache_readahead will not wait for the read to complete, whereas get_user_pages() will be fully synchronous.

I think I'd better come up with something else then,..

The asynch code: perhaps not worth doing for MADV_WILLNEED alone, but might prove useful for more general use when swapping in. Not really the same as Con's swap prefetch, but worth looking at that for reference. But I guess this becomes a much bigger issue than you were intending to get into here.

Quoted 5 lines · Hugh Dickins

The asynch code: perhaps not worth doing for MADV_WILLNEED alone, but might prove useful for more general use when swapping in. Not really the same as Con's swap prefetch, but worth looking at that for reference. But I guess this becomes a much bigger issue than you were intending to get into here.

heh, yeah, got somewhat more complex that I'd hoped for.

last patch for today (not even compile tested), will do a proper patch and test it tomorrow.

---
A best effort MADV_WILLNEED implementation for anonymous memory.

It adds a batch method to the page table walk routines so we can copy a few ptes while holding the kmap, which makes it possible to allocate the backing pages using GFP_KERNEL.

Quoted 14 lines · Peter Zijlstra

Quoted 5 lines · Hugh Dickins

The asynch code: perhaps not worth doing for MADV_WILLNEED alone, but might prove useful for more general use when swapping in. Not really the same as Con's swap prefetch, but worth looking at that for reference. But I guess this becomes a much bigger issue than you were intending to get into here.

heh, yeah, got somewhat more complex that I'd hoped for.

last patch for today (not even compile tested), will do a proper patch and test it tomorrow.

Quoted 6 lines

---
A best effort MADV_WILLNEED implementation for anonymous memory.

It adds a batch method to the page table walk routines so we can copy a few ptes while holding the kmap, which makes it possible to allocate the backing pages using GFP_KERNEL.

Yuck. We actually need to just fix the atomic kmap issue in the existing pagemap code rather than add a new method, I think.

If performance of map/unmap is too slow at a granularity of 1, we can add some internal batching in the CONFIG_HIGHPTE case.

Quoted 3 lines

Lennart asked for madvise(WILLNEED) to work on anonymous pages, he plans to use this to pre-fault pages. He currently uses: mlock/munlock for this purpose.

I certainly agree with this in principle: it just seems an unnecessary and surprising restriction to refuse on anonymous vmas; I guess the only reason for not adding this was not having anyone asking for it until now. Though, does Lennart realize he could use MAP_POPULATE in the mmap?