On Thu, Apr 17, 2014 at 07:34:31PM +0300, Jani Nikula wrote:
> On Wed, 16 Apr 2014, Robert Beckett <robert.beckett@xxxxxxxxx> 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@xxxxxxxxxx>
> >> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> >> ---
> >> 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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
