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@xxxxxxx>
[...]
- 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;
}
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
