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@xxxxxxxxxxxxxxxxxx>
> > ---
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
