On Thu, Feb 23, 2012 at 02:36:58PM -0800, Andrew Morton wrote:
> On Thu, 16 Feb 2012 13:01:36 +0100
> Daniel Vetter <daniel.vet...@ffwll.ch> wrote:
> 
> > drm/i915 wants to read/write more than one page in its fastpath
> > and hence needs to prefault more than PAGE_SIZE bytes.
> > 
> > I've checked the callsites and they all already clamp size when
> > calling fault_in_pages_* to the same as for the subsequent
> > __copy_to|from_user and hence don't rely on the implicit clamping
> > to PAGE_SIZE.
> > 
> > Also kill a copy&pasted spurious space in both functions while at it.
> >
> > ...
> >
> > --- 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).

> >     if (ret == 0) {
> > -           char __user *end = uaddr + size - 1;
> > -
> >             /*
> >              * If the page was already mapped, this will get a cache miss
> >              * for sure, so try to avoid doing it.
> >              */
> > -           if (((unsigned long)uaddr & PAGE_MASK) !=
> > +           if (((unsigned long)uaddr & PAGE_MASK) ==
> >                             ((unsigned long)end & PAGE_MASK))
> 
> Maybe I'm having a dim day, but I don't immediately see why != got
> turned into ==.

Because of the loop uaddr will now point one page beyond the last
prefaulted page. To check whether end spilled into a new page we therefore
need to check whether uaddr and end are in the same pfn. Before uaddr
wasn't changed and hence the checking for a different pfn worked
correctly.

> Once we have this settled I'd suggest that the patch be carried in
> whatever-git-tree-needs-it.

Thanks for the comments.

Yours, Daniel
-- 
Daniel Vetter
Mail: dan...@ffwll.ch
Mobile: +41 (0)79 365 57 48
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Reply via email to