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

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

[Dan Carpenter]

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 (unpublished) Smatch
static checker warning:

	drivers/gpu/drm/ttm/ttm_bo_util.c:991 __ttm_bo_lru_cursor_next()
	warn: duplicate check 'res' (previous on line 952)

drivers/gpu/drm/ttm/ttm_bo_util.c
   931  __ttm_bo_lru_cursor_next(struct ttm_bo_lru_cursor *curs)
   932  {
   933          spinlock_t *lru_lock = &curs->res_curs.man->bdev->lru_lock;
   934          struct ttm_resource *res = NULL;
   935          struct ttm_buffer_object *bo;
   936          struct ttm_lru_walk_arg *arg = curs->arg;
   937          bool first = !curs->bo;
   938  
   939          ttm_bo_lru_cursor_cleanup_bo(curs);
   940  
   941          spin_lock(lru_lock);
   942          for (;;) {
   943                  int mem_type, ret = 0;
   944                  bool bo_locked = false;
   945  
   946                  if (first) {
   947                          res = ttm_resource_manager_first(&curs->res_curs);
   948                          first = false;
   949                  } else {
   950                          res = ttm_resource_manager_next(&curs->res_curs);
   951                  }
   952                  if (!res)
   953                          break;

This is the only break statement

   954  
   955                  bo = res->bo;
   956                  if (ttm_lru_walk_trylock(curs, bo))
   957                          bo_locked = true;
   958                  else if (!arg->ticket || arg->ctx->no_wait_gpu || arg->trylock_only)
   959                          continue;
   960  
   961                  if (!ttm_bo_get_unless_zero(bo)) {
   962                          if (curs->needs_unlock)
   963                                  dma_resv_unlock(bo->base.resv);
   964                          continue;
   965                  }
   966  
   967                  mem_type = res->mem_type;
   968                  spin_unlock(lru_lock);
   969                  if (!bo_locked)
   970                          ret = ttm_lru_walk_ticketlock(curs, bo);
   971  
   972                  /*
   973                   * Note that in between the release of the lru lock and the
   974                   * ticketlock, the bo may have switched resource,
   975                   * and also memory type, since the resource may have been
   976                   * freed and allocated again with a different memory type.
   977                   * In that case, just skip it.
   978                   */
   979                  curs->bo = bo;
   980                  if (!ret && bo->resource && bo->resource->mem_type == mem_type)
   981                          return bo;
   982  
   983                  ttm_bo_lru_cursor_cleanup_bo(curs);
   984                  if (ret && ret != -EALREADY)
   985                          return ERR_PTR(ret);
   986  
   987                  spin_lock(lru_lock);
   988          }
   989  
   990          spin_unlock(lru_lock);
   991          return res ? bo : NULL;

So we know res is NULL and we could just change this to "return NULL;"

   992  }

regards,
dan carpenter

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

[Dan Carpenter]

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

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.

   903  }

regards,
dan carpenter

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

[Thomas Hellström]

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

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

[Thomas Hellström]

On Sat, 2025-06-28 at 22:51 -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 (unpublished) Smatch
> static checker warning:
> 
> 	drivers/gpu/drm/ttm/ttm_bo_util.c:991
> __ttm_bo_lru_cursor_next()
> 	warn: duplicate check 'res' (previous on line 952)
> 
> drivers/gpu/drm/ttm/ttm_bo_util.c
>    931  __ttm_bo_lru_cursor_next(struct ttm_bo_lru_cursor *curs)
>    932  {
>    933          spinlock_t *lru_lock = &curs->res_curs.man->bdev-
> >lru_lock;
>    934          struct ttm_resource *res = NULL;
>    935          struct ttm_buffer_object *bo;
>    936          struct ttm_lru_walk_arg *arg = curs->arg;
>    937          bool first = !curs->bo;
>    938  
>    939          ttm_bo_lru_cursor_cleanup_bo(curs);
>    940  
>    941          spin_lock(lru_lock);
>    942          for (;;) {
>    943                  int mem_type, ret = 0;
>    944                  bool bo_locked = false;
>    945  
>    946                  if (first) {
>    947                          res =
> ttm_resource_manager_first(&curs->res_curs);
>    948                          first = false;
>    949                  } else {
>    950                          res =
> ttm_resource_manager_next(&curs->res_curs);
>    951                  }
>    952                  if (!res)
>    953                          break;
> 
> This is the only break statement
> 
>    954  
>    955                  bo = res->bo;
>    956                  if (ttm_lru_walk_trylock(curs, bo))
>    957                          bo_locked = true;
>    958                  else if (!arg->ticket || arg->ctx-
> >no_wait_gpu || arg->trylock_only)
>    959                          continue;
>    960  
>    961                  if (!ttm_bo_get_unless_zero(bo)) {
>    962                          if (curs->needs_unlock)
>    963                                  dma_resv_unlock(bo-
> >base.resv);
>    964                          continue;
>    965                  }
>    966  
>    967                  mem_type = res->mem_type;
>    968                  spin_unlock(lru_lock);
>    969                  if (!bo_locked)
>    970                          ret = ttm_lru_walk_ticketlock(curs,
> bo);
>    971  
>    972                  /*
>    973                   * Note that in between the release of the
> lru lock and the
>    974                   * ticketlock, the bo may have switched
> resource,
>    975                   * and also memory type, since the resource
> may have been
>    976                   * freed and allocated again with a different
> memory type.
>    977                   * In that case, just skip it.
>    978                   */
>    979                  curs->bo = bo;
>    980                  if (!ret && bo->resource && bo->resource-
> >mem_type == mem_type)
>    981                          return bo;
>    982  
>    983                  ttm_bo_lru_cursor_cleanup_bo(curs);
>    984                  if (ret && ret != -EALREADY)
>    985                          return ERR_PTR(ret);
>    986  
>    987                  spin_lock(lru_lock);
>    988          }
>    989  
>    990          spin_unlock(lru_lock);
>    991          return res ? bo : NULL;
> 
> So we know res is NULL and we could just change this to "return
> NULL;"

Right.
Thanks for the report. Will put together a patch.

Thanks,
Thomas


> 
>    992  }
> 
> regards,
> dan carpenter