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

[Jani Nikula <jani.nikula@intel.com>]

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