Re: [Intel-gfx] [PATCH 2/2] drm/i915: properly prefault for pread/pwrite

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Nov 03, 2011 at 02:06:55PM -0700, Keith Packard wrote:
> On Mon, 24 Oct 2011 00:11:57 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > This patch only fixes things up so that we prefault the entire page range
> > and not just the first PAGE_SIZE bytes (i.e. at most 2 pages). So I don't
> > see the risk of extending the current behaviour to all pages. Userspace
> > can already see these zero writes, but only when doing something stupid.
> 
> When we posted a patch to instead fix fault_in_pages_writeable, Andrew
> complained that we'd have modified memory even on a short read, which
> wasn't considered polite. Could we read/write the same value and avoid
> that problem?

Hm, that might be a solution. My current plan was to ditch the prefault
for writing to userspace and beat my pwrite/pread patches into shape for
submission - the bug report only concerns -EFAULT due to handing in a gtt
mapping in pwrite, afaik.

otoh gem objects never change their size and we return -EINVAL if the read
would go past the end of it. And userspace should also never see short
reads due to signals, because the libdrm ioctl automatically restarts the
syscall - and that part is more or less abi. So in practice for our case,
I think it just doesn't matter because userspace really only sees these
zero writes when doing something buggy.

> Also, we should be fixing fault_in_pages_* going forward, rather than
> kludging in more code. And, we'd get to remove the version in ntfs,
> which should end in a patch that removes more code than it adds...

Hm, haven't noticed the version in nfs. The version in pagemap.h does what
all the other users of it want, namely prefault at most PAGE_SIZE bytes
(from at most two pages, in case the user pointer crosses a page boundary).
Which is why I've left it as is.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48