* [PATCH 1/2] io-mapping: ensure io_mapping_map_atomic _is_ atomic @ 2011-09-28 9:57 Daniel Vetter 2011-09-28 9:57 ` [PATCH 2/2] drm/i915: properly prefault for pread/pwrite Daniel Vetter 0 siblings, 1 reply; 14+ messages in thread From: Daniel Vetter @ 2011-09-28 9:57 UTC (permalink / raw) To: intel-gfx; +Cc: Daniel Vetter, stable For the !HAVE_ATOMIC_IOMAP case the stub functions did not call pagefault_disable/_enable. The i915 driver relies on the map actually being atomic, otherwise it can deadlock with it's own pagefault handler in the gtt pwrite fastpath. This is exercised by gem_mmap_gtt from the intel-gpu-toosl gem testsuite. v2: Chris Wilson noted the lack of an include. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=38115 Cc: stable@kernel.org Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> --- include/linux/io-mapping.h | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/include/linux/io-mapping.h b/include/linux/io-mapping.h index 8cdcc2a1..1feeb52 100644 --- a/include/linux/io-mapping.h +++ b/include/linux/io-mapping.h @@ -117,6 +117,8 @@ io_mapping_unmap(void __iomem *vaddr) #else +#include <linux/uaccess.h> + /* this struct isn't actually defined anywhere */ struct io_mapping; @@ -138,12 +140,14 @@ static inline void __iomem * io_mapping_map_atomic_wc(struct io_mapping *mapping, unsigned long offset) { + pagefault_disable(); return ((char __force __iomem *) mapping) + offset; } static inline void io_mapping_unmap_atomic(void __iomem *vaddr) { + pagefault_enable(); } /* Non-atomic map/unmap */ -- 1.7.6.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/2] drm/i915: properly prefault for pread/pwrite 2011-09-28 9:57 [PATCH 1/2] io-mapping: ensure io_mapping_map_atomic _is_ atomic Daniel Vetter @ 2011-09-28 9:57 ` Daniel Vetter 2011-09-28 11:24 ` Chris Wilson 2011-11-17 20:52 ` Keith Packard 0 siblings, 2 replies; 14+ messages in thread From: Daniel Vetter @ 2011-09-28 9:57 UTC (permalink / raw) To: intel-gfx; +Cc: Daniel Vetter, stable The helper functions used are designed for pagecache io and splice, i.e. they prefault at most PAGE_SIZE bytes spanning at most 2 pages. pread/pwrite want to write/read much more to avoid dropping the struct_mutex lock in between. So write our helper function to prefault. We're the only user of these pagemap.h helpers that want this behaviour, so keep these new helpers private. Based on a patch by Chris Wilson. In addition to his approach this alos tries to prefault the last page in case the user address range crosses a page boundary at the end (and hence might sit on n+1 pages for at most n*PAGE_SIZE of date). As a nice side-effect this rather reliably papers over the current code's inability to handle non-struct page-backed user memory in the slow paths. Because the real fix is grossly invasive, I think this patch is the right thing for backporting. Cc: stable@kernel.org Cc: Chris Wilson <chris@chris-wilson.co.uk> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=38115 Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_gem.c | 59 ++++++++++++++++++++++++++++++++++++-- 1 files changed, 55 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 5f0f46e..42dc922 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -503,6 +503,32 @@ out: return ret; } +static inline int __prefault_writeable(char __user *uaddr, int size) +{ + int ret; + char __user *end = uaddr + size - 1; + + if (unlikely(size == 0)) + return 0; + + /* + * Writing zeroes into userspace here is OK, because we know that if + * the zero gets there, we'll be overwriting it. + */ + while (uaddr <= end) { + ret = __put_user(0, uaddr); + if (ret != 0) + return ret; + uaddr += PAGE_SIZE; + } + if (ret == 0) { + if (((unsigned long)uaddr & PAGE_MASK) != + ((unsigned long)end & PAGE_MASK)) + ret = __put_user(0, end); + } + return ret; +} + /** * Reads data from the object referenced by handle. * @@ -524,8 +550,8 @@ i915_gem_pread_ioctl(struct drm_device *dev, void *data, args->size)) return -EFAULT; - ret = fault_in_pages_writeable((char __user *)(uintptr_t)args->data_ptr, - args->size); + ret = __prefault_writeable((char __user *)(uintptr_t)args->data_ptr, + args->size); if (ret) return -EFAULT; @@ -943,6 +969,31 @@ out: return ret; } +static inline int __prefault_readable(const char __user *uaddr, int size) +{ + volatile char c; + int ret; + const char __user *end = uaddr + size - 1; + + if (unlikely(size == 0)) + return 0; + + while (uaddr <= end) { + ret = __get_user(c, uaddr); + if (ret != 0) + return ret; + uaddr += PAGE_SIZE; + } + if (ret == 0) { + if (((unsigned long)uaddr & PAGE_MASK) != + ((unsigned long)end & PAGE_MASK)) { + ret = __get_user(c, end); + (void)c; + } + } + return ret; +} + /** * Writes data to the object referenced by handle. * @@ -964,8 +1015,8 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data, args->size)) return -EFAULT; - ret = fault_in_pages_readable((char __user *)(uintptr_t)args->data_ptr, - args->size); + ret = __prefault_readable((char __user *)(uintptr_t)args->data_ptr, + args->size); if (ret) return -EFAULT; -- 1.7.6.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] drm/i915: properly prefault for pread/pwrite 2011-09-28 9:57 ` [PATCH 2/2] drm/i915: properly prefault for pread/pwrite Daniel Vetter @ 2011-09-28 11:24 ` Chris Wilson 2011-10-23 10:18 ` Daniel Vetter 2011-11-17 20:52 ` Keith Packard 1 sibling, 1 reply; 14+ messages in thread From: Chris Wilson @ 2011-09-28 11:24 UTC (permalink / raw) To: intel-gfx; +Cc: Daniel Vetter, stable On Wed, 28 Sep 2011 11:57:24 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > The helper functions used are designed for pagecache io and splice, > i.e. they prefault at most PAGE_SIZE bytes spanning at most 2 pages. > > pread/pwrite want to write/read much more to avoid dropping the > struct_mutex lock in between. So write our helper function to prefault. > We're the only user of these pagemap.h helpers that want this behaviour, > so keep these new helpers private. > > Based on a patch by Chris Wilson. In addition to his approach this > alos tries to prefault the last page in case the user address range > crosses a page boundary at the end (and hence might sit on n+1 pages > for at most n*PAGE_SIZE of date). > > As a nice side-effect this rather reliably papers over the current > code's inability to handle non-struct page-backed user memory in the > slow paths. Because the real fix is grossly invasive, I think this > patch is the right thing for backporting. > > Cc: stable@kernel.org > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=38115 > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/i915_gem.c | 59 ++++++++++++++++++++++++++++++++++++-- > 1 files changed, 55 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 5f0f46e..42dc922 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -503,6 +503,32 @@ out: > return ret; > } > > +static inline int __prefault_writeable(char __user *uaddr, int size) > +{ > + int ret; > + char __user *end = uaddr + size - 1; > + > + if (unlikely(size == 0)) > + return 0; > + > + /* > + * Writing zeroes into userspace here is OK, because we know that if > + * the zero gets there, we'll be overwriting it. > + */ > + while (uaddr <= end) { > + ret = __put_user(0, uaddr); > + if (ret != 0) > + return ret; > + uaddr += PAGE_SIZE; > + } > + if (ret == 0) { > + if (((unsigned long)uaddr & PAGE_MASK) != > + ((unsigned long)end & PAGE_MASK)) This is a little ugly. Perhaps, #define page_align(addr) ((unsigned long)addr & PAGE_MASK) if (page_align(uaddr) != page_align(end)) Otherwise, both are Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] drm/i915: properly prefault for pread/pwrite 2011-09-28 11:24 ` Chris Wilson @ 2011-10-23 10:18 ` Daniel Vetter 2011-10-23 10:19 ` Daniel Vetter 2011-10-23 19:23 ` Keith Packard 0 siblings, 2 replies; 14+ messages in thread From: Daniel Vetter @ 2011-10-23 10:18 UTC (permalink / raw) To: Chris Wilson; +Cc: Daniel Vetter, intel-gfx, stable Hi Keith, This patch isn't in your -next pull. This papers over a spurious -EFAULT in the pwrite/pread paths that actually gets hit in the wild. The real fix in the form of a almost complete rewrite of the pwrite/pread paths won't be ready for 3.2. Do you want me to implement Chris' beautification suggestion? The code as-is is copied over from the pagecache prefault helper. Otherwise please consider merging for 3.2. Yours, Daniel On Wed, Sep 28, 2011 at 12:24:04PM +0100, Chris Wilson wrote: > > + if (ret == 0) { > > + if (((unsigned long)uaddr & PAGE_MASK) != > > + ((unsigned long)end & PAGE_MASK)) > This is a little ugly. Perhaps, > > #define page_align(addr) ((unsigned long)addr & PAGE_MASK) > if (page_align(uaddr) != page_align(end)) > > Otherwise, both are > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre -- Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] drm/i915: properly prefault for pread/pwrite 2011-10-23 10:18 ` Daniel Vetter @ 2011-10-23 10:19 ` Daniel Vetter 2011-10-23 19:23 ` Keith Packard 1 sibling, 0 replies; 14+ messages in thread From: Daniel Vetter @ 2011-10-23 10:19 UTC (permalink / raw) To: Chris Wilson, Keith Packard; +Cc: Daniel Vetter, intel-gfx, stable Oosp, forgotten to actually put Keith on the To: ... not enough coffee, yet. -Daniel On Sun, Oct 23, 2011 at 12:18:30PM +0200, Daniel Vetter wrote: > Hi Keith, > > This patch isn't in your -next pull. This papers over a spurious -EFAULT > in the pwrite/pread paths that actually gets hit in the wild. The real fix > in the form of a almost complete rewrite of the pwrite/pread paths won't > be ready for 3.2. > > Do you want me to implement Chris' beautification suggestion? The code > as-is is copied over from the pagecache prefault helper. Otherwise please > consider merging for 3.2. > > Yours, Daniel > > On Wed, Sep 28, 2011 at 12:24:04PM +0100, Chris Wilson wrote: > > > + if (ret == 0) { > > > + if (((unsigned long)uaddr & PAGE_MASK) != > > > + ((unsigned long)end & PAGE_MASK)) > > This is a little ugly. Perhaps, > > > > #define page_align(addr) ((unsigned long)addr & PAGE_MASK) > > if (page_align(uaddr) != page_align(end)) > > > > Otherwise, both are > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > -Chris > > > > -- > > Chris Wilson, Intel Open Source Technology Centre > > -- > Daniel Vetter > Mail: daniel@ffwll.ch > Mobile: +41 (0)79 365 57 48 -- Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] drm/i915: properly prefault for pread/pwrite 2011-10-23 10:18 ` Daniel Vetter 2011-10-23 10:19 ` Daniel Vetter @ 2011-10-23 19:23 ` Keith Packard 2011-10-23 22:11 ` Daniel Vetter 1 sibling, 1 reply; 14+ messages in thread From: Keith Packard @ 2011-10-23 19:23 UTC (permalink / raw) To: Daniel Vetter, Chris Wilson; +Cc: Daniel Vetter, intel-gfx, stable [-- Attachment #1.1: Type: text/plain, Size: 609 bytes --] On Sun, 23 Oct 2011 12:18:30 +0200, Daniel Vetter <daniel@ffwll.ch> wrote: > Hi Keith, > > This patch isn't in your -next pull. This papers over a spurious -EFAULT > in the pwrite/pread paths that actually gets hit in the wild. The real fix > in the form of a almost complete rewrite of the pwrite/pread paths won't > be ready for 3.2. We had several comments wondering whether writing zeros was OK as this occurs before some potential error returns that should leave the buffer unmodified. I didn't have a better suggestion, but that seems pretty sketchy to me. -- keith.packard@intel.com [-- Attachment #1.2: Type: application/pgp-signature, Size: 827 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] drm/i915: properly prefault for pread/pwrite 2011-10-23 19:23 ` Keith Packard @ 2011-10-23 22:11 ` Daniel Vetter 2011-11-03 21:06 ` Keith Packard 0 siblings, 1 reply; 14+ messages in thread From: Daniel Vetter @ 2011-10-23 22:11 UTC (permalink / raw) To: Keith Packard; +Cc: Daniel Vetter, intel-gfx, stable On Sun, Oct 23, 2011 at 12:23:47PM -0700, Keith Packard wrote: > On Sun, 23 Oct 2011 12:18:30 +0200, Daniel Vetter <daniel@ffwll.ch> wrote: > > Hi Keith, > > > > This patch isn't in your -next pull. This papers over a spurious -EFAULT > > in the pwrite/pread paths that actually gets hit in the wild. The real fix > > in the form of a almost complete rewrite of the pwrite/pread paths won't > > be ready for 3.2. > > We had several comments wondering whether writing zeros was OK as this > occurs before some potential error returns that should leave the buffer > unmodified. I didn't have a better suggestion, but that seems pretty > sketchy to me. 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. -Daniel -- Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Intel-gfx] [PATCH 2/2] drm/i915: properly prefault for pread/pwrite 2011-10-23 22:11 ` Daniel Vetter @ 2011-11-03 21:06 ` Keith Packard 0 siblings, 0 replies; 14+ messages in thread From: Keith Packard @ 2011-11-03 21:06 UTC (permalink / raw) To: Daniel Vetter Cc: Daniel Vetter, Chris Wilson, Daniel Vetter, intel-gfx, stable, linux-kernel, Morton, Andrew [-- Attachment #1: Type: text/plain, Size: 857 bytes --] 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? 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... -- keith.packard@intel.com [-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] drm/i915: properly prefault for pread/pwrite @ 2011-11-03 21:06 ` Keith Packard 0 siblings, 0 replies; 14+ messages in thread From: Keith Packard @ 2011-11-03 21:06 UTC (permalink / raw) To: Daniel Vetter Cc: Daniel Vetter, intel-gfx, linux-kernel, Morton, Andrew, stable [-- Attachment #1.1: Type: text/plain, Size: 857 bytes --] 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? 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... -- keith.packard@intel.com [-- Attachment #1.2: Type: application/pgp-signature, Size: 827 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Intel-gfx] [PATCH 2/2] drm/i915: properly prefault for pread/pwrite 2011-11-03 21:06 ` Keith Packard @ 2011-11-03 22:10 ` Daniel Vetter -1 siblings, 0 replies; 14+ messages in thread From: Daniel Vetter @ 2011-11-03 22:10 UTC (permalink / raw) To: Keith Packard Cc: Daniel Vetter, Chris Wilson, Daniel Vetter, intel-gfx, stable, linux-kernel, Morton, Andrew 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] drm/i915: properly prefault for pread/pwrite @ 2011-11-03 22:10 ` Daniel Vetter 0 siblings, 0 replies; 14+ messages in thread From: Daniel Vetter @ 2011-11-03 22:10 UTC (permalink / raw) To: Keith Packard Cc: Daniel Vetter, intel-gfx, linux-kernel, Morton, Andrew, stable 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] drm/i915: properly prefault for pread/pwrite 2011-09-28 9:57 ` [PATCH 2/2] drm/i915: properly prefault for pread/pwrite Daniel Vetter 2011-09-28 11:24 ` Chris Wilson @ 2011-11-17 20:52 ` Keith Packard 2011-11-18 9:12 ` Daniel Vetter 1 sibling, 1 reply; 14+ messages in thread From: Keith Packard @ 2011-11-17 20:52 UTC (permalink / raw) To: intel-gfx; +Cc: Daniel Vetter [-- Attachment #1.1: Type: text/plain, Size: 495 bytes --] On Wed, 28 Sep 2011 11:57:24 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > + char __user *end = uaddr + size - 1; ... > + if (ret == 0) { > + if (((unsigned long)uaddr & PAGE_MASK) != > + ((unsigned long)end & PAGE_MASK)) > + ret = __put_user(0, end); > + } This is wrong -- if size == PAGE_SIZE, then we'll be doing an extra write at 'end' every time (and, I imagine that's a common case). And, you mentioned a 'better' fix? -- keith.packard@intel.com [-- Attachment #1.2: Type: application/pgp-signature, Size: 827 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] drm/i915: properly prefault for pread/pwrite 2011-11-17 20:52 ` Keith Packard @ 2011-11-18 9:12 ` Daniel Vetter 2011-11-18 17:42 ` Keith Packard 0 siblings, 1 reply; 14+ messages in thread From: Daniel Vetter @ 2011-11-18 9:12 UTC (permalink / raw) To: Keith Packard; +Cc: intel-gfx On Thu, Nov 17, 2011 at 21:52, Keith Packard <keithp@keithp.com> wrote: > On Wed, 28 Sep 2011 11:57:24 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > >> + char __user *end = uaddr + size - 1; > > ... > >> + if (ret == 0) { >> + if (((unsigned long)uaddr & PAGE_MASK) != >> + ((unsigned long)end & PAGE_MASK)) >> + ret = __put_user(0, end); >> + } > > This is wrong -- if size == PAGE_SIZE, then we'll be doing an extra > write at 'end' every time (and, I imagine that's a common case). We want to prefault the last byte if the pfn of the last prefault address doesn't match the pfn of the last byte of the userspace address range. Only happens when userspace hands in badly aligned address, not every time. I've rechecked and I think the code actually does what I want it to do. > And, you mentioned a 'better' fix? Chris was despised at the lack of beauty of the code and I agree. Due to the ppgtt trip to Poland I haven't gotten around to do it actually. Actually I've just noticed that this is might be the old pwrite/pread series (mail here still sucks). The new one fixes up the prefault helpers in pagemap.h (instead of reinventing the wheel for i915.ko), but they have the same issue of profound ugliness. -Daniel -- Daniel Vetter daniel.vetter@ffwll.ch - +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] drm/i915: properly prefault for pread/pwrite 2011-11-18 9:12 ` Daniel Vetter @ 2011-11-18 17:42 ` Keith Packard 0 siblings, 0 replies; 14+ messages in thread From: Keith Packard @ 2011-11-18 17:42 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx [-- Attachment #1.1: Type: text/plain, Size: 1119 bytes --] On Fri, 18 Nov 2011 10:12:38 +0100, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > We want to prefault the last byte if the pfn of the last prefault > address doesn't match the pfn of the last byte of the userspace > address range. Only happens when userspace hands in badly aligned > address, not every time. I've rechecked and I think the code actually > does what I want it to do. Sorry, I was confused by PAGE_MASK (again); assumed it was 0xfff instead of ~0xfff. > Chris was despised at the lack of beauty of the code and I agree. Due > to the ppgtt trip to Poland I haven't gotten around to do it actually. > Actually I've just noticed that this is might be the old pwrite/pread > series (mail here still sucks). The new one fixes up the prefault > helpers in pagemap.h (instead of reinventing the wheel for i915.ko), > but they have the same issue of profound ugliness. Right, that was in a different mail thread and is still awaiting cleanups it seems. Sounds like you're having adventures in Poland at least. I'll pend this until you've gotten it finished and ready to merge. -- keith.packard@intel.com [-- Attachment #1.2: Type: application/pgp-signature, Size: 827 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2011-11-18 17:43 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-09-28 9:57 [PATCH 1/2] io-mapping: ensure io_mapping_map_atomic _is_ atomic Daniel Vetter 2011-09-28 9:57 ` [PATCH 2/2] drm/i915: properly prefault for pread/pwrite Daniel Vetter 2011-09-28 11:24 ` Chris Wilson 2011-10-23 10:18 ` Daniel Vetter 2011-10-23 10:19 ` Daniel Vetter 2011-10-23 19:23 ` Keith Packard 2011-10-23 22:11 ` Daniel Vetter 2011-11-03 21:06 ` [Intel-gfx] " Keith Packard 2011-11-03 21:06 ` Keith Packard 2011-11-03 22:10 ` [Intel-gfx] " Daniel Vetter 2011-11-03 22:10 ` Daniel Vetter 2011-11-17 20:52 ` Keith Packard 2011-11-18 9:12 ` Daniel Vetter 2011-11-18 17:42 ` Keith Packard
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.