On Tue, 10 Feb 2015, Jani Nikula <jani.nikula@xxxxxxxxx> wrote:
> On Mon, 09 Feb 2015, Daniel Vetter <daniel.vetter@xxxxxxxx> wrote:
>> From: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>>
>> This (partially) reverts
>>
>> commit 5537252b6b6d71fb1a8ed7395a8e5babf91953fd
>> Author: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>> 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@xxxxxxxxxxxxxxxxxx>
>> 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@xxxxxxxxx>
>> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>> Cc: Sean V Kelley <sean.v.kelley@xxxxxxxxx>
>> Cc: stable@xxxxxxxxxxxxxxx
>> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>> Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
>> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx> (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
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html