[PATCH] drm/i915: Do not invalidate obj->pages under mempressure

public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed

* [PATCH] drm/i915: Do not invalidate obj->pages under mempressure
@ 2015-01-14 20:34 Chris Wilson
  2015-01-15  0:55 ` [Intel-gfx] " Daniel Vetter
                   ` (3 more replies)
  0 siblings, 4 replies; 18+ messages in thread
From: Chris Wilson @ 2015-01-14 20:34 UTC (permalink / raw)
  To: intel-gfx; +Cc: Chris Wilson, Sean V Kelley, stable

This (partially) reverts

commit 5537252b6b6d71fb1a8ed7395a8e5babf91953fd
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Tue Mar 25 13:23:06 2014 +0000

    drm/i915: Invalidate our pages under memory pressure

It appears given the right workload, that pages which are swapped out
more than once are incorrectly invalidated and discarded. I had presumed
that the swapin would mark the pages dirty again and so preserve them
against the next cycle of invalidation - that appears to be false, and
leads to memory corruption (even leak of stale pages to userspace).

Reported-by: Sean V Kelley <sean.v.kelley@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Sean V Kelley <sean.v.kelley@intel.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/i915_gem.c | 23 ++---------------------
 1 file changed, 2 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 4d453490596e..b06f051a73de 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1947,26 +1947,6 @@ i915_gem_object_truncate(struct drm_i915_gem_object *obj)
 	obj->madv = __I915_MADV_PURGED;
 }
 
-/* Try to discard unwanted pages */
-static void
-i915_gem_object_invalidate(struct drm_i915_gem_object *obj)
-{
-	struct address_space *mapping;
-
-	switch (obj->madv) {
-	case I915_MADV_DONTNEED:
-		i915_gem_object_truncate(obj);
-	case __I915_MADV_PURGED:
-		return;
-	}
-
-	if (obj->base.filp == NULL)
-		return;
-
-	mapping = file_inode(obj->base.filp)->i_mapping,
-	invalidate_mapping_pages(mapping, 0, (loff_t)-1);
-}
-
 static void
 i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj)
 {
@@ -2029,7 +2009,8 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
 	ops->put_pages(obj);
 	obj->pages = NULL;
 
-	i915_gem_object_invalidate(obj);
+	if (i915_gem_object_is_purgeable(obj))
+		i915_gem_object_truncate(obj);
 
 	return 0;
 }
-- 
2.1.4

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

* Re: [Intel-gfx] [PATCH] drm/i915: Do not invalidate obj->pages under mempressure
  2015-01-14 20:34 [PATCH] drm/i915: Do not invalidate obj->pages under mempressure Chris Wilson
@ 2015-01-15  0:55 ` Daniel Vetter
  2015-01-15  1:19   ` Sean V Kelley
  2015-01-15  9:38   ` Chris Wilson
  2015-01-15  2:11 ` shuang.he
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 18+ messages in thread
From: Daniel Vetter @ 2015-01-15  0:55 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx, stable

On Wed, Jan 14, 2015 at 08:34:31PM +0000, Chris Wilson wrote:
> This (partially) reverts
>
> commit 5537252b6b6d71fb1a8ed7395a8e5babf91953fd
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Tue Mar 25 13:23:06 2014 +0000
>
>     drm/i915: Invalidate our pages under memory pressure
>
> It appears given the right workload, that pages which are swapped out
> more than once are incorrectly invalidated and discarded. I had presumed
> that the swapin would mark the pages dirty again and so preserve them
> against the next cycle of invalidation - that appears to be false, and
> leads to memory corruption (even leak of stale pages to userspace).
>
> Reported-by: Sean V Kelley <sean.v.kelley@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Sean V Kelley <sean.v.kelley@intel.com>
> Cc: stable@vger.kernel.org

Hm, scary. Do we have a testcase for this (might need a correctness
version for some of the swap thrashing tests maybe).
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem.c | 23 ++---------------------
>  1 file changed, 2 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 4d453490596e..b06f051a73de 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1947,26 +1947,6 @@ i915_gem_object_truncate(struct drm_i915_gem_object *obj)
>   obj->madv = __I915_MADV_PURGED;
>  }
>
> -/* Try to discard unwanted pages */
> -static void
> -i915_gem_object_invalidate(struct drm_i915_gem_object *obj)
> -{
> - struct address_space *mapping;
> -
> - switch (obj->madv) {
> - case I915_MADV_DONTNEED:
> - i915_gem_object_truncate(obj);
> - case __I915_MADV_PURGED:
> - return;
> - }
> -
> - if (obj->base.filp == NULL)
> - return;
> -
> - mapping = file_inode(obj->base.filp)->i_mapping,
> - invalidate_mapping_pages(mapping, 0, (loff_t)-1);
> -}
> -
>  static void
>  i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj)
>  {
> @@ -2029,7 +2009,8 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
>   ops->put_pages(obj);
>   obj->pages = NULL;
>
> - i915_gem_object_invalidate(obj);
> + if (i915_gem_object_is_purgeable(obj))
> + i915_gem_object_truncate(obj);
>
>   return 0;
>  }
> --
> 2.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

