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>]

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