Am 14.11.2017 um 11:02 schrieb Michel Dänzer:
> On 13/11/17 10:54 AM, Christian König wrote:
>> Deleted BOs with the same reservation object can be reaped even if they
>> can't be reserved.
>>
>> v2: rebase and we still need to remove/add the BO from/to the LRU.
>> v3: fix remove/add one more time, cleanup the logic a bit
>> v4: we should still check if the eviction is valuable
>>
>> Signed-off-by: Christian König <christian.koenig at amd.com>
> [...]
>
>> - ret = reservation_object_trylock(bo->resv) ? 0 : -EBUSY;
>> - if (ret)
>> - continue;
>> + if (bo->resv == resv) {
>> + if (list_empty(&bo->ddestroy))
>> + continue;
>> +
> Superfluous blank line here.
>
>
>> + } 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.
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.
Christian.
>
>
>> break;
>> + else
>> + bo = NULL;
>> }
>