* Re: [Intel-gfx] [PATCH] drm/i915: Do not invalidate obj->pages under mempressure
  2015-01-15  0:55 ` [Intel-gfx] " Daniel Vetter
@ 2015-01-15  1:19   ` Sean V Kelley
  2015-01-15  9:38   ` Chris Wilson
  1 sibling, 0 replies; 18+ messages in thread
From: Sean V Kelley @ 2015-01-15  1:19 UTC (permalink / raw)
  To: Daniel Vetter, Chris Wilson; +Cc: intel-gfx, stable

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1



On 01/14/2015 04:55 PM, Daniel Vetter wrote:
> On Wed, Jan 14, 2015 at 08:34:31PM +0000, Chris Wilson wrote:
>> This (partially) reverts
>> 
>> commit 5537252b6b6d71fb1a8ed7395a8e5babf91953fd Author: Chris
>> Wilson <chris@chris-wilson.co.uk> Date:   Tue Mar 25 13:23:06
>> 2014 +0000
>> 
>> drm/i915: Invalidate our pages under memory pressure
>> 
>> It appears given the right workload, that pages which are swapped
>> out more than once are incorrectly invalidated and discarded. I
>> had presumed that the swapin would mark the pages dirty again and
>> so preserve them against the next cycle of invalidation - that
>> appears to be false, and leads to memory corruption (even leak of
>> stale pages to userspace).
>> 
>> Reported-by: Sean V Kelley <sean.v.kelley@intel.com> 
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Sean V
>> Kelley <sean.v.kelley@intel.com> Cc: stable@vger.kernel.org
> 
> Hm, scary. Do we have a testcase for this (might need a
> correctness version for some of the swap thrashing tests maybe). 
> -Daniel

I can put together a test case we use with a media runtime workload
for an EU that we could add to igt.  Also this appears to be something
that has been also encountered on BDW during an Android kernel team
port that from what I can tell was patched locally (internally) and
not shared upstream.  I stumbled upon it only after getting Chris'
help with this patch.

Sean


> 
>> --- drivers/gpu/drm/i915/i915_gem.c | 23 ++--------------------- 
>> 1 file changed, 2 insertions(+), 21 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
>> b/drivers/gpu/drm/i915/i915_gem.c index
>> 4d453490596e..b06f051a73de 100644 ---
>> a/drivers/gpu/drm/i915/i915_gem.c +++
>> b/drivers/gpu/drm/i915/i915_gem.c @@ -1947,26 +1947,6 @@
>> i915_gem_object_truncate(struct drm_i915_gem_object *obj) 
>> obj->madv = __I915_MADV_PURGED; }
>> 
>> -/* Try to discard unwanted pages */ -static void 
>> -i915_gem_object_invalidate(struct drm_i915_gem_object *obj) -{ -
>> struct address_space *mapping; - - switch (obj->madv) { - case
>> I915_MADV_DONTNEED: - i915_gem_object_truncate(obj); - case
>> __I915_MADV_PURGED: - return; - } - - if (obj->base.filp ==
>> NULL) - return; - - mapping =
>> file_inode(obj->base.filp)->i_mapping, -
>> invalidate_mapping_pages(mapping, 0, (loff_t)-1); -} - static
>> void i915_gem_object_put_pages_gtt(struct drm_i915_gem_object
>> *obj) { @@ -2029,7 +2009,8 @@ i915_gem_object_put_pages(struct
>> drm_i915_gem_object *obj) ops->put_pages(obj); obj->pages =
>> NULL;
>> 
>> - i915_gem_object_invalidate(obj); + if
>> (i915_gem_object_is_purgeable(obj)) +
>> i915_gem_object_truncate(obj);
>> 
>> return 0; } -- 2.1.4
>> 
>> _______________________________________________ Intel-gfx mailing
>> list Intel-gfx@lists.freedesktop.org 
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBAgAGBQJUtxW9AAoJEGScDsMo8QYOlZkP/3IbGTlfw4aj37nlClMA+JpO
qHDbYi9evALt7NkiE5UcVz+LJwPZ/FLZR+Q8w5m/4NqPDVbGCDFDEovoJx/MKWgA
u6MuUrkuFC5KjGFiYZFuP/+EsXKMblNKRv4xWeWoPWgwlvd9ZYw1rSNVPLqkgIPu
RtvSJAsKdehvbl7Y4VyKDjKn9gUp58qKE9P/setx3u1vc/xbufKRACkdp9QLDzBm
s0HjmgIkXRQr1n7L4tUmZiYB9igZnapjxzyX2FazTQhHQFthUMu7pVJaP6MYLuSJ
Dr9CidL/DC4r4sciaOUhLlA5rD0PJ5AMrS9ZN6wf50gN1JWYYIgdl51TLkqFMjou
EpTv442qzjO+Mi5mY8YMheruDV/voGU7mc/11i3Ng9kSBdkv7kfVhcnSLsJsGCxw
4mI3fHOdSySIG+ZgM5Fw7XmFpXypbcqTZ99qbQkMoyQoXtbn96s1wCWp4j3qHHQ1
hQN6lgU3eb35gafmJ18qtEYouUahtRtB1SHOppGiLMuEaFcqr4hvFoq7fDl7VtQa
RqnoOyDcwUEht7btPLaRcnyd7kUl0fTcwmTbeFLo5hrOuv5Ke08ms9aMfAvk+cR7
mYO9OzxkvUJAgjPn9yCLR70fuDVGQa3rbM31LY6QBEGxfvjG2qPe2eFw5XdBkgTV
iCREv8mvFVotVg0vFAJB
=RERA
-----END PGP SIGNATURE-----

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

* Re: [Intel-gfx] [PATCH] drm/i915: Do not invalidate obj->pages under mempressure
  2015-01-15  0:55 ` [Intel-gfx] " Daniel Vetter
  2015-01-15  1:19   ` Sean V Kelley
@ 2015-01-15  9:38   ` Chris Wilson
  1 sibling, 0 replies; 18+ messages in thread
From: Chris Wilson @ 2015-01-15  9:38 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx, stable

On Thu, Jan 15, 2015 at 01:55:52AM +0100, Daniel Vetter wrote:
> On Wed, Jan 14, 2015 at 08:34:31PM +0000, Chris Wilson wrote:
> > This (partially) reverts
> >
> > commit 5537252b6b6d71fb1a8ed7395a8e5babf91953fd
> > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > Date:   Tue Mar 25 13:23:06 2014 +0000
> >
> >     drm/i915: Invalidate our pages under memory pressure
> >
> > It appears given the right workload, that pages which are swapped out
> > more than once are incorrectly invalidated and discarded. I had presumed
> > that the swapin would mark the pages dirty again and so preserve them
> > against the next cycle of invalidation - that appears to be false, and
> > leads to memory corruption (even leak of stale pages to userspace).
> >
> > Reported-by: Sean V Kelley <sean.v.kelley@intel.com>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Sean V Kelley <sean.v.kelley@intel.com>
> > Cc: stable@vger.kernel.org
> 
> Hm, scary. Do we have a testcase for this (might need a correctness
> version for some of the swap thrashing tests maybe).

Right, based on my theory about how this screws up, a few passes through
gem_tiled_swapping should be enough to hit the invalidate.

fill_bo -> GTT write, obj->dirty = true -> shinker -> GTT unbind, obj->dirty = false
-> check_bo -> GTT read, obj->dirty = false -> shrinker -> GTT unbind, invalidate
without us calling page_set_dirty()

i.e. a few loops of check_bo should be enough to cycle every object
through swap a couple of times and for subsequent swapin/swapouts not to
cause us to explicity mark the pages as dirty, thus prone to
invalidate_mapping_range() actually removing unwanted pages.

The error does not not show up with igt/gem_tiled_swapping.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

* Re: [PATCH] drm/i915: Do not invalidate obj->pages under mempressure
  2015-01-14 20:34 [PATCH] drm/i915: Do not invalidate obj->pages under mempressure Chris Wilson
  2015-01-15  0:55 ` [Intel-gfx] " Daniel Vetter
@ 2015-01-15  2:11 ` shuang.he
  2015-01-15 19:36 ` Daniel Vetter
  2015-02-09 16:43 ` Daniel Vetter
  3 siblings, 0 replies; 18+ messages in thread
