Re: [bug report] drm/ttm, drm_xe, Implement ttm_lru_walk_for_evict() using the guarded LRU iteration

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

Hi!

On Tue, 2025-07-01 at 12:49 -0500, Dan Carpenter wrote:
> Hello Thomas Hellström,
> 
> Commit bb8aa27eff6f ("drm/ttm, drm_xe, Implement
> ttm_lru_walk_for_evict() using the guarded LRU iteration") from Jun
> 23, 2025 (linux-next), leads to the following Smatch static checker
> warning:
> 
> 	drivers/gpu/drm/ttm/ttm_bo_util.c:899
> ttm_lru_walk_for_evict()
> 	warn: 'bo' isn't an ERR_PTR
> 
> drivers/gpu/drm/ttm/ttm_bo_util.c
>    883  s64 ttm_lru_walk_for_evict(struct ttm_lru_walk *walk, struct
> ttm_device *bdev,
>    884                             struct ttm_resource_manager *man,
> s64 target)
>    885  {
>    886          struct ttm_bo_lru_cursor cursor;
>    887          struct ttm_buffer_object *bo;
>    888          s64 progress = 0;
>    889          s64 lret;
>    890  
>    891          ttm_bo_lru_for_each_reserved_guarded(&cursor, man,
> &walk->arg, bo) {
>    892                  lret = walk->ops->process_bo(walk, bo);
>    893                  if (lret == -EBUSY || lret == -EALREADY)
>    894                          lret = 0;
>    895                  progress = (lret < 0) ? lret : progress +
> lret;
>    896                  if (progress < 0 || progress >= target)
>    897                          break;
>    898          }
>    899          if (IS_ERR(bo))
>    900                  return PTR_ERR(bo);
> 
> The ttm_bo_lru_for_each_reserved_guarded() macro checks for
> IS_ERR_OR_NULL()
> but it can only be NULL.

That's not correct.

>   These things are a bit frustrating to me because
> when a function returns both error pointers and NULL then ideally
> there is a
> reason for that and it should be documented.  "This function returns
> error
> pointers if there is an error (kmalloc failed etc) or NULL if the
> object is
> not found".

The error pointer is documented under the
ttm_bo_lru_for_each_reserved_guarded() macro. But it is true that I've
forgotten to update the doc for ttm_bo_lru_cursor_first() and
ttm_bo_lru_cursor_next() to reflect that they may return an error
pointer or NULL. I will put together a patch for that.

> 
> https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/
> 
>    901  
>    902          return progress;
> 
> It's strange to me that we returns progress values which are greater
> than the
> target value.

This is also documented in the ttm_lru_walk_for_evict() kerneldoc. In
short a typical intended use-case is that we're requested to evict
@target pages, but since we evict a buffer object at a time (multiple
pages) the total may exceed @progress.

In any case it looks like the ttm_lru_walk_for_evict() function may go
away with upcoming patches from Christian.

Thanks,
Thomas
 

> 
>    903  }
> 
> regards,
> dan carpenter