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

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

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

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c |   30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 8ab23ae..9028327 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -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)

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

[Thomas Hellstrom]

On 11/30/2012 09:05 AM, Maarten Lankhorst wrote:
> 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

> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
> ---
>   drivers/gpu/drm/ttm/ttm_bo.c |   30 +++++++++++++++---------------
>   1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 8ab23ae..9028327 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -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)

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

[Maarten Lankhorst]

Op 30-11-12 10:04, Thomas Hellstrom schreef:
> On 11/30/2012 09:05 AM, Maarten Lankhorst wrote:
>> 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.
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

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

[Thomas Hellstrom]

On 11/30/2012 11:16 AM, Maarten Lankhorst wrote:
> Op 30-11-12 10:04, Thomas Hellstrom schreef:
>> On 11/30/2012 09:05 AM, Maarten Lankhorst wrote:
>>> 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.
> As long as evict_first returns 0, that function is called over and over again

Ah, ok. You're right.

Reviewed-by: Thomas Hellstrom<thellstrom@vmware.com>