From: shuang.he @ 2015-01-15  2:11 UTC (permalink / raw)
  To: shuang.he, ethan.gao, intel-gfx, chris

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5582
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  353/353              353/353
ILK                                  200/200              200/200
SNB                                  400/422              400/422
IVB                 -43              487/487              444/487
BYT                                  296/296              296/296
HSW                                  486/508              486/508
BDW                 -1              402/402              401/402
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 IVB  igt_kms_cursor_crc_cursor-128x128-onscreen      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_cursor_crc_cursor-128x128-random      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_cursor_crc_cursor-128x128-sliding      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_cursor_crc_cursor-256x256-offscreen      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_cursor_crc_cursor-256x256-onscreen      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_cursor_crc_cursor-256x256-sliding      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_cursor_crc_cursor-64x64-offscreen      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_cursor_crc_cursor-64x64-onscreen      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_cursor_crc_cursor-64x64-random      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_cursor_crc_cursor-64x64-sliding      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_cursor_crc_cursor-size-change      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_fence_pin_leak      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_mmio_vs_cs_flip_setcrtc_vs_cs_flip      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_mmio_vs_cs_flip_setplane_vs_cs_flip      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_pipe_crc_basic_read-crc-pipe-A      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_pipe_crc_basic_read-crc-pipe-A-frame-sequence      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_pipe_crc_basic_read-crc-pipe-B      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_pipe_crc_basic_read-crc-pipe-B-frame-sequence      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_pipe_crc_basic_read-crc-pipe-C      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_pipe_crc_basic_read-crc-pipe-C-frame-sequence      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_plane_plane-panning-bottom-right-pipe-A-plane-1      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_plane_plane-panning-bottom-right-pipe-A-plane-2      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_plane_plane-panning-bottom-right-pipe-B-plane-1      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_plane_plane-panning-bottom-right-pipe-B-plane-2      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_plane_plane-panning-bottom-right-pipe-C-plane-2      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_plane_plane-panning-top-left-pipe-A-plane-1      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_plane_plane-panning-top-left-pipe-A-plane-2      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_plane_plane-panning-top-left-pipe-B-plane-1      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_plane_plane-panning-top-left-pipe-B-plane-2      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_plane_plane-panning-top-left-pipe-C-plane-2      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_plane_plane-position-covered-pipe-A-plane-1      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_plane_plane-position-covered-pipe-A-plane-2      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_plane_plane-position-covered-pipe-B-plane-1      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_plane_plane-position-covered-pipe-B-plane-2      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_plane_plane-position-covered-pipe-C-plane-2      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_plane_plane-position-hole-pipe-A-plane-1      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_plane_plane-position-hole-pipe-A-plane-2      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_plane_plane-position-hole-pipe-B-plane-1      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_plane_plane-position-hole-pipe-B-plane-2      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_plane_plane-position-hole-pipe-C-plane-1      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_plane_plane-position-hole-pipe-C-plane-2      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_rotation_crc_primary-rotation      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
 IVB  igt_kms_rotation_crc_sprite-rotation      DMESG_WARN(1, M4)PASS(1, M34)      DMESG_WARN(1, M4)
*BDW  igt_gem_concurrent_blit_gtt-rcs-early-read-interruptible      PASS(3, M30)      DMESG_WARN(1, M30)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH] drm/i915: Do not invalidate obj->pages under mempressure
  2015-01-14 20:34 [PATCH] drm/i915: Do not invalidate obj->pages under mempressure Chris Wilson
  2015-01-15  0:55 ` [Intel-gfx] " Daniel Vetter
  2015-01-15  2:11 ` shuang.he
@ 2015-01-15 19:36 ` Daniel Vetter
  2015-01-15 20:44   ` [Intel-gfx] " Chris Wilson
  2015-02-09 16:43 ` Daniel Vetter
  3 siblings, 1 reply; 18+ messages in thread
From: Daniel Vetter @ 2015-01-15 19:36 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx, stable

On Wed, Jan 14, 2015 at 9:34 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> This (partially) reverts
>
> commit 5537252b6b6d71fb1a8ed7395a8e5babf91953fd
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Tue Mar 25 13:23:06 2014 +0000
>
>     drm/i915: Invalidate our pages under memory pressure

Shouldn't we also revert the hunk in i915_gem_free_objects? Without
the truncate vs. invalidate disdinction it seems to have lost it's
reason for existence ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH] drm/i915: Do not invalidate obj->pages under mempressure
  2015-01-15 19:36 ` Daniel Vetter
@ 2015-01-15 20:44   ` Chris Wilson
  2015-01-17  4:05     ` Daniel Vetter
  0 siblings, 1 reply; 18+ messages in thread
From: Chris Wilson @ 2015-01-15 20:44 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx, stable

On Thu, Jan 15, 2015 at 08:36:15PM +0100, Daniel Vetter wrote:
> On Wed, Jan 14, 2015 at 9:34 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > This (partially) reverts
> >
> > commit 5537252b6b6d71fb1a8ed7395a8e5babf91953fd
> > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > Date:   Tue Mar 25 13:23:06 2014 +0000
> >
> >     drm/i915: Invalidate our pages under memory pressure
> 
> Shouldn't we also revert the hunk in i915_gem_free_objects? Without
> the truncate vs. invalidate disdinction it seems to have lost it's
> reason for existence ...

No, setting MADV_DONTNEED has other nice properties during put_pages() -
I think it is useful in its own right, for example that is where my page
stealing code goes...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

* Re: [PATCH] drm/i915: Do not invalidate obj->pages under mempressure
  2015-01-15 20:44   ` [Intel-gfx] " Chris Wilson
@ 2015-01-17  4:05     ` Daniel Vetter
  2015-02-08 23:27       ` [Intel-gfx] " Sean V Kelley
  0 siblings, 1 reply; 18+ messages in thread
From: Daniel Vetter @ 2015-01-17  4:05 UTC (permalink / raw)
  To: Chris Wilson, Daniel Vetter, intel-gfx, stable

On Thu, Jan 15, 2015 at 08:44:00PM +0000, Chris Wilson wrote:
> On Thu, Jan 15, 2015 at 08:36:15PM +0100, Daniel Vetter wrote:
> > On Wed, Jan 14, 2015 at 9:34 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > This (partially) reverts
> > >
> > > commit 5537252b6b6d71fb1a8ed7395a8e5babf91953fd
> > > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > > Date:   Tue Mar 25 13:23:06 2014 +0000
> > >
> > >     drm/i915: Invalidate our pages under memory pressure
> >
> > Shouldn't we also revert the hunk in i915_gem_free_objects? Without
> > the truncate vs. invalidate disdinction it seems to have lost it's
> > reason for existence ...
>
> No, setting MADV_DONTNEED has other nice properties during put_pages() -
> I think it is useful in its own right, for example that is where my page
> stealing code goes...

Well right now I can't make sense of this bit any more (tbh I didn't with
the other code either, but overlooked that while reviewing). When it's
just there for future work but atm dead code I prefer for it to get
removed.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH] drm/i915: Do not invalidate obj->pages under mempressure
  2015-01-17  4:05     ` Daniel Vetter
