Re: [PATCH 4/4] drm/i915: Invalidate our pages under memory pressure

[Robert Beckett <robert.beckett@intel.com>]

On 22 April 2014 20:06:14, Daniel Vetter wrote:
> On Thu, Apr 17, 2014 at 07:34:31PM +0300, Jani Nikula wrote:
>> On Wed, 16 Apr 2014, Robert Beckett <robert.beckett@intel.com> wrote:
>>> On 25/03/2014 13:23, Chris Wilson wrote:
>>>> Try to flush out dirty pages into the swapcache (and from there into the
>>>> swapfile) when under memory pressure and forced to drop GEM objects from
>>>> memory. In effect, this should just allow us to discard unused pages for
>>>> memory reclaim and to start writeback earlier.
>>>>
>>>> v2: Hugh Dickins warned that explicitly starting writeback from
>>>> shrink_slab was prone to deadlocks within shmemfs.
>>>>
>>>> Cc: Hugh Dickins <hughd@google.com>
>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_gem.c | 38 +++++++++++++++++++++++++++-----------
>>>>    1 file changed, 27 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>>>> index 135ee8bd55f6..8287fd6701c6 100644
>>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>>> @@ -60,7 +60,6 @@ static unsigned long i915_gem_shrinker_scan(struct shrinker *shrinker,
>>>>    					    struct shrink_control *sc);
>>>>    static unsigned long i915_gem_purge(struct drm_i915_private *dev_priv, long target);
>>>>    static unsigned long i915_gem_shrink_all(struct drm_i915_private *dev_priv);
>>>> -static void i915_gem_object_truncate(struct drm_i915_gem_object *obj);
>>>>    static void i915_gem_retire_requests_ring(struct intel_ring_buffer *ring);
>>>>
>>>>    static bool cpu_cache_is_coherent(struct drm_device *dev,
>>>> @@ -1685,12 +1684,16 @@ i915_gem_mmap_gtt_ioctl(struct drm_device *dev, void *data,
>>>>    	return i915_gem_mmap_gtt(file, dev, args->handle, &args->offset);
>>>>    }
>>>>
>>>> +static inline int
>>>> +i915_gem_object_is_purgeable(struct drm_i915_gem_object *obj)
>>>> +{
>>>> +	return obj->madv == I915_MADV_DONTNEED;
>>>> +}
>>>> +
>>>>    /* Immediately discard the backing storage */
>>>>    static void
>>>>    i915_gem_object_truncate(struct drm_i915_gem_object *obj)
>>>>    {
>>>> -	struct inode *inode;
>>>> -
>>>>    	i915_gem_object_free_mmap_offset(obj);
>>>>
>>>>    	if (obj->base.filp == NULL)
>>>> @@ -1701,16 +1704,28 @@ i915_gem_object_truncate(struct drm_i915_gem_object *obj)
>>>>    	 * To do this we must instruct the shmfs to drop all of its
>>>>    	 * backing pages, *now*.
>>>>    	 */
>>>> -	inode = file_inode(obj->base.filp);
>>>> -	shmem_truncate_range(inode, 0, (loff_t)-1);
>>>> -
>>>> +	shmem_truncate_range(file_inode(obj->base.filp), 0, (loff_t)-1);
>>>>    	obj->madv = __I915_MADV_PURGED;
>>>>    }
>>>>
>>>> -static inline int
>>>> -i915_gem_object_is_purgeable(struct drm_i915_gem_object *obj)
>>>> +/* Try to discard unwanted pages */
>>>> +static void
>>>> +i915_gem_object_invalidate(struct drm_i915_gem_object *obj)
>>>>    {
>>>> -	return obj->madv == I915_MADV_DONTNEED;
>>>> +	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
>>>> @@ -1775,8 +1790,7 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
>>>>    	ops->put_pages(obj);
>>>>    	obj->pages = NULL;
>>>>
>>>> -	if (i915_gem_object_is_purgeable(obj))
>>>> -		i915_gem_object_truncate(obj);
>>>> +	i915_gem_object_invalidate(obj);
>>>>
>>>>    	return 0;
>>>>    }
>>>> @@ -4201,6 +4215,8 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
>>>>
>>>>    	if (WARN_ON(obj->pages_pin_count))
>>>>    		obj->pages_pin_count = 0;
>>>> +	if (obj->madv != __I915_MADV_PURGED)
>>>> +		obj->madv = I915_MADV_DONTNEED;
>>>>    	i915_gem_object_put_pages(obj);
>>>>    	i915_gem_object_free_mmap_offset(obj);
>>>>    	i915_gem_object_release_stolen(obj);
>>>>
>>>
>>> Functionally it looks good to me.
>>>
>>> Though, you may want a /* fall-through */ comment (some people cant
>>> mentally parse fallthroughs without being prompted) and a default:
>>> break; (to avoid any static code analysis complaints) in the switch in
>>> i915_gem_object_invalidate.
>>
>> Or just two if statements.
>
> 	if (MADV_DONTNEED)
> 		i915_gem_object_truncate(obj);
> 	if (!MADV_WILLNEED)
> 		return;
>
> would indeed looka a bit cleaner.
>
> And a question to Bob: Is your r-b for the entire series or just this
> patch? Generally the assumption is that an r-b tag is only for the patch
> replied to, except when otherwise stated. Usually people just slap r-b
> tags onto patches as they go through a series.
> -Daniel

It was meant for the whole series.