drm/ttm: change fence_lock to inner lock

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.

Signed-off-by Maarten Lankhorst <[email protected]>

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

drivers/gpu/drm/ttm/ttm_bo.c +0 -8

index 202fc20..02b275b 100644
@@ -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)

drivers/gpu/drm/ttm/ttm_bo.c +57 -44

  		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;

Quoted 13 lines

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.

Quoted 3 lines

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.

Op 28-11-12 12:54, Thomas Hellstrom schreef:

Quoted 14 lines · Maarten Lankhorst

Quoted 13 lines

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.

Quoted 5 lines

Quoted 3 lines

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.

Because we would hold the reservation while waiting and with the object still on swap and lru lists still, that would defeat the whole purpose of keeping the object on multiple lists, plus break current code that assumes bo's on the those lists can always be reserved.

the if (ret && !no_wait_gpu) path has to drop the reservation and lru lock, and isn't guaranteed to be able to retake it. Maybe it could be guaranteed now, but I'm sure that would not be the case if the reservations were shared across devices.

Quoted 25 lines

Op 28-11-12 12:54, Thomas Hellstrom schreef:

Quoted 14 lines · Maarten Lankhorst

Quoted 13 lines

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.

Quoted 5 lines

Quoted 3 lines

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.

Because we would hold the reservation while waiting and with the object still on swap and lru lists still, that would defeat the whole purpose of keeping the object on multiple lists, plus break current code that assumes bo's on the those lists can always be reserved.

Quoted 4 lines

the if (ret && !no_wait_gpu) path has to drop the reservation and lru lock, and isn't guaranteed to be able to retake it. Maybe it could be guaranteed now, but I'm sure that would not be the case if the reservations were shared across devices.

The evict path removes the BO from the LRU lists, drops the LRU lock but hangs on to the reservation, and in case the wait goes wrong, re-adds the bo to the LRU lists and returns an error.

Isn't it possible to do the same in the !no_wait_gpu case? /Thomas

Op 28-11-12 14:21, Thomas Hellstrom schreef:

Quoted 30 lines · Maarten Lankhorst

Quoted 29 lines

Op 28-11-12 12:54, Thomas Hellstrom schreef:

Quoted 14 lines · Maarten Lankhorst

Quoted 13 lines

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.

Quoted 5 lines

Quoted 3 lines

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.

Because we would hold the reservation while waiting and with the object still on swap and lru lists still, that would defeat the whole purpose of keeping the object on multiple lists, plus break current code that assumes bo's on the those lists can always be reserved.

the if (ret && !no_wait_gpu) path has to drop the reservation and lru lock, and isn't guaranteed to be able to retake it. Maybe it could be guaranteed now, but I'm sure that would not be the case if the reservations were shared across devices.

Quoted 2 lines

The evict path removes the BO from the LRU lists, drops the LRU lock but hangs on to the reservation, and in case the wait goes wrong, re-adds the bo to the LRU lists and returns an error.

If you really want to, we could hang on to the !no_wait_gpu path, wait shouldn't ever fail there, so I suppose leaving it off the lru lists and not re-add on any list in case of wait fail is fine. It's still on the ddestroy list in that case, so not adding it back to the other lists is harmless.

Quoted 33 lines

Op 28-11-12 14:21, Thomas Hellstrom schreef:

Quoted 32 lines · Maarten Lankhorst

Quoted 25 lines

Op 28-11-12 12:54, Thomas Hellstrom schreef:

Quoted 14 lines · Maarten Lankhorst

Quoted 13 lines

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.

Quoted 5 lines

Quoted 3 lines

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.

Because we would hold the reservation while waiting and with the object still on swap and lru lists still, that would defeat the whole purpose of keeping the object on multiple lists, plus break current code that assumes bo's on the those lists can always be reserved.

Quoted 4 lines

the if (ret && !no_wait_gpu) path has to drop the reservation and lru lock, and isn't guaranteed to be able to retake it. Maybe it could be guaranteed now, but I'm sure that would not be the case if the reservations were shared across devices.

The evict path removes the BO from the LRU lists, drops the LRU lock but hangs on to the reservation, and in case the wait goes wrong, re-adds the bo to the LRU lists and returns an error.

Quoted 4 lines

If you really want to, we could hang on to the !no_wait_gpu path, wait shouldn't ever fail there, so I suppose leaving it off the lru lists and not re-add on any list in case of wait fail is fine. It's still on the ddestroy list in that case, so not adding it back to the other lists is harmless.

Well I'm a bit afraid that theoretically, other callers may have a bo reserved, while cleanup_refs_and_unlock more or less runs the whole destroy path on that buffer. Sure, we have control over those other reservers, but it may come back and bite us.

Also the wait might fail if a signal is hit, so it's definitely possible, and even likely in the case of the X server process.

Anyway, I prefer if we could try to keep the reservation across the ttm_cleanup_memtype_use function, and as far as I can tell, the only thing preventing that is the reservation release in the (!no_wait_gpu) path. So if we alter that to do the same as the evict path I think without looking to deeply into the consequences that we should be safe.

Then we should be able to skip patch 2 as well.

Op 28-11-12 15:23, Thomas Hellstrom schreef:

Quoted 34 lines · Maarten Lankhorst

Quoted 33 lines

Op 28-11-12 14:21, Thomas Hellstrom schreef:

Quoted 32 lines · Maarten Lankhorst

Quoted 25 lines

Op 28-11-12 12:54, Thomas Hellstrom schreef:

Quoted 14 lines · Maarten Lankhorst

Quoted 13 lines

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.

Quoted 5 lines

Quoted 3 lines

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.

Because we would hold the reservation while waiting and with the object still on swap and lru lists still, that would defeat the whole purpose of keeping the object on multiple lists, plus break current code that assumes bo's on the those lists can always be reserved.

Quoted 4 lines

the if (ret && !no_wait_gpu) path has to drop the reservation and lru lock, and isn't guaranteed to be able to retake it. Maybe it could be guaranteed now, but I'm sure that would not be the case if the reservations were shared across devices.

The evict path removes the BO from the LRU lists, drops the LRU lock but hangs on to the reservation, and in case the wait goes wrong, re-adds the bo to the LRU lists and returns an error.

Quoted 7 lines

Quoted 4 lines

If you really want to, we could hang on to the !no_wait_gpu path, wait shouldn't ever fail there, so I suppose leaving it off the lru lists and not re-add on any list in case of wait fail is fine. It's still on the ddestroy list in that case, so not adding it back to the other lists is harmless.

Well I'm a bit afraid that theoretically, other callers may have a bo reserved, while cleanup_refs_and_unlock more or less runs the whole destroy path on that buffer. Sure, we have control over those other reservers, but it may come back and bite us.

That's why initially I moved all the destruction to ttm_bo_release_list, to have all destruction in only 1 place. But even now it's serialized with the lru lock, while the destruction may not happen right away, it still happens before last list ref to the bo is dropped.

But it's your call, just choose the approach you want and I'll resubmit this. :-)

Quoted 5 lines

Also the wait might fail if a signal is hit, so it's definitely possible, and even likely in the case of the X server process.

Anyway, I prefer if we could try to keep the reservation across the ttm_cleanup_memtype_use function, and as far as I can tell, the only thing preventing that is the reservation release in the (!no_wait_gpu) path. So if we alter that to do the same as the evict path I think without looking to deeply into the consequences that we should be safe.

I think returning success early without removing off ddestroy list if re-reserving fails with lru lock held would be better.

We completed the wait and attempt to reserve the bo, which failed. Without the lru lock atomicity, this can't happen since the only places that would do it call this with the lru lock held.

With the atomicity removal, the only place that could do this is ttm_bo_delayed_delete with remove_all set to true. And even if that happened the destruction code would run *anyway* since we completed the waiting part already, it would just not necessarily be run from this thread, but that guarantee didn't exist anyway.

Quoted 1 line

Then we should be able to skip patch 2 as well.

If my tryreserve approach sounds sane, second patch should still be skippable. :-)

~Maarten

Quoted 60 lines

Op 28-11-12 15:23, Thomas Hellstrom schreef:

Quoted 34 lines · Maarten Lankhorst

Quoted 33 lines

Op 28-11-12 14:21, Thomas Hellstrom schreef:

Quoted 32 lines · Maarten Lankhorst

Quoted 25 lines

Op 28-11-12 12:54, Thomas Hellstrom schreef:

Quoted 14 lines · Maarten Lankhorst

Quoted 13 lines

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.

Quoted 5 lines

Quoted 3 lines

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.

Because we would hold the reservation while waiting and with the object still on swap and lru lists still, that would defeat the whole purpose of keeping the object on multiple lists, plus break current code that assumes bo's on the those lists can always be reserved.

Quoted 4 lines

the if (ret && !no_wait_gpu) path has to drop the reservation and lru lock, and isn't guaranteed to be able to retake it. Maybe it could be guaranteed now, but I'm sure that would not be the case if the reservations were shared across devices.

The evict path removes the BO from the LRU lists, drops the LRU lock but hangs on to the reservation, and in case the wait goes wrong, re-adds the bo to the LRU lists and returns an error.

Quoted 7 lines

Quoted 4 lines

If you really want to, we could hang on to the !no_wait_gpu path, wait shouldn't ever fail there, so I suppose leaving it off the lru lists and not re-add on any list in case of wait fail is fine. It's still on the ddestroy list in that case, so not adding it back to the other lists is harmless.

Well I'm a bit afraid that theoretically, other callers may have a bo reserved, while cleanup_refs_and_unlock more or less runs the whole destroy path on that buffer. Sure, we have control over those other reservers, but it may come back and bite us.

That's why initially I moved all the destruction to ttm_bo_release_list, to have all destruction in only 1 place. But even now it's serialized with the lru lock, while the destruction may not happen right away, it still happens before last list ref to the bo is dropped.

But it's your call, just choose the approach you want and I'll resubmit this. :-)

Quoted 5 lines

Also the wait might fail if a signal is hit, so it's definitely possible, and even likely in the case of the X server process.

Anyway, I prefer if we could try to keep the reservation across the ttm_cleanup_memtype_use function, and as far as I can tell, the only thing preventing that is the reservation release in the (!no_wait_gpu) path. So if we alter that to do the same as the evict path I think without looking to deeply into the consequences that we should be safe.

I think returning success early without removing off ddestroy list if re-reserving fails with lru lock held would be better.

We completed the wait and attempt to reserve the bo, which failed. Without the lru lock atomicity, this can't happen since the only places that would do it call this with the lru lock held.

Quoted 6 lines

With the atomicity removal, the only place that could do this is ttm_bo_delayed_delete with remove_all set to true. And even if that happened the destruction code would run *anyway* since we completed the waiting part already, it would just not necessarily be run from this thread, but that guarantee didn't exist anyway.

Quoted 1 line

Then we should be able to skip patch 2 as well.

If my tryreserve approach sounds sane, second patch should still be skippable. :-)

Sure, Lets go for that approach.

Op 28-11-12 16:10, Thomas Hellstrom schreef:

Quoted 61 lines · Maarten Lankhorst

Quoted 60 lines

Op 28-11-12 15:23, Thomas Hellstrom schreef:

Quoted 34 lines · Maarten Lankhorst

Quoted 33 lines

Op 28-11-12 14:21, Thomas Hellstrom schreef:

Quoted 32 lines · Maarten Lankhorst

Quoted 25 lines

Op 28-11-12 12:54, Thomas Hellstrom schreef:

Quoted 14 lines · Maarten Lankhorst

Quoted 13 lines

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.

Quoted 5 lines

Quoted 3 lines

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.

Because we would hold the reservation while waiting and with the object still on swap and lru lists still, that would defeat the whole purpose of keeping the object on multiple lists, plus break current code that assumes bo's on the those lists can always be reserved.

Quoted 4 lines

the if (ret && !no_wait_gpu) path has to drop the reservation and lru lock, and isn't guaranteed to be able to retake it. Maybe it could be guaranteed now, but I'm sure that would not be the case if the reservations were shared across devices.

The evict path removes the BO from the LRU lists, drops the LRU lock but hangs on to the reservation, and in case the wait goes wrong, re-adds the bo to the LRU lists and returns an error.

Quoted 7 lines

Quoted 4 lines

If you really want to, we could hang on to the !no_wait_gpu path, wait shouldn't ever fail there, so I suppose leaving it off the lru lists and not re-add on any list in case of wait fail is fine. It's still on the ddestroy list in that case, so not adding it back to the other lists is harmless.

Well I'm a bit afraid that theoretically, other callers may have a bo reserved, while cleanup_refs_and_unlock more or less runs the whole destroy path on that buffer. Sure, we have control over those other reservers, but it may come back and bite us.

That's why initially I moved all the destruction to ttm_bo_release_list, to have all destruction in only 1 place. But even now it's serialized with the lru lock, while the destruction may not happen right away, it still happens before last list ref to the bo is dropped.

But it's your call, just choose the approach you want and I'll resubmit this. :-)

Quoted 5 lines

Also the wait might fail if a signal is hit, so it's definitely possible, and even likely in the case of the X server process.

Anyway, I prefer if we could try to keep the reservation across the ttm_cleanup_memtype_use function, and as far as I can tell, the only thing preventing that is the reservation release in the (!no_wait_gpu) path. So if we alter that to do the same as the evict path I think without looking to deeply into the consequences that we should be safe.

I think returning success early without removing off ddestroy list if re-reserving fails with lru lock held would be better.

We completed the wait and attempt to reserve the bo, which failed. Without the lru lock atomicity, this can't happen since the only places that would do it call this with the lru lock held.

Quoted 7 lines

Quoted 6 lines

With the atomicity removal, the only place that could do this is ttm_bo_delayed_delete with remove_all set to true. And even if that happened the destruction code would run *anyway* since we completed the waiting part already, it would just not necessarily be run from this thread, but that guarantee didn't exist anyway.

Quoted 1 line

Then we should be able to skip patch 2 as well.

If my tryreserve approach sounds sane, second patch should still be skippable. :-)

Sure, Lets go for that approach.

Ok updated patch below, you only need to check if you like the approach or not, since I haven't tested it yet beyond compiling. Rest of series (minus patch 2) should still apply without modification.

drm/ttm: call ttm_bo_cleanup_refs with reservation and lru lock held, v2

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, 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.

Changes since v1:
- Simplify no_wait_gpu case by folding it in with empty ddestroy.
- Hold a reservation while calling ttm_bo_cleanup_memtype_use again.

Signed-off-by Maarten Lankhorst <[email protected]>

drivers/gpu/drm/ttm/ttm_bo.c +32 -26

index 202fc20..e9f01fe 100644
@@ -488,12 +488,16 @@ static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo)
  	ttm_bo_mem_put(bo, &bo->mem);
 
  	atomic_set(&bo->reserved, 0);
+	wake_up_all(&bo->event_queue);
 
  	/*
-	 * Make processes trying to reserve really pick it up.
+	 * Since the final reference to this bo may not be dropped by
+	 * the current task we have to put a memory barrier here to make
+	 * sure the changes done in this function are always visible.
+	 *
+	 * This function only needs protection against the final kref_put.
  	 */
-	smp_mb__after_atomic_dec();
-	wake_up_all(&bo->event_queue);
+	smp_mb__before_atomic_dec();
  }
 
  static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)

@@ -543,68 +547,95 @@ 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;
+	int ret;
 
-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))
-		return ret;
+	if (ret && !no_wait_gpu) {
+		void *sync_obj;
 
-retry_reserve:
-	spin_lock(&glob->lru_lock);
+		/*
+		 * 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);
 
-	if (unlikely(list_empty(&bo->ddestroy))) {
+		put_count = ttm_bo_del_from_lru(bo);
+
+		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);
+		ttm_bo_list_ref_sub(bo, put_count, true);
 
-	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);

Quoted 174 lines

Op 28-11-12 16:10, Thomas Hellstrom schreef:

Quoted 61 lines · Maarten Lankhorst

Quoted 60 lines

Op 28-11-12 15:23, Thomas Hellstrom schreef:

Quoted 34 lines · Maarten Lankhorst

Quoted 33 lines

Op 28-11-12 14:21, Thomas Hellstrom schreef:

Quoted 32 lines · Maarten Lankhorst

Quoted 25 lines

Op 28-11-12 12:54, Thomas Hellstrom schreef:

Quoted 14 lines · Maarten Lankhorst

Quoted 13 lines

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.

Quoted 5 lines

Quoted 3 lines

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.

Because we would hold the reservation while waiting and with the object still on swap and lru lists still, that would defeat the whole purpose of keeping the object on multiple lists, plus break current code that assumes bo's on the those lists can always be reserved.

Quoted 4 lines

the if (ret && !no_wait_gpu) path has to drop the reservation and lru lock, and isn't guaranteed to be able to retake it. Maybe it could be guaranteed now, but I'm sure that would not be the case if the reservations were shared across devices.

The evict path removes the BO from the LRU lists, drops the LRU lock but hangs on to the reservation, and in case the wait goes wrong, re-adds the bo to the LRU lists and returns an error.

Quoted 7 lines

Quoted 4 lines

If you really want to, we could hang on to the !no_wait_gpu path, wait shouldn't ever fail there, so I suppose leaving it off the lru lists and not re-add on any list in case of wait fail is fine. It's still on the ddestroy list in that case, so not adding it back to the other lists is harmless.

Well I'm a bit afraid that theoretically, other callers may have a bo reserved, while cleanup_refs_and_unlock more or less runs the whole destroy path on that buffer. Sure, we have control over those other reservers, but it may come back and bite us.

That's why initially I moved all the destruction to ttm_bo_release_list, to have all destruction in only 1 place. But even now it's serialized with the lru lock, while the destruction may not happen right away, it still happens before last list ref to the bo is dropped.

But it's your call, just choose the approach you want and I'll resubmit this. :-)

Quoted 5 lines

Also the wait might fail if a signal is hit, so it's definitely possible, and even likely in the case of the X server process.

Anyway, I prefer if we could try to keep the reservation across the ttm_cleanup_memtype_use function, and as far as I can tell, the only thing preventing that is the reservation release in the (!no_wait_gpu) path. So if we alter that to do the same as the evict path I think without looking to deeply into the consequences that we should be safe.

I think returning success early without removing off ddestroy list if re-reserving fails with lru lock held would be better.

We completed the wait and attempt to reserve the bo, which failed. Without the lru lock atomicity, this can't happen since the only places that would do it call this with the lru lock held.

Quoted 7 lines

Quoted 6 lines

With the atomicity removal, the only place that could do this is ttm_bo_delayed_delete with remove_all set to true. And even if that happened the destruction code would run *anyway* since we completed the waiting part already, it would just not necessarily be run from this thread, but that guarantee didn't exist anyway.

Quoted 1 line

Then we should be able to skip patch 2 as well.

If my tryreserve approach sounds sane, second patch should still be skippable. :-)

Sure, Lets go for that approach.

Ok updated patch below, you only need to check if you like the approach or not, since I haven't tested it yet beyond compiling. Rest of series (minus patch 2) should still apply without modification.

drm/ttm: call ttm_bo_cleanup_refs with reservation and lru lock held, v2
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, 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.
Changes since v1:
- Simplify no_wait_gpu case by folding it in with empty ddestroy.
- Hold a reservation while calling ttm_bo_cleanup_memtype_use again.

Signed-off-by Maarten Lankhorst <[email protected]>

drivers/gpu/drm/ttm/ttm_bo.c +32 -19

index 202fc20..e9f01fe 100644
@@ -488,12 +488,16 @@ static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo)
      ttm_bo_mem_put(bo, &bo->mem);
        atomic_set(&bo->reserved, 0);
+    wake_up_all(&bo->event_queue);
        /*
-     * Make processes trying to reserve really pick it up.
+     * Since the final reference to this bo may not be dropped by
+     * the current task we have to put a memory barrier here to make
+     * sure the changes done in this function are always visible.
+     *
+     * This function only needs protection against the final kref_put.
       */
-    smp_mb__after_atomic_dec();
-    wake_up_all(&bo->event_queue);
+    smp_mb__before_atomic_dec();
  }
    static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)

@@ -543,68 +547,95 @@ 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;
+    int ret;
  -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))
-        return ret;
+    if (ret && !no_wait_gpu) {
+        void *sync_obj;
  -retry_reserve:
-    spin_lock(&glob->lru_lock);
+        /*
+         * 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);
  -    if (unlikely(list_empty(&bo->ddestroy))) {
+        put_count = ttm_bo_del_from_lru(bo);
+
+        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);
+        ttm_bo_list_ref_sub(bo, put_count, true);
  -    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);

drivers/gpu/drm/ttm/ttm_bo.c static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo) · line 602

Quoted 10 lines

drivers/gpu/drm/ttm/ttm_bo.c +9 -0

+        driver->sync_obj_unref(&sync_obj);
+        if (ret) {
+            /*
+             * Either the wait returned -ERESTARTSYS, or -EDEADLK
+             * (radeon lockup) here. No effort is made to re-add
+             * this bo to any lru list. Instead the bo only
+             * appears on the delayed destroy list.
+             */
              return ret;
+        }

Actually, we *must* re-add the bo to LRU lists here, because otherwise when a driver needs to evict a memory type completely, there's a large chance it will miss this bo.

So I think either we need to keep the reservation, or keep the bo on the LRU lists.

Op 28-11-12 20:21, Thomas Hellstrom schreef:

Quoted 188 lines · Maarten Lankhorst

Quoted 174 lines

Op 28-11-12 16:10, Thomas Hellstrom schreef:

Quoted 61 lines · Maarten Lankhorst

Quoted 60 lines

Op 28-11-12 15:23, Thomas Hellstrom schreef:

Quoted 34 lines · Maarten Lankhorst

Quoted 33 lines

Op 28-11-12 14:21, Thomas Hellstrom schreef:

Quoted 32 lines · Maarten Lankhorst

Quoted 25 lines

Op 28-11-12 12:54, Thomas Hellstrom schreef:

Quoted 14 lines · Maarten Lankhorst

Quoted 13 lines

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.

Quoted 5 lines

Quoted 3 lines

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.

Because we would hold the reservation while waiting and with the object still on swap and lru lists still, that would defeat the whole purpose of keeping the object on multiple lists, plus break current code that assumes bo's on the those lists can always be reserved.

Quoted 4 lines

the if (ret && !no_wait_gpu) path has to drop the reservation and lru lock, and isn't guaranteed to be able to retake it. Maybe it could be guaranteed now, but I'm sure that would not be the case if the reservations were shared across devices.

The evict path removes the BO from the LRU lists, drops the LRU lock but hangs on to the reservation, and in case the wait goes wrong, re-adds the bo to the LRU lists and returns an error.

Quoted 7 lines

Quoted 4 lines

If you really want to, we could hang on to the !no_wait_gpu path, wait shouldn't ever fail there, so I suppose leaving it off the lru lists and not re-add on any list in case of wait fail is fine. It's still on the ddestroy list in that case, so not adding it back to the other lists is harmless.

Well I'm a bit afraid that theoretically, other callers may have a bo reserved, while cleanup_refs_and_unlock more or less runs the whole destroy path on that buffer. Sure, we have control over those other reservers, but it may come back and bite us.

That's why initially I moved all the destruction to ttm_bo_release_list, to have all destruction in only 1 place. But even now it's serialized with the lru lock, while the destruction may not happen right away, it still happens before last list ref to the bo is dropped.

But it's your call, just choose the approach you want and I'll resubmit this. :-)

Quoted 5 lines

Also the wait might fail if a signal is hit, so it's definitely possible, and even likely in the case of the X server process.

Anyway, I prefer if we could try to keep the reservation across the ttm_cleanup_memtype_use function, and as far as I can tell, the only thing preventing that is the reservation release in the (!no_wait_gpu) path. So if we alter that to do the same as the evict path I think without looking to deeply into the consequences that we should be safe.

I think returning success early without removing off ddestroy list if re-reserving fails with lru lock held would be better.

We completed the wait and attempt to reserve the bo, which failed. Without the lru lock atomicity, this can't happen since the only places that would do it call this with the lru lock held.

Quoted 7 lines

Quoted 6 lines

With the atomicity removal, the only place that could do this is ttm_bo_delayed_delete with remove_all set to true. And even if that happened the destruction code would run *anyway* since we completed the waiting part already, it would just not necessarily be run from this thread, but that guarantee didn't exist anyway.

Quoted 1 line

Then we should be able to skip patch 2 as well.

If my tryreserve approach sounds sane, second patch should still be skippable. :-)

Sure, Lets go for that approach.

Ok updated patch below, you only need to check if you like the approach or not, since I haven't tested it yet beyond compiling. Rest of series (minus patch 2) should still apply without modification.

drm/ttm: call ttm_bo_cleanup_refs with reservation and lru lock held, v2
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, 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.
Changes since v1:
- Simplify no_wait_gpu case by folding it in with empty ddestroy.
- Hold a reservation while calling ttm_bo_cleanup_memtype_use again.

Signed-off-by Maarten Lankhorst <[email protected]>

drivers/gpu/drm/ttm/ttm_bo.c +32 -19

index 202fc20..e9f01fe 100644
@@ -488,12 +488,16 @@ static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo)
       ttm_bo_mem_put(bo, &bo->mem);
         atomic_set(&bo->reserved, 0);
+    wake_up_all(&bo->event_queue);
         /*
-     * Make processes trying to reserve really pick it up.
+     * Since the final reference to this bo may not be dropped by
+     * the current task we have to put a memory barrier here to make
+     * sure the changes done in this function are always visible.
+     *
+     * This function only needs protection against the final kref_put.
        */
-    smp_mb__after_atomic_dec();
-    wake_up_all(&bo->event_queue);
+    smp_mb__before_atomic_dec();
   }
     static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)

@@ -543,68 +547,95 @@ 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;
+    int ret;
   -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))
-        return ret;
+    if (ret && !no_wait_gpu) {
+        void *sync_obj;
   -retry_reserve:
-    spin_lock(&glob->lru_lock);
+        /*
+         * 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);
   -    if (unlikely(list_empty(&bo->ddestroy))) {
+        put_count = ttm_bo_del_from_lru(bo);
+
+        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);
+        ttm_bo_list_ref_sub(bo, put_count, true);
   -    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);

drivers/gpu/drm/ttm/ttm_bo.c static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo) · line 602

Quoted 10 lines

drivers/gpu/drm/ttm/ttm_bo.c +9 -0

+        driver->sync_obj_unref(&sync_obj);
+        if (ret) {
+            /*
+             * Either the wait returned -ERESTARTSYS, or -EDEADLK
+             * (radeon lockup) here. No effort is made to re-add
+             * this bo to any lru list. Instead the bo only
+             * appears on the delayed destroy list.
+             */
               return ret;
+        }

Actually, we *must* re-add the bo to LRU lists here, because otherwise when a driver needs to evict a memory type completely, there's a large chance it will miss this bo.

Quoted 1 line

So I think either we need to keep the reservation, or keep the bo on the LRU lists.

The second option is what v1 did, except I never bothered to re-take the reservation. ;-)

It shouldn't cause troubles to leave it on the lru lists if we drop the the reservation, we can keep handling re-reservation failure in the same way as in v2.

In that case would v3 be the same as v2 of this patch, except with those 2 lines from the ret && !no_wait_gpu branch removed:

put_count = ttm_bo_del_from_lru(bo);
ttm_bo_list_ref_sub(bo, put_count, true);

And of course the comment after sync_obj_wait failure would no longer apply.

~Maarten

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, 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.

Changes since v1:
- Simplify no_wait_gpu case by folding it in with empty ddestroy.
- Hold a reservation while calling ttm_bo_cleanup_memtype_use again.
Changes since v2:
- Do not remove bo from lru list while waiting

Signed-off-by Maarten Lankhorst <[email protected]>

Quoted 25 lines

Signed-off-by Maarten Lankhorst <[email protected]>

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

drivers/gpu/drm/ttm/ttm_bo.c +0 -8

index 202fc20..02b275b 100644
@@ -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)

drivers/gpu/drm/ttm/ttm_bo.c static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo) · line 512

Quoted 5 lines

drivers/gpu/drm/ttm/ttm_bo.c +3 -0

          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.

