buggy/weird behavior in ttm
Maarten Lankhorst
maarten.lankhorst at canonical.com
Mon Oct 15 11:34:17 PDT 2012
Op 15-10-12 17:37, Maarten Lankhorst schreef:
>>> To make multi-object reservation work, the fix is to add a ticket "lock" of which all the
>>> reservation objects are a nested lock of. Since in this case the ticket lock would prevent
>>> deadlocks, this is acceptable. Having 2 ticket 'locks' at the same time would count as
>>> deadlock, rightfully. If you hold a reservation from a ticket, then try to reserve without
>>> a ticket, it counts as deadlock too. See below for some examples I was using to test.
>> But if a ticket lock can be used to abstract a locking sequence that we know will never deadlock,
>> why can't we abstract locking from a list with atomic list removal with another "lock", and make sure we get the order right between them?
> No, see the test below, lockdep won't be fooled by your diversions that easily!! :-)
>
> However, if the following rules are valid, life becomes a lot easier:
> 1. items on the ddestroy list are not allowed to have a new sync object attached, only
> reservations for destruction of this object are allowed.
> 2. the only valid thing at this point left is unmapping from vm space and destruction
> of buffer backing in memory or vram
> 3. All reservations outside of ttm_bo_cleanup_refs(or_queue) are forbidden.
> (XXX: check if an exception needed for drivers to accomplish this destruction?)
>
> Adding a WARN_ON(!list_empty(&bo->ddestroy)); to ttm_bo_reserve and ttm_eu_reserve_buffers
> should be enough to catch violators of the above rules.
>
> If those rules hold, destruction becomes a lot more straightforward.
> ttm_bo_swapout and ttm_mem_evict_first can both do the reserve with lru_lock held,
> which will be guaranteed to succeed, and the buffer is also guaranteed to be
> on the ddestroy list still since we haven't dropped the lock yet.
>
> So with a reservation, lru_lock held, and entry on the ddestroy list still alive:
>
> Do a trywait on the bo, if it returns -EBUSY, and no_wait_gpu is unset,
> get a reference to bo->sync_obj. unreserve.
>
> If -EBUSY was returned and no_wait_gpu is set, unlock lru_lock and return error.
>
> If -EBUSY and no_wait_gpu was not set, unlock lru_lock, do a wait on our copy of
> bo->sync_obj, and unref it, return if wait fails. If wait hasn't failed, retake
> lru_lock.
>
> Check if the if the ddestroy entry is gone. If so, someone else was faster,
> return 0. If not simply remove bo from all lists without reservation,
> unref bo->sync_obj**, and perform the remaining cleanup.
>
> As far as I can see, this wouldn't leave open a race even if 2 threads do the same,
> although the second thread might return 0 to signal success before the backing is
> gone, but this can also happen right now. It's even harmless, since in those cases
> the functions calling these functions would simply call them one more time, and this
> time the destroyed buffer would definitely not be on the swap/lru lists any more.
>
> It would also keep current behavior the same as far as I can tell, where multiple
> waiters could wait for the same bo to be destroyed.
>
> **) Yes strictly speaking a violation of fence rules, but in this case the buffer
> already dropped to 0 references, and there's a BUG_ON in ttm_bo_release_list that
> would get triggered otherwise. Can alternatively be fixed by doing a BUG_ON in
> ttm_bo_release_list if there is a sync_obj that's not in the signaled state.
Attached 3 followup patches to show what I mean, they're based on my tree,
which means with all patches applied that I posted on friday.
This is not thoroughly tested, but I think it should work.
First is fixing radeon not to crash on calling move_notify from release_list.
Second moves the memory cleanup to ttm_bo_cleanup_memtype_use, which is more
readable and a more logical place to put it.
Third cleans up how ttm_bo_cleanup_refs is called.
Also available on http://cgit.freedesktop.org/~mlankhorst/linux/log/?h=v10-wip
If this looks ok I'll send those below out when the rest of the patches I
posted on friday are reviewed. :-)
======
drm/radeon: allow move_notify to be called without reservation
The few places that care should have those checks instead.
This allow destruction of bo backed memory without a reservation.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst at canonical.com>
---
drivers/gpu/drm/radeon/radeon_gart.c | 1 -
drivers/gpu/drm/radeon/radeon_object.c | 2 +-
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_gart.c b/drivers/gpu/drm/radeon/radeon_gart.c
index 0ee5e29..701b215 100644
--- a/drivers/gpu/drm/radeon/radeon_gart.c
+++ b/drivers/gpu/drm/radeon/radeon_gart.c
@@ -1044,7 +1044,6 @@ void radeon_vm_bo_invalidate(struct radeon_device *rdev,
{
struct radeon_bo_va *bo_va;
- BUG_ON(!radeon_bo_is_reserved(bo));
list_for_each_entry(bo_va, &bo->va, bo_list) {
bo_va->valid = false;
}
diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c
index 5b959b6..fa954d7 100644
--- a/drivers/gpu/drm/radeon/radeon_object.c
+++ b/drivers/gpu/drm/radeon/radeon_object.c
@@ -539,7 +539,7 @@ void radeon_bo_get_tiling_flags(struct radeon_bo *bo,
int radeon_bo_check_tiling(struct radeon_bo *bo, bool has_moved,
bool force_drop)
{
- BUG_ON(!radeon_bo_is_reserved(bo));
+ BUG_ON(!radeon_bo_is_reserved(bo) && !force_drop);
if (!(bo->tiling_flags & RADEON_TILING_SURFACE))
return 0;
======
drm/ttm: remove ttm_bo_cleanup_memtype_use
move to release_list instead
Signed-off-by: Maarten Lankhorst <maarten.lankhorst at canonical.com>
---
drivers/gpu/drm/ttm/ttm_bo.c | 45 ++++++++++++------------------------------
1 file changed, 13 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index dd6a3e6..9b95e96 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -146,8 +146,16 @@ static void ttm_bo_release_list(struct kref *list_kref)
BUG_ON(!list_empty(&bo->lru));
BUG_ON(!list_empty(&bo->ddestroy));
- if (bo->ttm)
+ if (bo->bdev->driver->move_notify)
+ bo->bdev->driver->move_notify(bo, NULL);
+
+ if (bo->ttm) {
+ ttm_tt_unbind(bo->ttm);
ttm_tt_destroy(bo->ttm);
+ bo->ttm = NULL;
+ }
+ ttm_bo_mem_put(bo, &bo->mem);
+
atomic_dec(&bo->glob->bo_count);
if (bo->destroy)
bo->destroy(bo);
@@ -465,35 +473,6 @@ out_err:
return ret;
}
-/**
- * Call bo::reserved.
- * Will release GPU memory type usage on destruction.
- * This is the place to put in driver specific hooks to release
- * driver private resources.
- * Will release the bo::reserved lock.
- */
-
-static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo)
-{
- if (bo->bdev->driver->move_notify)
- bo->bdev->driver->move_notify(bo, NULL);
-
- if (bo->ttm) {
- ttm_tt_unbind(bo->ttm);
- ttm_tt_destroy(bo->ttm);
- bo->ttm = NULL;
- }
- ttm_bo_mem_put(bo, &bo->mem);
-
- atomic_set(&bo->reserved, 0);
-
- /*
- * Make processes trying to reserve really pick it up.
- */
- smp_mb__after_atomic_dec();
- wake_up_all(&bo->event_queue);
-}
-
static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
{
struct ttm_bo_device *bdev = bo->bdev;
@@ -517,8 +496,9 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
put_count = ttm_bo_del_from_lru(bo);
+ atomic_set(&bo->reserved, 0);
+ wake_up_all(&bo->event_queue);
spin_unlock(&glob->lru_lock);
- ttm_bo_cleanup_memtype_use(bo);
ttm_bo_list_ref_sub(bo, put_count, true);
@@ -608,8 +588,9 @@ retry:
list_del_init(&bo->ddestroy);
++put_count;
+ atomic_set(&bo->reserved, 0);
+ wake_up_all(&bo->event_queue);
spin_unlock(&glob->lru_lock);
- ttm_bo_cleanup_memtype_use(bo);
ttm_bo_list_ref_sub(bo, put_count, true);
======
drm/ttm: add sense to ttm_bo_cleanup_refs
Signed-off-by: Maarten Lankhorst <maarten.lankhorst at canonical.com>
---
drivers/gpu/drm/ttm/ttm_bo.c | 98 ++++++++++++++------------------
drivers/gpu/drm/ttm/ttm_execbuf_util.c | 1
2 files changed, 45 insertions(+), 54 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 9b95e96..34d4bb1 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -295,6 +295,8 @@ int ttm_bo_reserve(struct ttm_buffer_object *bo,
int put_count = 0;
int ret;
+ WARN_ON(!list_empty_careful(&bo->ddestroy));
+
spin_lock(&glob->lru_lock);
ret = ttm_bo_reserve_locked(bo, interruptible, no_wait, use_sequence,
sequence);
@@ -522,14 +524,15 @@ queue:
* If bo idle, remove from delayed- and lru lists, and unref.
* If not idle, do nothing.
*
+ * Must be called with lru_lock and reservation held, this function
+ * will drop both before returning.
+ *
* @interruptible Any sleeps should occur interruptibly.
- * @no_wait_reserve Never wait for reserve. Return -EBUSY instead.
* @no_wait_gpu Never wait for gpu. Return -EBUSY instead.
*/
static int ttm_bo_cleanup_refs(struct ttm_buffer_object *bo,
bool interruptible,
- bool no_wait_reserve,
bool no_wait_gpu)
{
struct ttm_bo_device *bdev = bo->bdev;
@@ -537,53 +540,45 @@ static int ttm_bo_cleanup_refs(struct ttm_buffer_object *bo,
struct ttm_bo_global *glob = bo->glob;
int put_count;
int ret = 0;
- void *sync_obj;
-
-retry:
- spin_lock(&glob->lru_lock);
- ret = ttm_bo_reserve_locked(bo, interruptible,
- no_wait_reserve, false, 0);
-
- if (unlikely(ret != 0)) {
- spin_unlock(&glob->lru_lock);
- return ret;
- }
+ ret = ttm_bo_wait(bo, false, false, true);
- if (unlikely(list_empty(&bo->ddestroy))) {
+ if (ret && no_wait_gpu) {
atomic_set(&bo->reserved, 0);
wake_up_all(&bo->event_queue);
spin_unlock(&glob->lru_lock);
- return 0;
- }
- ret = ttm_bo_wait(bo, false, false, true);
-
- if (ret) {
- if (no_wait_gpu) {
- atomic_set(&bo->reserved, 0);
- wake_up_all(&bo->event_queue);
- spin_unlock(&glob->lru_lock);
- return ret;
- }
+ return ret;
+ } else if (ret) {
+ void *sync_obj;
/**
- * Take a reference to the fence and unreserve, if the wait
- * was succesful and no new sync_obj was attached,
- * ttm_bo_wait in retry will return ret = 0, and end the loop.
+ * Take a reference to the fence and unreserve,
+ * at this point the buffer should be dead, so
+ * no new sync objects can be attached.
*/
-
sync_obj = driver->sync_obj_ref(&bo->sync_obj);
atomic_set(&bo->reserved, 0);
wake_up_all(&bo->event_queue);
spin_unlock(&glob->lru_lock);
- ret = driver->sync_obj_wait(bo->sync_obj, false, interruptible);
+ ret = driver->sync_obj_wait(sync_obj, false, interruptible);
driver->sync_obj_unref(&sync_obj);
if (ret)
return ret;
- goto retry;
+ spin_lock(&glob->lru_lock);
+ } else {
+ atomic_set(&bo->reserved, 0);
+ wake_up_all(&bo->event_queue);
+ }
+
+ if (unlikely(list_empty(&bo->ddestroy))) {
+ spin_unlock(&glob->lru_lock);
+ return 0;
}
+ if (bo->sync_obj)
+ driver->sync_obj_unref(&bo->sync_obj);
+
put_count = ttm_bo_del_from_lru(bo);
list_del_init(&bo->ddestroy);
++put_count;
@@ -625,9 +620,12 @@ static int ttm_bo_delayed_delete(struct ttm_bo_device *bdev, bool remove_all)
kref_get(&nentry->list_kref);
}
- spin_unlock(&glob->lru_lock);
- ret = ttm_bo_cleanup_refs(entry, false, !remove_all,
- !remove_all);
+ ret = ttm_bo_reserve_locked(entry, false, !remove_all);
+ if (ret)
+ spin_unlock(&glob->lru_lock);
+ else
+ ret = ttm_bo_cleanup_refs(entry, false, !remove_all);
+
kref_put(&entry->list_kref, ttm_bo_release_list);
entry = nentry;
@@ -778,18 +776,6 @@ retry:
bo = list_first_entry(&man->lru, struct ttm_buffer_object, lru);
kref_get(&bo->list_kref);
- if (!list_empty(&bo->ddestroy)) {
- spin_unlock(&glob->lru_lock);
- ret = ttm_bo_cleanup_refs(bo, interruptible,
- true, no_wait_gpu);
- kref_put(&bo->list_kref, ttm_bo_release_list);
-
- if (likely(ret == 0 || ret == -ERESTARTSYS))
- return ret;
-
- goto retry;
- }
-
ret = ttm_bo_reserve_locked(bo, false, true, false, 0);
if (unlikely(ret == -EBUSY)) {
@@ -808,6 +794,12 @@ retry:
goto retry;
}
+ if (!list_empty(&bo->ddestroy)) {
+ ret = ttm_bo_cleanup_refs(bo, interruptible, no_wait_gpu);
+ kref_put(&bo->list_kref, ttm_bo_release_list);
+ return ret;
+ }
+
put_count = ttm_bo_del_from_lru(bo);
spin_unlock(&glob->lru_lock);
@@ -1718,14 +1710,6 @@ static int ttm_bo_swapout(struct ttm_mem_shrink *shrink)
struct ttm_buffer_object, swap);
kref_get(&bo->list_kref);
- if (!list_empty(&bo->ddestroy)) {
- spin_unlock(&glob->lru_lock);
- (void) ttm_bo_cleanup_refs(bo, false, false, false);
- kref_put(&bo->list_kref, ttm_bo_release_list);
- spin_lock(&glob->lru_lock);
- continue;
- }
-
/**
* Reserve buffer. Since we unlock while sleeping, we need
* to re-check that nobody removed us from the swap-list while
@@ -1741,6 +1725,12 @@ static int ttm_bo_swapout(struct ttm_mem_shrink *shrink)
}
}
+ if (!list_empty(&bo->ddestroy)) {
+ ret = ttm_bo_cleanup_refs(bo, false, false);
+ kref_put(&bo->list_kref, ttm_bo_release_list);
+ return ret;
+ }
+
BUG_ON(ret != 0);
put_count = ttm_bo_del_from_lru(bo);
spin_unlock(&glob->lru_lock);
diff --git a/drivers/gpu/drm/ttm/ttm_execbuf_util.c b/drivers/gpu/drm/ttm/ttm_execbuf_util.c
index 51b5e97..3ea2457 100644
--- a/drivers/gpu/drm/ttm/ttm_execbuf_util.c
+++ b/drivers/gpu/drm/ttm/ttm_execbuf_util.c
@@ -153,6 +153,7 @@ retry:
struct ttm_buffer_object *bo = entry->bo;
retry_this_bo:
+ WARN_ON(!list_empty_careful(&bo->ddestroy));
ret = ttm_bo_reserve_locked(bo, true, true, true, val_seq);
switch (ret) {
case 0:
More information about the dri-devel
mailing list