@ 2015-02-08 23:27       ` Sean V Kelley
  2015-02-09 16:46         ` Chris Wilson
  0 siblings, 1 reply; 18+ messages in thread
From: Sean V Kelley @ 2015-02-08 23:27 UTC (permalink / raw)
  To: Daniel Vetter, Chris Wilson, intel-gfx, stable



On 01/16/2015 08:05 PM, Daniel Vetter wrote:
> On Thu, Jan 15, 2015 at 08:44:00PM +0000, Chris Wilson wrote:
>> On Thu, Jan 15, 2015 at 08:36:15PM +0100, Daniel Vetter wrote:
>>> On Wed, Jan 14, 2015 at 9:34 PM, Chris Wilson
>>> <chris@chris-wilson.co.uk> wrote:
>>>> This (partially) reverts
>>>> 
>>>> commit 5537252b6b6d71fb1a8ed7395a8e5babf91953fd Author: Chris
>>>> Wilson <chris@chris-wilson.co.uk> Date:   Tue Mar 25 13:23:06
>>>> 2014 +0000
>>>> 
>>>> drm/i915: Invalidate our pages under memory pressure
>>> 
>>> Shouldn't we also revert the hunk in i915_gem_free_objects?
>>> Without the truncate vs. invalidate disdinction it seems to
>>> have lost it's reason for existence ...
>> 
>> No, setting MADV_DONTNEED has other nice properties during
>> put_pages() - I think it is useful in its own right, for example
>> that is where my page stealing code goes...
> 
> Well right now I can't make sense of this bit any more (tbh I
> didn't with the other code either, but overlooked that while
> reviewing). When it's just there for future work but atm dead code
> I prefer for it to get removed. -Daniel


So can we also revert the hunk in i915_gem_free_objects?  I would like
to get this patch merged, it looks like that is the primary concern.

Thanks,

Sean

> 

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

* Re: [Intel-gfx] [PATCH] drm/i915: Do not invalidate obj->pages under mempressure
  2015-02-08 23:27       ` [Intel-gfx] " Sean V Kelley
@ 2015-02-09 16:46         ` Chris Wilson
  2015-02-09 18:31           ` Sean V Kelley
  2015-02-11  0:55           ` Sean V Kelley
  0 siblings, 2 replies; 18+ messages in thread
From: Chris Wilson @ 2015-02-09 16:46 UTC (permalink / raw)
  To: Sean V Kelley; +Cc: Daniel Vetter, intel-gfx, stable

On Sun, Feb 08, 2015 at 03:27:13PM -0800, Sean V Kelley wrote:
> 
> 
> On 01/16/2015 08:05 PM, Daniel Vetter wrote:
> > On Thu, Jan 15, 2015 at 08:44:00PM +0000, Chris Wilson wrote:
> >> On Thu, Jan 15, 2015 at 08:36:15PM +0100, Daniel Vetter wrote:
> >>> On Wed, Jan 14, 2015 at 9:34 PM, Chris Wilson
> >>> <chris@chris-wilson.co.uk> wrote:
> >>>> This (partially) reverts
> >>>> 
> >>>> commit 5537252b6b6d71fb1a8ed7395a8e5babf91953fd Author: Chris
> >>>> Wilson <chris@chris-wilson.co.uk> Date:   Tue Mar 25 13:23:06
> >>>> 2014 +0000
> >>>> 
> >>>> drm/i915: Invalidate our pages under memory pressure
> >>> 
> >>> Shouldn't we also revert the hunk in i915_gem_free_objects?
> >>> Without the truncate vs. invalidate disdinction it seems to
> >>> have lost it's reason for existence ...
> >> 
> >> No, setting MADV_DONTNEED has other nice properties during
> >> put_pages() - I think it is useful in its own right, for example
> >> that is where my page stealing code goes...
> > 
> > Well right now I can't make sense of this bit any more (tbh I
> > didn't with the other code either, but overlooked that while
> > reviewing). When it's just there for future work but atm dead code
> > I prefer for it to get removed. -Daniel
> 
> 
> So can we also revert the hunk in i915_gem_free_objects?  I would like
> to get this patch merged, it looks like that is the primary concern.

A problem I have is that the test written to hit the exact condition
considered in the changelog does not ellict the bug. 

Can you test whether

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 39e032615b31..6269204ba16f 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1030,6 +1030,7 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas,
                        /* update for the implicit flush after a batch */
                        obj->base.write_domain &= ~I915_GEM_GPU_DOMAINS;
                }