Replace the while loop with a simple for each loop, and only run the delayed destroy cleanup if we can reserve the buffer first.

No race occurs, since lru lock is never dropped any more. An empty list and a list full of unreservable buffers both cause -EBUSY to be returned, which is identical to the previous situation.

Signed-off-by Maarten Lankhorst <[email protected]>

---
  drivers/gpu/drm/ttm/ttm_bo.c | 44 ++++++++++++++------------------------------
  1 file changed, 14 insertions(+), 30 deletions(-)

This requires changing the order in ttm_bo_cleanup_refs_or_queue to take the reservation first, as there is otherwise no race free way to take lru lock before fence_lock.

Signed-off-by Maarten Lankhorst <[email protected]>

---
  drivers/gpu/drm/ttm/ttm_bo.c           | 31 +++++++++++--------------------
  drivers/gpu/drm/ttm/ttm_execbuf_util.c |  4 ++--
  2 files changed, 13 insertions(+), 22 deletions(-)

drivers/gpu/drm/ttm/ttm_bo.c +11 -20

index 7426fe5..202fc20 100644
@@ -500,27 +500,17 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
  {
  	struct ttm_bo_device *bdev = bo->bdev;
  	struct ttm_bo_global *glob = bo->glob;
-	struct ttm_bo_driver *driver;
+	struct ttm_bo_driver *driver = bdev->driver;
  	void *sync_obj = NULL;
  	int put_count;
  	int ret;
 
+	spin_lock(&glob->lru_lock);
+	ret = ttm_bo_reserve_locked(bo, false, true, false, 0);
+
  	spin_lock(&bdev->fence_lock);
  	(void) ttm_bo_wait(bo, false, false, true);
-	if (!bo->sync_obj) {
-
-		spin_lock(&glob->lru_lock);
-
-		/**
-		 * Lock inversion between bo:reserve and bdev::fence_lock here,
-		 * but that's OK, since we're only trylocking.
-		 */
-
-		ret = ttm_bo_reserve_locked(bo, false, true, false, 0);
-
-		if (unlikely(ret == -EBUSY))
-			goto queue;
-
+	if (!ret && !bo->sync_obj) {
  		spin_unlock(&bdev->fence_lock);
  		put_count = ttm_bo_del_from_lru(bo);
 

@@ -530,18 +520,19 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
  		ttm_bo_list_ref_sub(bo, put_count, true);
 
  		return;
-	} else {
-		spin_lock(&glob->lru_lock);
  	}
-queue:
-	driver = bdev->driver;
  	if (bo->sync_obj)
  		sync_obj = driver->sync_obj_ref(bo->sync_obj);
+	spin_unlock(&bdev->fence_lock);
+
+	if (!ret) {
+		atomic_set(&bo->reserved, 0);
+		wake_up_all(&bo->event_queue);
+	}
 
  	kref_get(&bo->list_kref);
  	list_add_tail(&bo->ddestroy, &bdev->ddestroy);
  	spin_unlock(&glob->lru_lock);
-	spin_unlock(&bdev->fence_lock);
 
  	if (sync_obj) {
  		driver->sync_obj_flush(sync_obj);

drivers/gpu/drm/ttm/ttm_execbuf_util.c +2 -2

index 1986d00..cd9e452 100644
@@ -213,8 +213,8 @@ void ttm_eu_fence_buffer_objects(struct list_head *list, void *sync_obj)
  	driver = bdev->driver;
  	glob = bo->glob;
 
-	spin_lock(&bdev->fence_lock);
  	spin_lock(&glob->lru_lock);
+	spin_lock(&bdev->fence_lock);
 
  	list_for_each_entry(entry, list, head) {
  		bo = entry->bo;

@@ -223,8 +223,8 @@ void ttm_eu_fence_buffer_objects(struct list_head *list, void *sync_obj)
  		ttm_bo_unreserve_locked(bo);
  		entry->reserved = false;
  	}
-	spin_unlock(&glob->lru_lock);
  	spin_unlock(&bdev->fence_lock);
+	spin_unlock(&glob->lru_lock);
 
  	list_for_each_entry(entry, list, head) {
  		if (entry->old_sync_obj)