[Intel-gfx] [PATCH 31/46] drm/i915: Stop tracking MRU activity on VMA

Chris Wilson chris at chris-wilson.co.uk
Wed Jan 16 16:37:37 UTC 2019


Quoting Tvrtko Ursulin (2019-01-16 16:27:16)
> 
> On 07/01/2019 11:54, Chris Wilson wrote:
> > Our goal is to remove struct_mutex and replace it with fine grained
> > locking. One of the thorny issues is our eviction logic for reclaiming
> > space for an execbuffer (or GTT mmaping, among a few other examples).
> > While eviction itself is easy to move under a per-VM mutex, performing
> > the activity tracking is less agreeable. One solution is not to do any
> > MRU tracking and do a simple coarse evaluation during eviction of
> > active/inactive, with a loose temporal ordering of last
> > insertion/evaluation. That keeps all the locking constrained to when we
> > are manipulating the VM itself, neatly avoiding the tricky handling of
> > possible recursive locking during execbuf and elsewhere.
> > 
> > Note that discarding the MRU is unlikely to impact upon our efficiency
> > to reclaim VM space (where we think a LRU model is best) as our
> > current strategy is to use random idle replacement first before doing
> > a search, and over time the use of softpinned 48b per-ppGTT is growing
> > (thereby eliminating any need to perform any eviction searches, in
> > theory at least).
> 
> I've noticed you did some changes since I last reviewed it, but there is 
> not change log so I have to find them manually. Also, the ones you did 
> not do I suppose means you disagree with?

I updated the commit msg wrt to the changes

> On the commit message my comment was that I think you should mention the 
> removal of active/inactive lists in favour of a single list.

I did, did I not?

> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >   drivers/gpu/drm/i915/i915_gem.c               | 10 +--
> >   drivers/gpu/drm/i915/i915_gem_evict.c         | 71 ++++++++++++-------
> >   drivers/gpu/drm/i915/i915_gem_gtt.c           | 15 ++--
> >   drivers/gpu/drm/i915/i915_gem_gtt.h           | 26 +------
> >   drivers/gpu/drm/i915/i915_gem_shrinker.c      |  8 ++-
> >   drivers/gpu/drm/i915/i915_gem_stolen.c        |  3 +-
> >   drivers/gpu/drm/i915/i915_gpu_error.c         | 37 +++++-----
> >   drivers/gpu/drm/i915/i915_vma.c               |  9 +--
> >   .../gpu/drm/i915/selftests/i915_gem_evict.c   |  4 +-
> >   drivers/gpu/drm/i915/selftests/i915_gem_gtt.c |  2 +-
> >   10 files changed, 84 insertions(+), 101 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 83fb02dab18c..6ed44aeee583 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -254,10 +254,7 @@ i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data,
> >   
> >       pinned = ggtt->vm.reserved;
> >       mutex_lock(&dev->struct_mutex);
> > -     list_for_each_entry(vma, &ggtt->vm.active_list, vm_link)
> > -             if (i915_vma_is_pinned(vma))
> > -                     pinned += vma->node.size;
> > -     list_for_each_entry(vma, &ggtt->vm.inactive_list, vm_link)
> > +     list_for_each_entry(vma, &ggtt->vm.bound_list, vm_link)
> >               if (i915_vma_is_pinned(vma))
> >                       pinned += vma->node.size;
> >       mutex_unlock(&dev->struct_mutex);
> > @@ -1540,13 +1537,10 @@ static void i915_gem_object_bump_inactive_ggtt(struct drm_i915_gem_object *obj)
> >       GEM_BUG_ON(!i915_gem_object_has_pinned_pages(obj));
> >   
> >       for_each_ggtt_vma(vma, obj) {
> > -             if (i915_vma_is_active(vma))
> > -                     continue;
> > -
> >               if (!drm_mm_node_allocated(&vma->node))
> >                       continue;
> >   
> > -             list_move_tail(&vma->vm_link, &vma->vm->inactive_list);
> > +             list_move_tail(&vma->vm_link, &vma->vm->bound_list);
> >       }
> >   
> >       i915 = to_i915(obj->base.dev);
> > diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
> > index 02b83a5ed96c..a76f65fe86be 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> > @@ -127,14 +127,10 @@ i915_gem_evict_something(struct i915_address_space *vm,
> >       struct drm_i915_private *dev_priv = vm->i915;
> >       struct drm_mm_scan scan;
> >       struct list_head eviction_list;
> > -     struct list_head *phases[] = {
> > -             &vm->inactive_list,
> > -             &vm->active_list,
> > -             NULL,
> > -     }, **phase;
> >       struct i915_vma *vma, *next;
> >       struct drm_mm_node *node;
> >       enum drm_mm_insert_mode mode;
> > +     struct i915_vma *active;
> >       int ret;
> >   
> >       lockdep_assert_held(&vm->i915->drm.struct_mutex);
> 
> There is this a comment around here not shown in the diff which talks 
> about active and inactive lists. Plus it is misleading on the lists 
> ordering now.

The sequence is still in tact. Just a minor mental adjustment that we
scan both in the same list, with softer ordering. Par for the course as
comments go.

> > @@ -170,17 +166,46 @@ i915_gem_evict_something(struct i915_address_space *vm,
> >        */
> >       if (!(flags & PIN_NONBLOCK))
> >               i915_retire_requests(dev_priv);
> > -     else
> > -             phases[1] = NULL;
> >   
> >   search_again:
> > +     active = NULL;
> >       INIT_LIST_HEAD(&eviction_list);
> > -     phase = phases;
> > -     do {
> > -             list_for_each_entry(vma, *phase, vm_link)
> > -                     if (mark_free(&scan, vma, flags, &eviction_list))
> > -                             goto found;
> > -     } while (*++phase);
> > +     list_for_each_entry_safe(vma, next, &vm->bound_list, vm_link) {
> > +             /*
> > +              * We keep this list in a rough least-recently scanned order
> > +              * of active elements (inactive elements are cheap to reap).
> > +              * New entries are added to the end, and we move anything we
> > +              * scan to the end. The assumption is that the working set
> > +              * of applications is either steady state (and thanks to the
> > +              * userspace bo cache it almost always is) or volatile and
> > +              * frequently replaced after a frame, which are self-evicting!
> > +              * Given that assumption, the MRU order of the scan list is
> > +              * fairly static, and keeping it in least-recently scan order
> > +              * is suitable.
> > +              *
> > +              * To notice when we complete one full cycle, we record the
> > +              * first active element seen, before moving it to the tail.
> > +              */
> 
> This is one change since v1 I spotted and it is a good one.

The intent was to highlight it in the commitmsg by rewriting the
commitmsg...
-Chris


More information about the Intel-gfx mailing list