Re: [PATCH 3/6] drm/ttm: call ttm_bo_cleanup_refs with reservation and lru lock held

On 11/28/2012 12:25 PM, Maarten Lankhorst wrote:

By removing the unlocking of lru and retaking it immediately, a race is removed where the bo is taken off the swap list or the lru list between the unlock and relock. As such the cleanup_refs code can be simplified, it will attempt to call ttm_bo_wait non-blockingly, and if it fails it will drop the locks and perform a blocking wait, or return an error if no_wait_gpu was set.

The need for looping is also eliminated, since swapout and evict_mem_first will always follow the destruction path, so no new fence is allowed to be attached. As far as I can see this may already have been the case, but the unlocking / relocking required a complicated loop to deal with re-reservation.

The downside is that ttm_bo_cleanup_memtype_use is no longer called with reservation held, so drivers must be aware that move_notify with a null parameter doesn't require a reservation.

Why can't we unreserve *after* ttm_bo_cleanup_memtype_use? That's not immediately clear from this patch.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@canonical.com

drivers/gpu/drm/ttm/ttm_bo.c | 112 +++++++++++++++++++++++-------------------- 1 file changed, 60 insertions(+), 52 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 202fc20..02b275b 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -486,14 +486,6 @@ static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo) 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)

@@ -515,6 +507,9 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo) put_count = ttm_bo_del_from_lru(bo);

spin_unlock(&glob->lru_lock);

• atomic_set(&bo->reserved, 0);
  

• wake_up_all(&bo->event_queue);
  

I think (although I'm not 100% sure) that if we use atomic_set() to unreserve, and it's not followed by a spin_unlock(), we need to insert a memory barrier, like is done above in the removed code, otherwise memory operations protected by reserve may be reordered until after reservation.

ttm_bo_cleanup_memtype_use(bo);

ttm_bo_list_ref_sub(bo, put_count, true);

@@ -543,68 +538,72 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo) }

/**

• • function ttm_bo_cleanup_refs

• • function ttm_bo_cleanup_refs_and_unlock
  • 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)
  

+static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,

• 			  bool interruptible,
  

• 			  bool no_wait_gpu)
  

  { struct ttm_bo_device *bdev = bo->bdev;
• struct ttm_bo_driver *driver = bdev->driver; struct ttm_bo_global *glob = bo->glob; int put_count; int ret = 0;

-retry: spin_lock(&bdev->fence_lock);

• ret = ttm_bo_wait(bo, false, interruptible, no_wait_gpu);
• spin_unlock(&bdev->fence_lock);

• ret = ttm_bo_wait(bo, false, false, true);

• if (unlikely(ret != 0))

• if (ret && no_wait_gpu) {

• spin_unlock(&bdev->fence_lock);
  

• atomic_set(&bo->reserved, 0);
  

• wake_up_all(&bo->event_queue);
  

• spin_unlock(&glob->lru_lock);
  

  return ret;
• } else if (ret) {

• void *sync_obj;
  
  

-retry_reserve:

• spin_lock(&glob->lru_lock);
• if (unlikely(list_empty(&bo->ddestroy))) {

• /**
  

•  * 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);
  

• spin_unlock(&bdev->fence_lock);
  

• atomic_set(&bo->reserved, 0);
  

• wake_up_all(&bo->event_queue);
  

  spin_unlock(&glob->lru_lock);

• return 0;
  

• }

• ret = ttm_bo_reserve_locked(bo, false, true, false, 0);

• if (unlikely(ret == -EBUSY)) {

• spin_unlock(&glob->lru_lock);
  

• if (likely(!no_wait_reserve))
  

• 	ret = ttm_bo_wait_unreserved(bo, interruptible);
  

• if (unlikely(ret != 0))
  

• ret = driver->sync_obj_wait(sync_obj, false, interruptible);
  

• driver->sync_obj_unref(&sync_obj);
  

• if (ret)
  return ret;
  
  

• goto retry_reserve;
  

• }
• BUG_ON(ret != 0);

• /* remove sync_obj with ttm_bo_wait */
  

• spin_lock(&bdev->fence_lock);
  

• ret = ttm_bo_wait(bo, false, false, true);
  

• spin_unlock(&bdev->fence_lock);
  
  

• /**

• * We can re-check for sync object without taking
  

• * the bo::lock since setting the sync object requires
  

• * also bo::reserved. A busy object at this point may
  

• * be caused by another thread recently starting an accelerated
  

• * eviction.
  

• */
  

• WARN_ON(ret);
  
  

• if (unlikely(bo->sync_obj)) {

• spin_lock(&glob->lru_lock);
  

• } else {

• spin_unlock(&bdev->fence_lock);
  

  atomic_set(&bo->reserved, 0); wake_up_all(&bo->event_queue);
• }
• if (unlikely(list_empty(&bo->ddestroy))) { spin_unlock(&glob->lru_lock);

• goto retry;
  

• return 0;
  

  }

  put_count = ttm_bo_del_from_lru(bo);

@@ -647,9 +646,13 @@ 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, false, 0);
  

• if (!ret)
  

• 	ret = ttm_bo_cleanup_refs_and_unlock(entry, false,
  

• 					     !remove_all);
  

• else
  

• 	spin_unlock(&glob->lru_lock);
  

• kref_put(&entry->list_kref, ttm_bo_release_list); entry = nentry;

@@ -803,9 +806,13 @@ retry: kref_get(&bo->list_kref);

if (!list_empty(&bo->ddestroy)) {

• spin_unlock(&glob->lru_lock);
  

• ret = ttm_bo_cleanup_refs(bo, interruptible,
  

• 			  no_wait_reserve, no_wait_gpu);
  

• ret = ttm_bo_reserve_locked(bo, interruptible, no_wait_reserve, false, 0);
  

• if (!ret)
  

• 	ret = ttm_bo_cleanup_refs_and_unlock(bo, interruptible,
  

• 					     no_wait_gpu);
  

• else
  

• 	spin_unlock(&glob->lru_lock);
  

• kref_put(&bo->list_kref, ttm_bo_release_list);

  return ret;

@@ -1799,8 +1806,9 @@ static int ttm_bo_swapout(struct ttm_mem_shrink *shrink) kref_get(&bo->list_kref);

if (!list_empty(&bo->ddestroy)) {

• 	spin_unlock(&glob->lru_lock);
  

• 	(void) ttm_bo_cleanup_refs(bo, false, false, false);
  

• 	ttm_bo_reserve_locked(bo, false, false, false, 0);
  

• 	ttm_bo_cleanup_refs_and_unlock(bo, false, false);
  

• kref_put(&bo->list_kref, ttm_bo_release_list);
  spin_lock(&glob->lru_lock);
  continue;