Re: get_user_pages rewrite rediffed against 2.5.47-mm1

[Andrew Morton <akpm@digeo.com>]

Ingo Oeser wrote:
> 
> Hi Andrew,
> 
> the get_user_pages rewrite has been rediffed against 2.5.47-mm1

The single diff helps, thanks.

I'm still having indigestion over this:

+/*** Page walking API ***/
+
+/* &custom_page_walker - A custom page walk handler for walk_user_pages().
+ * vma:         The vma we walk pages of.
+ * page:        The page we found or an %ERR_PTR() value
+ * virt_addr:   The virtual address we are at while walking.
+ * customdata:  Anything you would like to pass additionally.
+ *
+ * Returns:
+ *      Negative values -> ERRNO values.
+ *      0               -> continue page walking.
+ *      1               -> abort page walking.
+ *
+ * If this functions gets a page, for which %IS_ERR(@page) is true, than it
+ * should do it's cleanup of customdata and return -PTR_ERR(@page).
+ *
+ * If IS_ERR(@page) is NOT TRUE, this function is called with
+ * @vma->vm_mm->page_table_lock held. 
+ *
+ * The value of @vma is undefined if IS_ERR(@page) is TRUE.
+ * (So never use or check it if IS_ERR(@page) is TRUE)
+ *
+ * If it returns a negative value but got a valid page, then the
+ * page_table_lock must be dropped by this function. (This condition should be
+ * rather rare.)
+ */
+typedef int (*custom_page_walker_t)(struct vm_area_struct *vma, 
+		struct page *page, unsigned long virt_addr, void *customdata);
+

I think I see what you're doing now.  You've overloaded the callback,
with an IS_ERR value of "page" to mean "something went wrong".

Would that be a correct interpretation?

If so, it would be better (ie: more Linus-friendly) to make that a
separate callback.  One which is called outside the lock, and which
has distinctly different semantics from the normal page walker.

Some (all?) callers of walk_user_pages() may not even be interested
in the error-time callout.  In fact it may be possible to just leave
the state at time-of-error in the state structure (see below) and just
return an error code to the caller of walk_user_pages()?

I suggest that it's time to fold all these arguments into a structure
which is on the caller's stack, and pass the address of that around.
This will simplify things, but one needs to be careful to think through
the ownership rules of the various parts of that structure.

Please review your ERR_PTR handling.  You have lots of these:

+               return ERR_PTR(EFAULT);

which are all wrong.  It needs to be `ERR_PTR(-EFAULT)'.  (editing
the diff is the easy fix ;))

When you do the above, this becomes wrong too:

	return -PTR_ERR(page);

So please check all those too.

Also, please rip everything which is appropriate out of mm/memory.c
and create a new file in mm/ for it.


I cannot guarantee that we can get this merged up, frankly.  We need
a *reason* for doing that.  The current code is "good enough" for
current callers.  So the best I can do is to get it under test, give
Linus a heads-up that it's floating about while you get in there and
start creating reasons for merging it - namely the clients down in
device drivers.

If we don't make it then we can definitely push it for 2.7.

How does that suit?
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/