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

[Tvrtko Ursulin]

On 25/01/2019 13:46, Chris Wilson wrote:
> Quoting Chris Wilson (2019-01-25 13:38:09)
>> Quoting Tvrtko Ursulin (2019-01-25 10:46:19)
>>>
>>> On 22/01/2019 14:19, Chris Wilson wrote:
>>>> Quoting Tvrtko Ursulin (2019-01-18 16:03:27)
>>>>>
>>>>> On 18/01/2019 14:00, 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).
>>>>>
>>>>> There is still no mention of list consolidation.
>>>>
>>>> "Note that discarding the MRU (currently implemented as a pair of lists,
>>>> to avoid scanning the active list for a NONBLOCKING search)"
>>>>
>>>> Is that enough to make it clear?
>>>
>>> How about:
>>>
>>> "Note that discarding the MRU, which is implemented both by not doing
>>> list operations while tracking activity, and by replacing
>>> active/inactive list with a single bound list, is unlikely..."
>>>
>>> ?
>>>
>>> Plus I really want a changelog and that stale comment which talks about
>>> active/inactive *lists* updated.
>>
>> But there's active/inactive lists within the list...
> 
> @@ -136,17 +136,15 @@ i915_gem_evict_something(struct i915_address_space *vm,
>          trace_i915_gem_evict(vm, min_size, alignment, flags);
> 
>          /*
> -        * The goal is to evict objects and amalgamate space in LRU order.
> -        * The oldest idle objects reside on the inactive list, which is in
> -        * retirement order. The next objects to retire are those in flight,
> -        * on the active list, again in retirement order.
> +        * The goal is to evict objects and amalgamate space in rough LRU order.
> +        * Since both active and inactive objects reside on the same list,
> +        * in a mix of creation and last scanned order, as we process the list
> +        * we sort it into inactive/active, which keeps the active portion
> +        * in a rough MRU order.
>           *
>           * The retirement sequence is thus:
>           *   1. Inactive objects (already retired, random order)
>           *   2. Active objects (will stall on unbinding, oldest scanned first)
> -        *
> -        * On each list, the oldest objects lie at the HEAD with the freshest
> -        * object on the TAIL.
>           */

Yep, I'm happy with that.

Regards,

Tvrtko