+               obj->dirty = 1;
                if (entry->flags & EXEC_OBJECT_NEEDS_FENCE) {
                        i915_gem_request_assign(&obj->last_fenced_req, req);
                        if (entry->flags & __EXEC_OBJECT_HAS_FENCE) {

makes the bug go away. If so, I think the bug is in the caller not
setting reloc domains correctly.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

* Re: [PATCH] drm/i915: Do not invalidate obj->pages under mempressure
  2015-02-09 16:46         ` Chris Wilson
@ 2015-02-09 18:31           ` Sean V Kelley
  2015-02-11  0:55           ` Sean V Kelley
  1 sibling, 0 replies; 18+ messages in thread
From: Sean V Kelley @ 2015-02-09 18:31 UTC (permalink / raw)
  To: Chris Wilson, Daniel Vetter, intel-gfx, stable



On 02/09/2015 08:46 AM, Chris Wilson wrote:
> On Sun, Feb 08, 2015 at 03:27:13PM -0800, Sean V Kelley wrote:
>> 
>> 
>> On 01/16/2015 08:05 PM, Daniel Vetter wrote:
>>> On Thu, Jan 15, 2015 at 08:44:00PM +0000, Chris Wilson wrote:
>>>> On Thu, Jan 15, 2015 at 08:36:15PM +0100, Daniel Vetter
>>>> wrote:
>>>>> On Wed, Jan 14, 2015 at 9:34 PM, Chris Wilson 
>>>>> <chris@chris-wilson.co.uk> wrote:
>>>>>> This (partially) reverts
>>>>>> 
>>>>>> commit 5537252b6b6d71fb1a8ed7395a8e5babf91953fd Author:
>>>>>> Chris Wilson <chris@chris-wilson.co.uk> Date:   Tue Mar
>>>>>> 25 13:23:06 2014 +0000
>>>>>> 
>>>>>> drm/i915: Invalidate our pages under memory pressure
>>>>> 
>>>>> Shouldn't we also revert the hunk in
>>>>> i915_gem_free_objects? Without the truncate vs. invalidate
>>>>> disdinction it seems to have lost it's reason for existence
>>>>> ...
>>>> 
>>>> No, setting MADV_DONTNEED has other nice properties during 
>>>> put_pages() - I think it is useful in its own right, for
>>>> example that is where my page stealing code goes...
>>> 
>>> Well right now I can't make sense of this bit any more (tbh I 
>>> didn't with the other code either, but overlooked that while 
>>> reviewing). When it's just there for future work but atm dead
>>> code I prefer for it to get removed. -Daniel
>> 
>> 
>> So can we also revert the hunk in i915_gem_free_objects?  I would
>> like to get this patch merged, it looks like that is the primary
>> concern.
> 
> A problem I have is that the test written to hit the exact
> condition considered in the changelog does not ellict the bug.
> 
> Can you test whether
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index
> 39e032615b31..6269204ba16f 100644 ---
> a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++
> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1030,6 +1030,7 @@
> i915_gem_execbuffer_move_to_active(struct list_head *vmas, /*
> update for the implicit flush after a batch */ 
> obj->base.write_domain &= ~I915_GEM_GPU_DOMAINS; } +
> obj->dirty = 1; if (entry->flags & EXEC_OBJECT_NEEDS_FENCE) { 
> i915_gem_request_assign(&obj->last_fenced_req, req); if
> (entry->flags & __EXEC_OBJECT_HAS_FENCE) {
> 
> makes the bug go away. If so, I think the bug is in the caller not 
> setting reloc domains correctly. -Chris

Sure,  I will take a look.

Thanks,

Sean

> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH] drm/i915: Do not invalidate obj->pages under mempressure
  2015-02-09 16:46         ` Chris Wilson
  2015-02-09 18:31           ` Sean V Kelley
@ 2015-02-11  0:55           ` Sean V Kelley
  2015-02-11 13:02             ` Daniel Vetter
  1 sibling, 1 reply; 18+ messages in thread
From: Sean V Kelley @ 2015-02-11  0:55 UTC (permalink / raw)
  To: Chris Wilson, Daniel Vetter, intel-gfx, stable



On 02/09/2015 08:46 AM, Chris Wilson wrote:
> On Sun, Feb 08, 2015 at 03:27:13PM -0800, Sean V Kelley wrote:
>>
>>
>> On 01/16/2015 08:05 PM, Daniel Vetter wrote:
>>> On Thu, Jan 15, 2015 at 08:44:00PM +0000, Chris Wilson wrote:
>>>> On Thu, Jan 15, 2015 at 08:36:15PM +0100, Daniel Vetter wrote:
>>>>> On Wed, Jan 14, 2015 at 9:34 PM, Chris Wilson
>>>>> <chris@chris-wilson.co.uk> wrote:
>>>>>> This (partially) reverts
>>>>>>
>>>>>> commit 5537252b6b6d71fb1a8ed7395a8e5babf91953fd Author: Chris
>>>>>> Wilson <chris@chris-wilson.co.uk> Date:   Tue Mar 25 13:23:06
>>>>>> 2014 +0000
>>>>>>
>>>>>> drm/i915: Invalidate our pages under memory pressure
>>>>>
>>>>> Shouldn't we also revert the hunk in i915_gem_free_objects?
>>>>> Without the truncate vs. invalidate disdinction it seems to
>>>>> have lost it's reason for existence ...
>>>>
>>>> No, setting MADV_DONTNEED has other nice properties during
>>>> put_pages() - I think it is useful in its own right, for example
>>>> that is where my page stealing code goes...
>>>
>>> Well right now I can't make sense of this bit any more (tbh I
>>> didn't with the other code either, but overlooked that while
>>> reviewing). When it's just there for future work but atm dead code
>>> I prefer for it to get removed. -Daniel
>>
>>
>> So can we also revert the hunk in i915_gem_free_objects?  I would like
>> to get this patch merged, it looks like that is the primary concern.
> 
> A problem I have is that the test written to hit the exact condition
> considered in the changelog does not ellict the bug. 
> 
> Can you test whether
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 39e032615b31..6269204ba16f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1030,6 +1030,7 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas,
>                         /* update for the implicit flush after a batch */
>                         obj->base.write_domain &= ~I915_GEM_GPU_DOMAINS;
>                 }
> +               obj->dirty = 1;
>                 if (entry->flags & EXEC_OBJECT_NEEDS_FENCE) {
>                         i915_gem_request_assign(&obj->last_fenced_req, req);
>                         if (entry->flags & __EXEC_OBJECT_HAS_FENCE) {
> 
> makes the bug go away. If so, I think the bug is in the caller not
> setting reloc domains correctly.

I think you may be right.  This implies a caller issue, because
essentially you are forcing a write back here as if it were in the
write domain.

No corruption seen.  I will add reloc domains to my growing audit list.

via

drm-intel-nightly: 2014y-12m-08d-22h-24m-34s UTC integration manifest


diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 0c25f62..4cb2755 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -970,6 +970,8 @@ i915_gem_execbuffer_move_to_active(struct list_head
*vmas,
                        /* update for the implicit flush after a batch */
                        obj->base.write_domain &= ~I915_GEM_GPU_DOMAINS;
                }
+
+               obj->dirty = 1;
                if (entry->flags & EXEC_OBJECT_NEEDS_FENCE) {
                        i915_gem_request_assign(&obj->last_fenced_req, req);
                        if (entry->flags & __EXEC_OBJECT_HAS_FENCE) {


Sean


> -Chris
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH] drm/i915: Do not invalidate obj->pages under mempressure
  2015-02-11  0:55           ` Sean V Kelley
@ 2015-02-11 13:02             ` Daniel Vetter
  2015-02-20 22:19               ` Sean V Kelley
  0 siblings, 1 reply; 18+ messages in thread
From: Daniel Vetter @ 2015-02-11 13:02 UTC (permalink / raw)
  To: Sean V Kelley; +Cc: intel-gfx, stable

On Wed, Feb 11, 2015 at 1:55 AM, Sean V Kelley <seanvk@posteo.de> wrote:
> No corruption seen.  I will add reloc domains to my growing audit list.

One more for the libva audit list:

If you do any ioctl directly, please make sure that you clear the
ioctl structure with memset(&arg, 0, sizeof(arg)); or similar.
Otherwise when we extended them in upstream (which we're just doing,
the kernel 0-fills the new fields automatically) it'll break because
after a recompile there's now garbage in the new fields.

I just spent a while fixing up libdrm ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH] drm/i915: Do not invalidate obj->pages under mempressure
  2015-02-11 13:02             ` Daniel Vetter
@ 2015-02-20 22:19               ` Sean V Kelley
  0 siblings, 0 replies; 18+ messages in thread
From: Sean V Kelley @ 2015-02-20 22:19 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx, stable

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1



On 02/11/2015 05:02 AM, Daniel Vetter wrote:
> On Wed, Feb 11, 2015 at 1:55 AM, Sean V Kelley <seanvk@posteo.de>
> wrote:
>> No corruption seen.  I will add reloc domains to my growing audit
>> list.
> 
> One more for the libva audit list:
> 
> If you do any ioctl directly, please make sure that you clear the 
> ioctl structure with memset(&arg, 0, sizeof(arg)); or similar. 
> Otherwise when we extended them in upstream (which we're just
> doing, the kernel 0-fills the new fields automatically) it'll break
> because after a recompile there's now garbage in the new fields.
> 
> I just spent a while fixing up libdrm ;-) -Daniel
> 

