All of lore.kernel.org
 help / color / mirror / Atom feed
* fault_in_pages_writeable/fault_in_pages_readable don't fault in everything
@ 2011-07-09 21:01 Keith Packard
  2011-07-27 21:10 ` Andrew Morton
  0 siblings, 1 reply; 2+ messages in thread
From: Keith Packard @ 2011-07-09 21:01 UTC (permalink / raw)
  To: linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1210 bytes --]


fault_in_pages_writeable and fault_in_pages_readable are only willing to
fault two pages at the most. I can't find any place where this is a good
idea, and in fact ntfs has replaced fault_in_pages_readable with a
private version which does the right thing.

Here's an (untested) patch which makes fault_in_pages_writeable hit
every page instead of just the first and last. It seems like this might
improve performance of larger read operations which may now end up
taking the slow path when an intermediate page is faulted in
__copy_to_user_inatomic.

diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index 716875e..f355f29 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -418,7 +418,13 @@ static inline int fault_in_pages_writeable(char __user *uaddr, int size)
 	 * Writing zeroes into userspace here is OK, because we know that if
 	 * the zero gets there, we'll be overwriting it.
 	 */
-	ret = __put_user(0, uaddr);
+	for (;;) {
+		ret = __put_user(0, uaddr);
+		if (size < PAGE_SIZE)
+			break;
+		size -= PAGE_SIZE;
+		uaddr += PAGE_SIZE;
+	}
 	if (ret == 0) {
 		char __user *end = uaddr + size - 1;
 

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply related	[flat|nested] 2+ messages in thread

* Re: fault_in_pages_writeable/fault_in_pages_readable don't fault in everything
  2011-07-09 21:01 fault_in_pages_writeable/fault_in_pages_readable don't fault in everything Keith Packard
@ 2011-07-27 21:10 ` Andrew Morton
  0 siblings, 0 replies; 2+ messages in thread
From: Andrew Morton @ 2011-07-27 21:10 UTC (permalink / raw)
  To: Keith Packard; +Cc: linux-kernel

On Sat, 09 Jul 2011 14:01:24 -0700
"Keith Packard" <keithp@keithp.com> wrote:

> 
> fault_in_pages_writeable and fault_in_pages_readable are only willing to
> fault two pages at the most. I can't find any place where this is a good
> idea, and in fact ntfs has replaced fault_in_pages_readable with a
> private version which does the right thing.
> 
> Here's an (untested) patch which makes fault_in_pages_writeable hit
> every page instead of just the first and last. It seems like this might
> improve performance of larger read operations which may now end up
> taking the slow path when an intermediate page is faulted in
> __copy_to_user_inatomic.
> 
> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> index 716875e..f355f29 100644
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -418,7 +418,13 @@ static inline int fault_in_pages_writeable(char __user *uaddr, int size)
>  	 * Writing zeroes into userspace here is OK, because we know that if
>  	 * the zero gets there, we'll be overwriting it.
>  	 */
> -	ret = __put_user(0, uaddr);
> +	for (;;) {
> +		ret = __put_user(0, uaddr);
> +		if (size < PAGE_SIZE)
> +			break;
> +		size -= PAGE_SIZE;
> +		uaddr += PAGE_SIZE;
> +	}
>  	if (ret == 0) {
>  		char __user *end = uaddr + size - 1;

I worry a bit about the "Writing zeroes into userspace here is OK,
because we know that if the zero gets there, we'll be overwriting it"
thing.  As we're now dealing with multiple pages I suspect there will
be scenarios in whcih the subsequent copy_to_user() will fail and we'll
be left having written a sprinkle of zeroes into userspace while
falsely telling userspace that the affected memory was unaltered (via a
short return from read()).

Should we alter fault_in_pages_readable() to match?

The functions are getting too large for inlining.  Perhaps make them
non-static inline in filemap.c so the most common callsite inlines it
as well as generating an out-of-line copy.  The compiler might uninline
it anyway in that case without addition of funky compiler options, but
it's a hot path in the read() and write() cases, so effort here is
warranted.

Extra marks for removing the custom code in ntfs.

More extra marks for designing the code in such a way that the
newly-added code gets elided by the compiler in the single-page cases
in filemap.c.

Even more extra marks for testing it ;)

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2011-07-27 21:10 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-07-09 21:01 fault_in_pages_writeable/fault_in_pages_readable don't fault in everything Keith Packard
2011-07-27 21:10 ` Andrew Morton

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.