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

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

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