Cleaned up the hybrid driver.  Found numerous instances of the
wrong reloc domain assignments, e.g., write_domain of 0. I've since
verified it was the root cause of the run 2 run issue. Also cleaned up
ioctl and structure clearing.

Thanks,

Sean
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBAgAGBQJU57L/AAoJEGScDsMo8QYOFsIQANHSmmYUG5JSqsmewCDO1zHk
dh2h56x5oCIooJ2mOh9lcWTRpVbE4uv6HOt6WfThb4dP11Yg5LlOZA1uR65iniIy
9gi2vUotuajm9cOdXKVXCbQBuymR0rKJuRb74e73jk/u5QpOv/zNtThXOG7FfUVu
p1DpoVuh8iLGexwAd+6UrCOze5hWfCuh+9t9jTZ8Yr0pgSiRsGClX1mW8vUD4+99
7h5VGonr3tKYqHNZBtneaTcmRkVBtuZ/RBGr5FOnzP1VKIXidgt2t4ID33nZmjsx
rJjzQfh408qniBr9L7zIIDUZMiiAucIRm1+4eQ1b+yA9pYkXQfrs/lA9dH80IjAw
iWFS+IX7xUCpKZzLB38+dIV/Nsn0nxoF5N1ptz4RMKoeXJVWsRS72jgCjlrE5w4R
dnyodU7nlmdcNoeGcDEhcN5Jek7OPYoJLgaC100wu6yDZKJ/2CXJ6XwyORWQ7GU1
UWyqOuam84DJqiwzwkLl6Q9Ur7oC8S8jUIFLqRk/1PZOesu90/yNoJUByLQMMZbl
WzUMGdV4D18+uTHhmWGPruCYHLMgRXKVlT/diKH1q5aBOvtP4xeANn+0GSw+E3OZ
TN1TGDW/q8U0uN1CDk5hTIDgiJX0Eo9an0CsJ2yZL3K+J56+e6QUY6EPrU/ctqp0
QbT5eI44/dZX2Xmf/kc7
=24vy
-----END PGP SIGNATURE-----
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* [PATCH] drm/i915: Do not invalidate obj->pages under mempressure
  2015-01-14 20:34 [PATCH] drm/i915: Do not invalidate obj->pages under mempressure Chris Wilson
                   ` (2 preceding siblings ...)
  2015-01-15 19:36 ` Daniel Vetter
@ 2015-02-09 16:43 ` Daniel Vetter
  2015-02-10  8:19   ` shuang.he
  2015-02-10 11:38   ` Jani Nikula
  3 siblings, 2 replies; 18+ messages in thread
From: Daniel Vetter @ 2015-02-09 16:43 UTC (permalink / raw)
  To: Intel Graphics Development
  Cc: Chris Wilson, Sean V Kelley, stable, Jani Nikula, Daniel Vetter

From: Chris Wilson <chris@chris-wilson.co.uk>

This (partially) reverts

commit 5537252b6b6d71fb1a8ed7395a8e5babf91953fd
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Tue Mar 25 13:23:06 2014 +0000

    drm/i915: Invalidate our pages under memory pressure

It appears given the right workload, that pages which are swapped out
more than once are incorrectly invalidated and discarded. I had presumed
that the swapin would mark the pages dirty again and so preserve them
against the next cycle of invalidation - that appears to be false, and
leads to memory corruption (even leak of stale pages to userspace).

v2: Do a more throughrought revert and als get rid of the hunk in
gem_free_objects which we've tried to patch up already in

commit 340fbd8ca1c7d6006a6b6afe716c10007bbfde85
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Thu May 22 09:16:52 2014 +0100

    drm/i915: Only discard backing storage on releasing the last ref

This means this patch also fully reverts this fixup. Apparently this
is just too tricky.

Reported-by: Sean V Kelley <sean.v.kelley@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Sean V Kelley <sean.v.kelley@intel.com>
Cc: stable@vger.kernel.org
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> (v2)
---
 drivers/gpu/drm/i915/i915_gem.c | 49 ++---------------------------------------
 1 file changed, 2 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 36f1093e3c63..39e2af9b5fef 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1946,26 +1946,6 @@ i915_gem_object_truncate(struct drm_i915_gem_object *obj)
 	obj->madv = __I915_MADV_PURGED;
 }
 
-/* Try to discard unwanted pages */
-static void
-i915_gem_object_invalidate(struct drm_i915_gem_object *obj)
-{
-	struct address_space *mapping;
-
-	switch (obj->madv) {
-	case I915_MADV_DONTNEED:
-		i915_gem_object_truncate(obj);
-	case __I915_MADV_PURGED:
-		return;
-	}
-
-	if (obj->base.filp == NULL)
-		return;
-
-	mapping = file_inode(obj->base.filp)->i_mapping,
-	invalidate_mapping_pages(mapping, 0, (loff_t)-1);
-}
-
 static void
 i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj)
 {
@@ -2028,7 +2008,8 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
 	ops->put_pages(obj);
 	obj->pages = NULL;
 
-	i915_gem_object_invalidate(obj);
+	if (i915_gem_object_is_purgeable(obj))
+		i915_gem_object_truncate(obj);
 
 	return 0;
 }
@@ -4458,30 +4439,6 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
 	return obj;
 }
 
-static bool discard_backing_storage(struct drm_i915_gem_object *obj)
-{
-	/* If we are the last user of the backing storage (be it shmemfs
-	 * pages or stolen etc), we know that the pages are going to be
-	 * immediately released. In this case, we can then skip copying
-	 * back the contents from the GPU.
-	 */
-
-	if (obj->madv != I915_MADV_WILLNEED)
-		return false;
-
-	if (obj->base.filp == NULL)
-		return true;
-
-	/* At first glance, this looks racy, but then again so would be
-	 * userspace racing mmap against close. However, the first external
-	 * reference to the filp can only be obtained through the
-	 * i915_gem_mmap_ioctl() which safeguards us against the user
-	 * acquiring such a reference whilst we are in the middle of
-	 * freeing the object.
-	 */
-	return atomic_long_read(&obj->base.filp->f_count) == 1;
-}
-
 void i915_gem_free_object(struct drm_gem_object *gem_obj)
 {
 	struct drm_i915_gem_object *obj = to_intel_bo(gem_obj);
@@ -4524,8 +4481,6 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
 
 	if (WARN_ON(obj->pages_pin_count))
 		obj->pages_pin_count = 0;
-	if (discard_backing_storage(obj))
-		obj->madv = I915_MADV_DONTNEED;
 	i915_gem_object_put_pages(obj);
 	i915_gem_object_free_mmap_offset(obj);
 
-- 
2.1.4

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

* Re: [PATCH] drm/i915: Do not invalidate obj->pages under mempressure
  2015-02-09 16:43 ` Daniel Vetter
