buggy/weird behavior in ttm
Jerome Glisse
j.glisse at gmail.com
Mon Oct 15 12:44:29 PDT 2012
On Mon, Oct 15, 2012 at 08:34:17PM +0200, Maarten Lankhorst wrote:
> 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. :-)
The bo_va list is protected by the reservation. I am not against removing the
check as it's should be ok in cleanup path to not hold the reservation assuming
no other thread will call into radeon with this same bo.
Cheers,
Jerome Glisse
> ======
>
> 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:
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
More information about the dri-devel
mailing list