| Message ID | 20211105130333.797862-1-thomas.hellstrom@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gem: Fix gem_madvise for ttm+shmem objects | expand |
On 05/11/2021 13:03, Thomas Hellström wrote: > Gem-TTM objects that are backed by shmem might have populated > page-vectors without having the Gem pages set. Those objects > aren't moved to the correct shrinker / purge list by the > gem_madvise. Furthermore they are purged directly on > MADV_DONTNEED rather than waiting for the shrinker to do that. > > For such objects, identified by having the > _SELF_MANAGED_SHRINK_LIST set, make sure they end up on the > correct list and defer purging to the shrinker. > > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index d0e642c82064..da972c8d45b1 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1005,7 +1005,8 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data, > obj->ops->adjust_lru(obj); > } > > - if (i915_gem_object_has_pages(obj)) { > + if (i915_gem_object_has_pages(obj) || > + i915_gem_object_has_self_managed_shrink_list(obj)) { Makes sense. > unsigned long flags; > > spin_lock_irqsave(&i915->mm.obj_lock, flags); > @@ -1024,7 +1025,8 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data, > > /* if the object is no longer attached, discard its backing storage */ > if (obj->mm.madv == I915_MADV_DONTNEED && > - !i915_gem_object_has_pages(obj)) > + !i915_gem_object_has_pages(obj) && > + !i915_gem_object_has_self_managed_shrink_list(obj)) > i915_gem_object_truncate(obj); And it looks like this also matches the workings of lmem, where under memory pressure we also just purge such objects, instead of moving them, making sure to keep them first in the LRU? One thing is to maybe immediately discard already swapped-out objects here, since the shrinker will be oblivious to them, and they sort of just linger in swap until the object is destroyed? > > args->retained = obj->mm.madv != __I915_MADV_PURGED; >
On 11/5/21 16:18, Matthew Auld wrote: > On 05/11/2021 13:03, Thomas Hellström wrote: >> Gem-TTM objects that are backed by shmem might have populated >> page-vectors without having the Gem pages set. Those objects >> aren't moved to the correct shrinker / purge list by the >> gem_madvise. Furthermore they are purged directly on >> MADV_DONTNEED rather than waiting for the shrinker to do that. >> >> For such objects, identified by having the >> _SELF_MANAGED_SHRINK_LIST set, make sure they end up on the >> correct list and defer purging to the shrinker. >> >> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> >> --- >> drivers/gpu/drm/i915/i915_gem.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem.c >> b/drivers/gpu/drm/i915/i915_gem.c >> index d0e642c82064..da972c8d45b1 100644 >> --- a/drivers/gpu/drm/i915/i915_gem.c >> +++ b/drivers/gpu/drm/i915/i915_gem.c >> @@ -1005,7 +1005,8 @@ i915_gem_madvise_ioctl(struct drm_device *dev, >> void *data, >> obj->ops->adjust_lru(obj); >> } >> - if (i915_gem_object_has_pages(obj)) { >> + if (i915_gem_object_has_pages(obj) || >> + i915_gem_object_has_self_managed_shrink_list(obj)) { > > Makes sense. > >> unsigned long flags; >> spin_lock_irqsave(&i915->mm.obj_lock, flags); >> @@ -1024,7 +1025,8 @@ i915_gem_madvise_ioctl(struct drm_device *dev, >> void *data, >> /* if the object is no longer attached, discard its backing >> storage */ >> if (obj->mm.madv == I915_MADV_DONTNEED && >> - !i915_gem_object_has_pages(obj)) >> + !i915_gem_object_has_pages(obj) && >> + !i915_gem_object_has_self_managed_shrink_list(obj)) >> i915_gem_object_truncate(obj); > > And it looks like this also matches the workings of lmem, where under > memory pressure we also just purge such objects, instead of moving > them, making sure to keep them first in the LRU? > > One thing is to maybe immediately discard already swapped-out objects > here, since the shrinker will be oblivious to them, and they sort of > just linger in swap until the object is destroyed? This might be a bit ugly if we want to avoid exposing even more gem object ops. Could we perhaps for the truncate callback only truncate swapped-out objects if we have a self-managed shrinker list? That will match all the current call-sites AFAICT since truncate is never called from the shrinker with the self-managed shrinker list... /Thomas > >> args->retained = obj->mm.madv != __I915_MADV_PURGED; >>
On 11/8/21 09:08, Thomas Hellström wrote: > > On 11/5/21 16:18, Matthew Auld wrote: >> On 05/11/2021 13:03, Thomas Hellström wrote: >>> Gem-TTM objects that are backed by shmem might have populated >>> page-vectors without having the Gem pages set. Those objects >>> aren't moved to the correct shrinker / purge list by the >>> gem_madvise. Furthermore they are purged directly on >>> MADV_DONTNEED rather than waiting for the shrinker to do that. >>> >>> For such objects, identified by having the >>> _SELF_MANAGED_SHRINK_LIST set, make sure they end up on the >>> correct list and defer purging to the shrinker. >>> >>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_gem.c | 6 ++++-- >>> 1 file changed, 4 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_gem.c >>> b/drivers/gpu/drm/i915/i915_gem.c >>> index d0e642c82064..da972c8d45b1 100644 >>> --- a/drivers/gpu/drm/i915/i915_gem.c >>> +++ b/drivers/gpu/drm/i915/i915_gem.c >>> @@ -1005,7 +1005,8 @@ i915_gem_madvise_ioctl(struct drm_device *dev, >>> void *data, >>> obj->ops->adjust_lru(obj); >>> } >>> - if (i915_gem_object_has_pages(obj)) { >>> + if (i915_gem_object_has_pages(obj) || >>> + i915_gem_object_has_self_managed_shrink_list(obj)) { >> >> Makes sense. >> >>> unsigned long flags; >>> spin_lock_irqsave(&i915->mm.obj_lock, flags); >>> @@ -1024,7 +1025,8 @@ i915_gem_madvise_ioctl(struct drm_device *dev, >>> void *data, >>> /* if the object is no longer attached, discard its backing >>> storage */ >>> if (obj->mm.madv == I915_MADV_DONTNEED && >>> - !i915_gem_object_has_pages(obj)) >>> + !i915_gem_object_has_pages(obj) && >>> + !i915_gem_object_has_self_managed_shrink_list(obj)) >>> i915_gem_object_truncate(obj); >> >> And it looks like this also matches the workings of lmem, where under >> memory pressure we also just purge such objects, instead of moving >> them, making sure to keep them first in the LRU? >> >> One thing is to maybe immediately discard already swapped-out objects >> here, since the shrinker will be oblivious to them, and they sort of >> just linger in swap until the object is destroyed? > > This might be a bit ugly if we want to avoid exposing even more gem > object ops. > > Could we perhaps for the truncate callback only truncate swapped-out > objects if we have a self-managed shrinker list? That will match all > the current call-sites AFAICT since truncate is never called from the > shrinker with the self-managed shrinker list... > Or actually, Thinking of it, if we don't have GEM pages, even if there are CPU PTEs set up, I can't see why we shouldn't purge this object immediately. I'll just revert that last hunk. /Thomas > /Thomas > >> >>> args->retained = obj->mm.madv != __I915_MADV_PURGED; >>>
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index d0e642c82064..da972c8d45b1 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1005,7 +1005,8 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data, obj->ops->adjust_lru(obj); } - if (i915_gem_object_has_pages(obj)) { + if (i915_gem_object_has_pages(obj) || + i915_gem_object_has_self_managed_shrink_list(obj)) { unsigned long flags; spin_lock_irqsave(&i915->mm.obj_lock, flags); @@ -1024,7 +1025,8 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data, /* if the object is no longer attached, discard its backing storage */ if (obj->mm.madv == I915_MADV_DONTNEED && - !i915_gem_object_has_pages(obj)) + !i915_gem_object_has_pages(obj) && + !i915_gem_object_has_self_managed_shrink_list(obj)) i915_gem_object_truncate(obj); args->retained = obj->mm.madv != __I915_MADV_PURGED;
Gem-TTM objects that are backed by shmem might have populated page-vectors without having the Gem pages set. Those objects aren't moved to the correct shrinker / purge list by the gem_madvise. Furthermore they are purged directly on MADV_DONTNEED rather than waiting for the shrinker to do that. For such objects, identified by having the _SELF_MANAGED_SHRINK_LIST set, make sure they end up on the correct list and defer purging to the shrinker. Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> --- drivers/gpu/drm/i915/i915_gem.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)