@ 2015-02-10  8:19   ` shuang.he
  2015-02-10 11:38   ` Jani Nikula
  1 sibling, 0 replies; 18+ messages in thread
From: shuang.he @ 2015-02-10  8:19 UTC (permalink / raw)
  To: shuang.he, ethan.gao, intel-gfx, daniel.vetter

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5733
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  282/283              282/283
ILK              +3-1              308/319              310/319
SNB              +1-3              340/346              338/346
IVB                 -1              378/384              377/384
BYT                                  296/296              296/296
HSW              +2                 421/428              423/428
BDW                                  318/333              318/333
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 ILK  igt_gem_unfence_active_buffers      DMESG_WARN(1, M37)PASS(1, M37)      DMESG_WARN(1, M37)
 ILK  igt_kms_flip_busy-flip-interruptible      TIMEOUT(2, M37)PASS(1, M37)      PASS(1, M37)
*ILK  igt_kms_flip_flip-vs-panning      TIMEOUT(2, M37)      PASS(1, M37)
*ILK  igt_kms_flip_plain-flip-ts-check-interruptible      TIMEOUT(1, M37)      PASS(1, M37)
*SNB  igt_kms_flip_dpms-vs-vblank-race      PASS(4, M22)      DMESG_WARN(1, M22)
 SNB  igt_kms_flip_dpms-vs-vblank-race-interruptible      DMESG_WARN(3, M22)PASS(2, M22)      DMESG_WARN(1, M22)
 SNB  igt_kms_flip_modeset-vs-vblank-race-interruptible      DMESG_WARN(1, M22)PASS(2, M22)      DMESG_WARN(1, M22)
 SNB  igt_kms_pipe_crc_basic_read-crc-pipe-A      DMESG_WARN(1, M22)PASS(6, M22)      PASS(1, M22)
 IVB  igt_gem_pwrite_pread_snooped-copy-performance      DMESG_WARN(1, M34)PASS(3, M34)      DMESG_WARN(1, M34)
 HSW  igt_gem_storedw_loop_blt      DMESG_WARN(2, M20)PASS(3, M20)      PASS(1, M20)
 HSW  igt_gem_storedw_loop_vebox      DMESG_WARN(2, M20)PASS(2, M20)      PASS(1, M20)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH] drm/i915: Do not invalidate obj->pages under mempressure
  2015-02-09 16:43 ` Daniel Vetter
  2015-02-10  8:19   ` shuang.he
@ 2015-02-10 11:38   ` Jani Nikula
  2015-02-24 13:27     ` Jani Nikula
  1 sibling, 1 reply; 18+ messages in thread
From: Jani Nikula @ 2015-02-10 11:38 UTC (permalink / raw)
  To: Daniel Vetter, Intel Graphics Development; +Cc: Daniel Vetter, stable

On Mon, 09 Feb 2015, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
>
> This (partially) reverts
>
> commit 5537252b6b6d71fb1a8ed7395a8e5babf91953fd
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Tue Mar 25 13:23:06 2014 +0000
>
>     drm/i915: Invalidate our pages under memory pressure
>
> It appears given the right workload, that pages which are swapped out
> more than once are incorrectly invalidated and discarded. I had presumed
> that the swapin would mark the pages dirty again and so preserve them
> against the next cycle of invalidation - that appears to be false, and
> leads to memory corruption (even leak of stale pages to userspace).
>
> v2: Do a more throughrought revert and als get rid of the hunk in
> gem_free_objects which we've tried to patch up already in
>
> commit 340fbd8ca1c7d6006a6b6afe716c10007bbfde85
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Thu May 22 09:16:52 2014 +0100
>
>     drm/i915: Only discard backing storage on releasing the last ref
>
> This means this patch also fully reverts this fixup. Apparently this
> is just too tricky.
>
> Reported-by: Sean V Kelley <sean.v.kelley@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Sean V Kelley <sean.v.kelley@intel.com>
> Cc: stable@vger.kernel.org
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> (v2)

Pushed this one to drm-intel-next-fixes, thanks for the patch, and v2 of
the patch.

BR,
Jani.

> ---
>  drivers/gpu/drm/i915/i915_gem.c | 49 ++---------------------------------------
>  1 file changed, 2 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 36f1093e3c63..39e2af9b5fef 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1946,26 +1946,6 @@ i915_gem_object_truncate(struct drm_i915_gem_object *obj)
>  	obj->madv = __I915_MADV_PURGED;
>  }
>  
> -/* Try to discard unwanted pages */
> -static void
> -i915_gem_object_invalidate(struct drm_i915_gem_object *obj)
> -{
> -	struct address_space *mapping;
> -
> -	switch (obj->madv) {
> -	case I915_MADV_DONTNEED:
> -		i915_gem_object_truncate(obj);
> -	case __I915_MADV_PURGED:
> -		return;
> -	}
> -
> -	if (obj->base.filp == NULL)
> -		return;
> -
> -	mapping = file_inode(obj->base.filp)->i_mapping,
> -	invalidate_mapping_pages(mapping, 0, (loff_t)-1);
> -}
> -
>  static void
>  i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj)
>  {
> @@ -2028,7 +2008,8 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
>  	ops->put_pages(obj);
>  	obj->pages = NULL;
>  
> -	i915_gem_object_invalidate(obj);
> +	if (i915_gem_object_is_purgeable(obj))
> +		i915_gem_object_truncate(obj);
>  
>  	return 0;
>  }
> @@ -4458,30 +4439,6 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
>  	return obj;
>  }
>  
> -static bool discard_backing_storage(struct drm_i915_gem_object *obj)
> -{
> -	/* If we are the last user of the backing storage (be it shmemfs
> -	 * pages or stolen etc), we know that the pages are going to be
> -	 * immediately released. In this case, we can then skip copying
> -	 * back the contents from the GPU.
> -	 */
> -
> -	if (obj->madv != I915_MADV_WILLNEED)
> -		return false;
> -
> -	if (obj->base.filp == NULL)
> -		return true;
> -
> -	/* At first glance, this looks racy, but then again so would be
> -	 * userspace racing mmap against close. However, the first external
> -	 * reference to the filp can only be obtained through the
> -	 * i915_gem_mmap_ioctl() which safeguards us against the user
> -	 * acquiring such a reference whilst we are in the middle of
> -	 * freeing the object.
> -	 */
> -	return atomic_long_read(&obj->base.filp->f_count) == 1;
> -}
> -
>  void i915_gem_free_object(struct drm_gem_object *gem_obj)
>  {
>  	struct drm_i915_gem_object *obj = to_intel_bo(gem_obj);
> @@ -4524,8 +4481,6 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
>  
>  	if (WARN_ON(obj->pages_pin_count))
>  		obj->pages_pin_count = 0;
> -	if (discard_backing_storage(obj))
> -		obj->madv = I915_MADV_DONTNEED;
>  	i915_gem_object_put_pages(obj);
>  	i915_gem_object_free_mmap_offset(obj);
>  
> -- 
> 2.1.4
>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH] drm/i915: Do not invalidate obj->pages under mempressure
  2015-02-10 11:38   ` Jani Nikula
@ 2015-02-24 13:27     ` Jani Nikula
  0 siblings, 0 replies; 18+ messages in thread
From: Jani Nikula @ 2015-02-24 13:27 UTC (permalink / raw)
  To: Daniel Vetter, Intel Graphics Development; +Cc: Daniel Vetter, stable

On Tue, 10 Feb 2015, Jani Nikula <jani.nikula@intel.com> wrote:
> On Mon, 09 Feb 2015, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> From: Chris Wilson <chris@chris-wilson.co.uk>
>>
>> This (partially) reverts
>>
>> commit 5537252b6b6d71fb1a8ed7395a8e5babf91953fd
>> Author: Chris Wilson <chris@chris-wilson.co.uk>
>> Date:   Tue Mar 25 13:23:06 2014 +0000
>>
>>     drm/i915: Invalidate our pages under memory pressure
>>
>> It appears given the right workload, that pages which are swapped out
>> more than once are incorrectly invalidated and discarded. I had presumed
>> that the swapin would mark the pages dirty again and so preserve them
>> against the next cycle of invalidation - that appears to be false, and
>> leads to memory corruption (even leak of stale pages to userspace).
>>
>> v2: Do a more throughrought revert and als get rid of the hunk in
>> gem_free_objects which we've tried to patch up already in
>>
>> commit 340fbd8ca1c7d6006a6b6afe716c10007bbfde85
>> Author: Chris Wilson <chris@chris-wilson.co.uk>
>> Date:   Thu May 22 09:16:52 2014 +0100
>>
>>     drm/i915: Only discard backing storage on releasing the last ref
>>
>> This means this patch also fully reverts this fixup. Apparently this
>> is just too tricky.
>>
>> Reported-by: Sean V Kelley <sean.v.kelley@intel.com>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Sean V Kelley <sean.v.kelley@intel.com>
>> Cc: stable@vger.kernel.org
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> (v2)
>
> Pushed this one to drm-intel-next-fixes, thanks for the patch, and v2 of
> the patch.

For completeness, this one was dropped again on Feb 11.

Jani.


>
> BR,
> Jani.
>
>> ---
>>  drivers/gpu/drm/i915/i915_gem.c | 49 ++---------------------------------------
>>  1 file changed, 2 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index 36f1093e3c63..39e2af9b5fef 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -1946,26 +1946,6 @@ i915_gem_object_truncate(struct drm_i915_gem_object *obj)
>>  	obj->madv = __I915_MADV_PURGED;
>>  }
>>  
>> -/* Try to discard unwanted pages */
>> -static void
>> -i915_gem_object_invalidate(struct drm_i915_gem_object *obj)
>> -{
>> -	struct address_space *mapping;
>> -
>> -	switch (obj->madv) {
>> -	case I915_MADV_DONTNEED:
>> -		i915_gem_object_truncate(obj);
>> -	case __I915_MADV_PURGED:
>> -		return;
>> -	}
>> -
>> -	if (obj->base.filp == NULL)
>> -		return;
>> -
>> -	mapping = file_inode(obj->base.filp)->i_mapping,
>> -	invalidate_mapping_pages(mapping, 0, (loff_t)-1);
>> -}
>> -
>>  static void
>>  i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj)
>>  {
>> @@ -2028,7 +2008,8 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
>>  	ops->put_pages(obj);
>>  	obj->pages = NULL;
>>  
>> -	i915_gem_object_invalidate(obj);
>> +	if (i915_gem_object_is_purgeable(obj))
>> +		i915_gem_object_truncate(obj);
>>  
>>  	return 0;
>>  }
>> @@ -4458,30 +4439,6 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
>>  	return obj;
>>  }
>>  
>> -static bool discard_backing_storage(struct drm_i915_gem_object *obj)
>> -{
>> -	/* If we are the last user of the backing storage (be it shmemfs
>> -	 * pages or stolen etc), we know that the pages are going to be
>> -	 * immediately released. In this case, we can then skip copying
>> -	 * back the contents from the GPU.
>> -	 */
>> -
>> -	if (obj->madv != I915_MADV_WILLNEED)
>> -		return false;
>> -
>> -	if (obj->base.filp == NULL)
>> -		return true;
>> -
>> -	/* At first glance, this looks racy, but then again so would be
>> -	 * userspace racing mmap against close. However, the first external
>> -	 * reference to the filp can only be obtained through the
>> -	 * i915_gem_mmap_ioctl() which safeguards us against the user
>> -	 * acquiring such a reference whilst we are in the middle of
>> -	 * freeing the object.
>> -	 */
>> -	return atomic_long_read(&obj->base.filp->f_count) == 1;
>> -}
>> -
>>  void i915_gem_free_object(struct drm_gem_object *gem_obj)
>>  {
>>  	struct drm_i915_gem_object *obj = to_intel_bo(gem_obj);
>> @@ -4524,8 +4481,6 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
>>  
>>  	if (WARN_ON(obj->pages_pin_count))
>>  		obj->pages_pin_count = 0;
>> -	if (discard_backing_storage(obj))
>> -		obj->madv = I915_MADV_DONTNEED;
>>  	i915_gem_object_put_pages(obj);
>>  	i915_gem_object_free_mmap_offset(obj);
>>  
>> -- 
>> 2.1.4
>>
>
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

end of thread, other threads:[~2015-02-24 13:26 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-01-14 20:34 [PATCH] drm/i915: Do not invalidate obj->pages under mempressure Chris Wilson
2015-01-15  0:55 ` [Intel-gfx] " Daniel Vetter
2015-01-15  1:19   ` Sean V Kelley
2015-01-15  9:38   ` Chris Wilson
2015-01-15  2:11 ` shuang.he
2015-01-15 19:36 ` Daniel Vetter
2015-01-15 20:44   ` [Intel-gfx] " Chris Wilson
2015-01-17  4:05     ` Daniel Vetter
2015-02-08 23:27       ` [Intel-gfx] " Sean V Kelley
2015-02-09 16:46         ` Chris Wilson
2015-02-09 18:31           ` Sean V Kelley
2015-02-11  0:55           ` Sean V Kelley
2015-02-11 13:02             ` Daniel Vetter
2015-02-20 22:19               ` Sean V Kelley
2015-02-09 16:43 ` Daniel Vetter
2015-02-10  8:19   ` shuang.he
2015-02-10 11:38   ` Jani Nikula
2015-02-24 13:27     ` Jani Nikula

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox