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
