drm/ttm: do not check if list is empty in ttm_bo_force_list_clean, v2

PATCH In review Maarten Lankhorst 2 participants 4 messages Nov 30, 2012

Subsystems (3)

DRM drivers DRM misc DRM TTM

lore

Share & tools

lore

Maarten Lankhorst

author

2012-11-30 08:05 UTC #0 lore raw

PATCH 1/1 drm/ttm: do not check if list is empty in ttm_bo_force_list_clean, v2

Just use the return error from ttm_mem_evict_first instead.

Changes since v1:
- Add warning if list is not empty, nothing else we can do here.

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

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

drivers/gpu/drm/ttm/ttm_bo.c +15 ~~-15~~

index 8ab23ae..9028327 100644
@@ -1323,25 +1323,25 @@ static int ttm_bo_force_list_clean(struct ttm_bo_device *bdev,
 	struct ttm_bo_global *glob = bdev->glob;
 	int ret;
 
-	/*
-	 * Can't use standard list traversal since we're unlocking.
-	 */
-
-	spin_lock(&glob->lru_lock);
-	while (!list_empty(&man->lru)) {
-		spin_unlock(&glob->lru_lock);
+	do {
 		ret = ttm_mem_evict_first(bdev, mem_type, false, false);
-		if (ret) {
-			if (allow_errors) {
-				return ret;
-			} else {
-				pr_err("Cleanup eviction failed\n");
-			}
+		if (ret && ret != -EBUSY && !allow_errors) {
+			pr_err("Cleanup eviction failed with %i\n", ret);
+			ret = 0;
 		}
+	} while (!ret);
+
+	if (likely(ret == -EBUSY)) {
+		/*
+		* lru list should be empty, verify this is the case.
+		*/
 		spin_lock(&glob->lru_lock);
+		WARN_ON(!list_empty(&man->lru));
+		if (list_empty(&man->lru) || !allow_errors)
+			ret = 0;
+		spin_unlock(&glob->lru_lock);
 	}
-	spin_unlock(&glob->lru_lock);
-	return 0;
+	return ret;
 }
 
 int ttm_bo_clean_mm(struct ttm_bo_device *bdev, unsigned mem_type)

Thomas Hellstrom

2012-11-30 09:42 UTC #1 lore raw

Quoted 4 lines · Maarten Lankhorst

Just use the return error from ttm_mem_evict_first instead.
Changes since v1:
- Add warning if list is not empty, nothing else we can do here.

Marten, when this function is called, all cross-device reservers have been shut out with the TTM lock or whatever similar mechanism is in place. It's a critical function that must succeed, unless there is a hardware failure to evict.

As mentioned in the comments on the previous patch, ttm_bo_evict_first() may return 0 (OK) if it failed to reclaim the trylock in cleanup_refs_and_unlock(), with the assumption that another process will destroy the bo anyway (possibly at a later time which we know nothing about). The lru list needs to be empty when this function returns.

This means we must either keep the while(list_empty) or perhaps better, retry instead of WARN if list_empty() is detected at the end of list.

/Thomas

Quoted 50 lines

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

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

drivers/gpu/drm/ttm/ttm_bo.c +15 ~~-15~~

index 8ab23ae..9028327 100644
@@ -1323,25 +1323,25 @@ static int ttm_bo_force_list_clean(struct ttm_bo_device *bdev,
  	struct ttm_bo_global *glob = bdev->glob;
  	int ret;
 
-	/*
-	 * Can't use standard list traversal since we're unlocking.
-	 */
-
-	spin_lock(&glob->lru_lock);
-	while (!list_empty(&man->lru)) {
-		spin_unlock(&glob->lru_lock);
+	do {
  		ret = ttm_mem_evict_first(bdev, mem_type, false, false);
-		if (ret) {
-			if (allow_errors) {
-				return ret;
-			} else {
-				pr_err("Cleanup eviction failed\n");
-			}
+		if (ret && ret != -EBUSY && !allow_errors) {
+			pr_err("Cleanup eviction failed with %i\n", ret);
+			ret = 0;
  		}
+	} while (!ret);
+
+	if (likely(ret == -EBUSY)) {
+		/*
+		* lru list should be empty, verify this is the case.
+		*/
  		spin_lock(&glob->lru_lock);
+		WARN_ON(!list_empty(&man->lru));
+		if (list_empty(&man->lru) || !allow_errors)
+			ret = 0;
+		spin_unlock(&glob->lru_lock);
  	}
-	spin_unlock(&glob->lru_lock);
-	return 0;
+	return ret;
  }
 
  int ttm_bo_clean_mm(struct ttm_bo_device *bdev, unsigned mem_type)

Maarten Lankhorst

author

2012-11-30 10:16 UTC #2 lore raw

Op 30-11-12 10:04, Thomas Hellstrom schreef:

Quoted 6 lines · Maarten Lankhorst

Quoted 4 lines
Just use the return error from ttm_mem_evict_first instead.
Changes since v1:
- Add warning if list is not empty, nothing else we can do here.

Quoted 8 lines

Marten, when this function is called, all cross-device reservers have been shut out with the TTM lock or whatever similar mechanism is in place. It's a critical function that must succeed, unless there is a hardware failure to evict.
As mentioned in the comments on the previous patch, ttm_bo_evict_first() may return 0 (OK) if it failed to reclaim the trylock in cleanup_refs_and_unlock(), with the assumption that another process will destroy the bo anyway (possibly at a later time which we know nothing about). The lru list needs to be empty when this function returns.
This means we must either keep the while(list_empty) or perhaps better, retry instead of WARN if list_empty() is detected at the end of list.

As long as evict_first returns 0, that function is called over and over again.

But regarding the race.. Even if in this case it returns 0 early, that bo would no longer be on the lru list anyway, since I always take the reservation with lru lock held in the cleanup code, so either this is a cross-device reservation (needs more thought on how to handle that correctly), or that other function is inside the cleanup_memtype_use function, and has already taken the bo off all lists before dropping lru lock.

Now it may seem that blocking on reservation can help in this case, maybe it would, but it doesn't close the race entirely..If another function also called ttm_bo_cleanup_refs_and_unlock from another context BEFORE ttm_mem_evict_first was called, at the time ttm_mem_evict_first gets the lru lock the buffer was already taken off the lru lists, and evict_first wouldn't know anything about it and return -EBUSY too.

But the lru list is still empty in any of those cases, so the WARN wouldn't trigger..

~Maarten

Thomas Hellstrom

Reviewed-by

2012-11-30 11:55 UTC #3 lore raw

Quoted 9 lines · Maarten Lankhorst

Op 30-11-12 10:04, Thomas Hellstrom schreef:
Quoted 8 lines · Maarten Lankhorst
Quoted 4 lines
Just use the return error from ttm_mem_evict_first instead.
Changes since v1:
- Add warning if list is not empty, nothing else we can do here.
Marten, when this function is called, all cross-device reservers have been shut out with the TTM lock or whatever similar mechanism is in place. It's a critical function that must succeed, unless there is a hardware failure to evict.

Quoted 6 lines

Quoted 5 lines
As mentioned in the comments on the previous patch, ttm_bo_evict_first() may return 0 (OK) if it failed to reclaim the trylock in cleanup_refs_and_unlock(), with the assumption that another process will destroy the bo anyway (possibly at a later time which we know nothing about). The lru list needs to be empty when this function returns.
This means we must either keep the while(list_empty) or perhaps better, retry instead of WARN if list_empty() is detected at the end of list.
As long as evict_first returns 0, that function is called over and over again

Ah, ok. You're right.

Reviewed-by Thomas Hellstrom<[email protected]>