From mboxrd@z Thu Jan 1 00:00:00 1970 From: Robert Beckett Subject: Re: [PATCH 4/4] drm/i915: Invalidate our pages under memory pressure Date: Wed, 23 Apr 2014 00:23:44 +0100 Message-ID: <5356FA00.8010602@intel.com> References: <1395753786-6442-1-git-send-email-chris@chris-wilson.co.uk> <1395753786-6442-4-git-send-email-chris@chris-wilson.co.uk> <534EBA40.3080909@intel.com> <87mwfj3ofc.fsf@intel.com> <20140422190614.GZ10722@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id B1EA26E5C0 for ; Tue, 22 Apr 2014 16:23:47 -0700 (PDT) In-Reply-To: <20140422190614.GZ10722@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter Cc: intel-gfx@lists.freedesktop.org, Hugh Dickins List-Id: intel-gfx@lists.freedesktop.org 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 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 >>>> Signed-off-by: Chris Wilson >>>> --- >>>> 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.