[PATCH 2/2] drm/ttm: optimize ttm_mem_evict_first v4

[Michel Dänzer]

On 14/11/17 01:18 PM, Christian König wrote:
> Am 14.11.2017 um 11:02 schrieb Michel Dänzer:
>> On 13/11/17 10:54 AM, Christian König wrote:
>>>
>>> +            } else {
>>> +                locked = reservation_object_trylock(bo->resv);
>>> +                if (!locked)
>>> +                    continue;
>>> +            }
>>>                 if (place && !bdev->driver->eviction_valuable(bo,
>>>                                         place)) {
>>> -                reservation_object_unlock(bo->resv);
>>> -                ret = -EBUSY;
>>> +                if (locked)
>>> +                    reservation_object_unlock(bo->resv);
>> I think we need to always set bo = NULL before continue, otherwise we
>> can end up trying to evict the very last BO even though we didn't
>> consider it suitable for eviction (and if
>> bdev->driver->eviction_valuable returned false, with locked = true even
>> though we already unlocked bo->resv).
>>
>>
>>> -        if (!ret)
>>> +        if (&bo->lru != &man->lru[i])
>> I'm not sure what the purpose of this test is, can you explain?
> 
> It prevents the case you described above.
> 
> bo is the element variable of the loop, so we can't set it to NULL
> inside the loop.

Yeah, that was a brainfart. :)


> Instead we check after the loop if we reached the end of the list, if
> yes we set the bo variable to NULL, otherwise we have found a valid
> entry and can break out of the for loop.
>
> To make it clearer we could also test "i" after the outer loop instead
> of bo for being NULL. That would at least be easier to understand I think.

That would still require detecting early termination of the inner loop
and breaking out of the outer loop, so it doesn't buy anything.

The clearest solution might be:

	for (i = 0; !bo && i < TTM_MAX_BO_PRIORITY; ++i) {
		struct ttm_buffer_object *candidate;

		list_for_each_entry(candidate, &man->lru[i], lru) {
			[if/continue using candidate instead of bo]

			bo = candidate;
			break;
		}
	}

	if (!bo) {
		[...]


Alternatively, this might clarify the test for early termination:

		/* If the inner loop terminated early, we have our candidate */
		if (&bo->lru != &man->lru[i])
 			break;

		bo = NULL;
	}

(could even move the bo = NULL to the end of the outer loop's for ()
line)


I'll leave it up to you which you choose, either way

Reviewed-and-Tested-by: Michel Dänzer <michel.daenzer at amd.com>


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer