| Message ID | 1478006886-4489-1-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Nov 01, 2016 at 01:28:06PM +0000, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Commit 1bec9b0bda3d ("drm/i915/shrinker: Only shmemfs objects > are backed by swap") stopped considering the userptr objects > in shrinker callbacks. > > Restore that so idle userptr objects can be discarded in order > to free up memory. > > One change further to what was introduced in 1bec9b0bda3d is > to start considering userptr objects in oom but that should > also be a correct thing to do. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Fixes: 1bec9b0bda3d ("drm/i915/shrinker: Only shmemfs objects are backed by swap") > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Mika Kuoppala <mika.kuoppala@intel.com> > Cc: <stable@vger.kernel.org> > --- > drivers/gpu/drm/i915/i915_gem_shrinker.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c > index 0993afc0e725..dfca1f6b3630 100644 > --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c > +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c > @@ -83,8 +83,8 @@ static bool can_release_pages(struct drm_i915_gem_object *obj) > if (!obj->mm.pages) > return false; > > - /* Only shmemfs objects are backed by swap */ > - if (!obj->base.filp) > + /* shmemfs and userptr objects are backed by swap */ > + if (!obj->base.filp && !obj->userptr.mm) Hmm. Another sticking point if we want to use the union again. How about obj->ops->flags & I915_GEM_OBJECT_HAS_BACKING_STORE ? Or I915_GEM_OBJECT_CAN_SWAP since we want this for i915_gem_internal.c as well, and that techinically doesn't have a backing store but can be reaped. Hmm. if (!(obj->ops->flags & I915_GEM_OBJECT_HAS_BACKING_STORE || obj->mm.madv == I915_MADV_DONTNEED)) -Chris
On 01/11/2016 13:52, Chris Wilson wrote: > On Tue, Nov 01, 2016 at 01:28:06PM +0000, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Commit 1bec9b0bda3d ("drm/i915/shrinker: Only shmemfs objects >> are backed by swap") stopped considering the userptr objects >> in shrinker callbacks. >> >> Restore that so idle userptr objects can be discarded in order >> to free up memory. >> >> One change further to what was introduced in 1bec9b0bda3d is >> to start considering userptr objects in oom but that should >> also be a correct thing to do. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Fixes: 1bec9b0bda3d ("drm/i915/shrinker: Only shmemfs objects are backed by swap") >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> Cc: Mika Kuoppala <mika.kuoppala@intel.com> >> Cc: <stable@vger.kernel.org> >> --- >> drivers/gpu/drm/i915/i915_gem_shrinker.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c >> index 0993afc0e725..dfca1f6b3630 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c >> +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c >> @@ -83,8 +83,8 @@ static bool can_release_pages(struct drm_i915_gem_object *obj) >> if (!obj->mm.pages) >> return false; >> >> - /* Only shmemfs objects are backed by swap */ >> - if (!obj->base.filp) >> + /* shmemfs and userptr objects are backed by swap */ >> + if (!obj->base.filp && !obj->userptr.mm) > > Hmm. Another sticking point if we want to use the union again. > > How about obj->ops->flags & I915_GEM_OBJECT_HAS_BACKING_STORE ? > Or I915_GEM_OBJECT_CAN_SWAP since we want this for i915_gem_internal.c > as well, and that techinically doesn't have a backing store but can be > reaped. Hmm. > > if (!(obj->ops->flags & I915_GEM_OBJECT_HAS_BACKING_STORE || > obj->mm.madv == I915_MADV_DONTNEED)) Hm I915_GEM_OBJECT_HAS_STRUCT_PAGE happens to be right - shm, userptr and internal. But that would be bad. Neither do I like HAS_BACKING_STORE. Maybe I915_GEM_OBJECT_IS_SHRINKABLE, fully dumb and explicit? Regards, Tvrtko
On Tue, Nov 01, 2016 at 02:29:39PM +0000, Tvrtko Ursulin wrote: > > On 01/11/2016 13:52, Chris Wilson wrote: > >On Tue, Nov 01, 2016 at 01:28:06PM +0000, Tvrtko Ursulin wrote: > >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> > >>Commit 1bec9b0bda3d ("drm/i915/shrinker: Only shmemfs objects > >>are backed by swap") stopped considering the userptr objects > >>in shrinker callbacks. > >> > >>Restore that so idle userptr objects can be discarded in order > >>to free up memory. > >> > >>One change further to what was introduced in 1bec9b0bda3d is > >>to start considering userptr objects in oom but that should > >>also be a correct thing to do. > >> > >>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >>Fixes: 1bec9b0bda3d ("drm/i915/shrinker: Only shmemfs objects are backed by swap") > >>Cc: Chris Wilson <chris@chris-wilson.co.uk> > >>Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > >>Cc: Mika Kuoppala <mika.kuoppala@intel.com> > >>Cc: <stable@vger.kernel.org> > >>--- > >> drivers/gpu/drm/i915/i915_gem_shrinker.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >>diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c > >>index 0993afc0e725..dfca1f6b3630 100644 > >>--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c > >>+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c > >>@@ -83,8 +83,8 @@ static bool can_release_pages(struct drm_i915_gem_object *obj) > >> if (!obj->mm.pages) > >> return false; > >> > >>- /* Only shmemfs objects are backed by swap */ > >>- if (!obj->base.filp) > >>+ /* shmemfs and userptr objects are backed by swap */ > >>+ if (!obj->base.filp && !obj->userptr.mm) > > > >Hmm. Another sticking point if we want to use the union again. > > > >How about obj->ops->flags & I915_GEM_OBJECT_HAS_BACKING_STORE ? > >Or I915_GEM_OBJECT_CAN_SWAP since we want this for i915_gem_internal.c > >as well, and that techinically doesn't have a backing store but can be > >reaped. Hmm. > > > >if (!(obj->ops->flags & I915_GEM_OBJECT_HAS_BACKING_STORE || > > obj->mm.madv == I915_MADV_DONTNEED)) > > Hm I915_GEM_OBJECT_HAS_STRUCT_PAGE happens to be right - shm, > userptr and internal. But that would be bad. Neither do I like > HAS_BACKING_STORE. Yeah, I was trying to keep the concept separate from has-struct-page just in case it made it easier for future extension. > Maybe I915_GEM_OBJECT_IS_SHRINKABLE, fully dumb and explicit? Dumb describes me best, yup. -Chris
On 01/11/2016 16:16, Patchwork wrote: > == Series Details == > > Series: drm/i915: Allow shrinking of userptr objects once again (rev2) > URL : https://patchwork.freedesktop.org/series/14675/ > State : warning > > == Summary == > > Series 14675v2 drm/i915: Allow shrinking of userptr objects once again > https://patchwork.freedesktop.org/api/1.0/series/14675/revisions/2/mbox/ > > Test drv_module_reload_basic: > dmesg-warn -> PASS (fi-skl-6770hq) > dmesg-warn -> PASS (fi-ilk-650) > Test kms_busy: > Subgroup basic-flip-default-b: > pass -> DMESG-WARN (fi-ilk-650) > Test kms_pipe_crc_basic: > Subgroup hang-read-crc-pipe-b: > pass -> DMESG-WARN (fi-ilk-650) > Subgroup nonblocking-crc-pipe-a-frame-sequence: > pass -> DMESG-WARN (fi-ilk-650) > Subgroup nonblocking-crc-pipe-b-frame-sequence: > pass -> DMESG-WARN (fi-ilk-650) > Subgroup read-crc-pipe-b: > pass -> DMESG-WARN (fi-ilk-650) DP bw issues.. https://bugs.freedesktop.org/show_bug.cgi?id=98531 > > fi-bdw-5557u total:241 pass:226 dwarn:0 dfail:0 fail:0 skip:15 > fi-bsw-n3050 total:241 pass:201 dwarn:0 dfail:0 fail:0 skip:40 > fi-bxt-t5700 total:241 pass:213 dwarn:0 dfail:0 fail:0 skip:28 > fi-byt-j1900 total:241 pass:213 dwarn:0 dfail:0 fail:0 skip:28 > fi-byt-n2820 total:241 pass:209 dwarn:0 dfail:0 fail:0 skip:32 > fi-hsw-4770 total:241 pass:221 dwarn:0 dfail:0 fail:0 skip:20 > fi-hsw-4770r total:241 pass:220 dwarn:0 dfail:0 fail:0 skip:21 > fi-ilk-650 total:241 pass:182 dwarn:5 dfail:0 fail:0 skip:54 > fi-ivb-3520m total:241 pass:218 dwarn:0 dfail:0 fail:0 skip:23 > fi-ivb-3770 total:241 pass:218 dwarn:0 dfail:0 fail:0 skip:23 > fi-kbl-7200u total:241 pass:219 dwarn:0 dfail:0 fail:0 skip:22 > fi-skl-6260u total:241 pass:227 dwarn:0 dfail:0 fail:0 skip:14 > fi-skl-6700hq total:241 pass:220 dwarn:0 dfail:0 fail:0 skip:21 > fi-skl-6700k total:241 pass:219 dwarn:1 dfail:0 fail:0 skip:21 > fi-skl-6770hq total:241 pass:227 dwarn:0 dfail:0 fail:0 skip:14 > fi-snb-2520m total:241 pass:208 dwarn:0 dfail:0 fail:0 skip:33 > fi-snb-2600 total:241 pass:207 dwarn:0 dfail:0 fail:0 skip:34 > > 659f3942757fa5e85f37a2cab9da8c94b8f16f7a drm-intel-nightly: 2016y-11m-01d-14h-42m-02s UTC integration manifest > ec90e56 drm/i915: Allow shrinking of userptr objects once again Merged to dinq, thanks for the review! Regards, Tvrtko
On Tue, Nov 01, 2016 at 04:36:34PM +0000, Tvrtko Ursulin wrote: > > Merged to dinq, thanks for the review! Almost as bad as me in forgetting to add the r-b. dim save us! :) -Chris
On 01/11/2016 16:41, Chris Wilson wrote: > On Tue, Nov 01, 2016 at 04:36:34PM +0000, Tvrtko Ursulin wrote: >> >> Merged to dinq, thanks for the review! > > Almost as bad as me in forgetting to add the r-b. dim save us! :) Took me some time to figure out what is bad in this case. :) For the reference of others, Chris gave the r-b, I downloaded the patch from PW and pushed it, but did not check the tags to notice r-b wasn't actually registered because of the missing colon. Regards, Tvrtko
On Wed, Nov 02, 2016 at 08:33:56AM +0000, Tvrtko Ursulin wrote: > > On 01/11/2016 16:41, Chris Wilson wrote: > >On Tue, Nov 01, 2016 at 04:36:34PM +0000, Tvrtko Ursulin wrote: > >> > >>Merged to dinq, thanks for the review! > > > >Almost as bad as me in forgetting to add the r-b. dim save us! :) > > Took me some time to figure out what is bad in this case. :) > > For the reference of others, Chris gave the r-b, I downloaded the > patch from PW and pushed it, but did not check the tags to notice > r-b wasn't actually registered because of the missing colon. Ha, still my fault then. :) -Chris
On 02/11/2016 08:37, Chris Wilson wrote: > On Wed, Nov 02, 2016 at 08:33:56AM +0000, Tvrtko Ursulin wrote: >> >> On 01/11/2016 16:41, Chris Wilson wrote: >>> On Tue, Nov 01, 2016 at 04:36:34PM +0000, Tvrtko Ursulin wrote: >>>> >>>> Merged to dinq, thanks for the review! >>> >>> Almost as bad as me in forgetting to add the r-b. dim save us! :) >> >> Took me some time to figure out what is bad in this case. :) >> >> For the reference of others, Chris gave the r-b, I downloaded the >> patch from PW and pushed it, but did not check the tags to notice >> r-b wasn't actually registered because of the missing colon. > > Ha, still my fault then. :) No, didn't mean to suggest that at all - fully my fault for missing it. In my defense, I probably did eyeball it after applying since I always do that, but I suspect I just was not systematic enough. Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c index 0993afc0e725..dfca1f6b3630 100644 --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c @@ -83,8 +83,8 @@ static bool can_release_pages(struct drm_i915_gem_object *obj) if (!obj->mm.pages) return false; - /* Only shmemfs objects are backed by swap */ - if (!obj->base.filp) + /* shmemfs and userptr objects are backed by swap */ + if (!obj->base.filp && !obj->userptr.mm) return false; /* Only report true if by unbinding the object and putting its pages