Re: [PATCH] mm: extend prefault helpers to fault in more thanPAGE_SIZE

[Andrew Morton]

On Fri, 24 Feb 2012 14:34:31 +0100
Daniel Vetter <daniel@xxxxxxxx> wrote:

> > > --- a/include/linux/pagemap.h
> > > +++ b/include/linux/pagemap.h
> > > @@ -408,6 +408,7 @@ extern void add_page_wait_queue(struct page *page, wait_queue_t *waiter);
> > >  static inline int fault_in_pages_writeable(char __user *uaddr, int size)
> > >  {
> > >  	int ret;
> > > +	char __user *end = uaddr + size - 1;
> > >  
> > >  	if (unlikely(size == 0))
> > >  		return 0;
> > > @@ -416,17 +417,20 @@ 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);
> > > +	while (uaddr <= end) {
> > > +		ret = __put_user(0, uaddr);
> > > +		if (ret != 0)
> > > +			return ret;
> > > +		uaddr += PAGE_SIZE;
> > > +	}
> > 
> > The callsites in filemap.c are pretty hot paths, which is why this
> > thing remains explicitly inlined.  I think it would be worth adding a
> > bit of code here to avoid adding a pointless test-n-branch and larger
> > cache footprint to read() and write().
> > 
> > A way of doing that is to add another argument to these functions, say
> > "bool multipage".  Change the code to do
> > 
> > 	if (multipage) {
> > 		while (uaddr <= end) {
> > 			...
> > 		}
> > 	}
> > 
> > and change the callsites to pass in constant "true" or "false".  Then
> > compile it up and manually check that the compiler completely removed
> > the offending code from the filemap.c callsites.
> > 
> > Wanna have a think about that?  If it all looks OK then please be sure
> > to add code comments explaining why we did this.
> 
> I wasn't really happy with the added branch either, but failed to come up
> with a trick to avoid it. Imho adding new _multipage variants of these
> functions instead of adding a constant argument is simpler because the
> functions don't really share much thanks to the block below. I'll see what
> it looks like (and obviously add a comment explaining what's going on).

well... that's just syntactic sugar:

static inline int __fault_in_pages_writeable(char __user *uaddr, int size, bool multipage)
{
	...
}

static inline int fault_in_pages_writeable(char __user *uaddr, int size)
{
	return __fault_in_pages_writeable(uaddr, size, false);
}

static inline int fault_in_multipages_writeable(char __user *uaddr, int size)
{
	return __fault_in_pages_writeable(uaddr, size, true);
}

which I don't think is worth bothering with given the very small number
of callsites.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/