drm/ttm: Fix a theoretical race in ttm_bo_cleanup_refs()

In theory, that function could release the lru lock between checking for bo on ddestroy list and a successful reserve if the bo was already reserved, and the function was called with waiting reserves allowed. However, all current reservers of a bo on the ddestroy list would atomically take the bo off the list after a successful reserve so this race should not have been hit, so no need to backport for stable. This patch also fixes a case found by Maarten Lankhorst where ttm_mem_evict_first called with no_wait_gpu would incorrectly spin waiting for bo idle if trying to evict a busy buffer that also sits on the ddestroy list. Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com> Signed-off-by: Dave Airlie <airlied@redhat.com>

drm/ttm: Fix a theoretical race in ttm_bo_cleanup_refs()
In theory, that function could release the lru lock between checking for bo on ddestroy list and a successful reserve if the bo was already reserved, and the function was called with waiting reserves allowed. However, all current reservers of a bo on the ddestroy list would atomically take the bo off the list after a successful reserve so this race should not have been hit, so no need to backport for stable. This patch also fixes a case found by Maarten Lankhorst where ttm_mem_evict_first called with no_wait_gpu would incorrectly spin waiting for bo idle if trying to evict a busy buffer that also sits on the ddestroy list. Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com> Signed-off-by: Dave Airlie <airlied@redhat.com>
Thomas Hellstrom · Dave Airlie
1 parent 7bc17a7837
Showing 1 changed file with 12 additions and 8 deletions Side-by-side Diff
drivers/gpu/drm/ttm/ttm_bo.c
@@ -580,6 +580,7 @@
 	if (unlikely(ret != 0))
 		return ret;
  
+retry_reserve:
 	spin_lock(&glob->lru_lock);
  
 	if (unlikely(list_empty(&bo->ddestroy))) {
  
  
  
@@ -587,14 +588,20 @@
 		return 0;
 	}
  
-	ret = ttm_bo_reserve_locked(bo, interruptible,
-				    no_wait_reserve, false, 0);
+	ret = ttm_bo_reserve_locked(bo, false, true, false, 0);
  
-	if (unlikely(ret != 0)) {
+	if (unlikely(ret == -EBUSY)) {
 		spin_unlock(&glob->lru_lock);
-		return ret;
+		if (likely(!no_wait_reserve))
+			ret = ttm_bo_wait_unreserved(bo, interruptible);
+		if (unlikely(ret != 0))
+			return ret;
+
+		goto retry_reserve;
 	}
  
+	BUG_ON(ret != 0);
+
 	/**
 	 * We can re-check for sync object without taking
 	 * the bo::lock since setting the sync object requires
@@ -811,10 +818,7 @@
 					  no_wait_reserve, no_wait_gpu);
 		kref_put(&bo->list_kref, ttm_bo_release_list);
  
-		if (likely(ret == 0 || ret == -ERESTARTSYS))
-			return ret;
-
-		goto retry;
+		return ret;
 	}
  
 	ret = ttm_bo_reserve_locked(bo, false, true, false, 0);
...	...	@@ -580,6 +580,7 @@
580	580	if (unlikely(ret != 0))
581	581	return ret;
582	582
	583	+retry_reserve:
583	584	spin_lock(&glob->lru_lock);
584	585
585	586	if (unlikely(list_empty(&bo->ddestroy))) {
586	587
587	588
588	589
...	...	@@ -587,14 +588,20 @@
587	588	return 0;
588	589	}
589	590
590		- ret = ttm_bo_reserve_locked(bo, interruptible,
591		- no_wait_reserve, false, 0);
	591	+ ret = ttm_bo_reserve_locked(bo, false, true, false, 0);
592	592
593		- if (unlikely(ret != 0)) {
	593	+ if (unlikely(ret == -EBUSY)) {
594	594	spin_unlock(&glob->lru_lock);
595		- return ret;
	595	+ if (likely(!no_wait_reserve))
	596	+ ret = ttm_bo_wait_unreserved(bo, interruptible);
	597	+ if (unlikely(ret != 0))
	598	+ return ret;
	599	+
	600	+ goto retry_reserve;
596	601	}
597	602
	603	+ BUG_ON(ret != 0);
	604	+
598	605	/**
599	606	* We can re-check for sync object without taking
600	607	* the bo::lock since setting the sync object requires
...	...	@@ -811,10 +818,7 @@
811	818	no_wait_reserve, no_wait_gpu);
812	819	kref_put(&bo->list_kref, ttm_bo_release_list);
813	820
814		- if (likely(ret == 0 \|\| ret == -ERESTARTSYS))
815		- return ret;
816		-
817		- goto retry;
	821	+ return ret;
818	822	}
819	823
820	824	ret = ttm_bo_reserve_locked(bo, false, true, false, 0);