* [bug report] drm/ttm, drm_xe, Implement ttm_lru_walk_for_evict() using the guarded LRU iteration
@ 2025-06-29 3:51 Dan Carpenter
2025-07-21 14:35 ` Thomas Hellström
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2025-06-29 3:51 UTC (permalink / raw)
To: Thomas Hellström; +Cc: dri-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [bug report] drm/ttm, drm_xe, Implement ttm_lru_walk_for_evict() using the guarded LRU iteration
2025-06-29 3:51 [bug report] drm/ttm, drm_xe, Implement ttm_lru_walk_for_evict() using the guarded LRU iteration Dan Carpenter
@ 2025-07-21 14:35 ` Thomas Hellström
0 siblings, 0 replies; 4+ messages in thread
From: Thomas Hellström @ 2025-07-21 14:35 UTC (permalink / raw)
To: Dan Carpenter; +Cc: dri-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* [bug report] drm/ttm, drm_xe, Implement ttm_lru_walk_for_evict() using the guarded LRU iteration
@ 2025-07-01 17:49 Dan Carpenter
2025-07-21 14:10 ` Thomas Hellström
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2025-07-01 17:49 UTC (permalink / raw)
To: Thomas Hellström; +Cc: dri-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [bug report] drm/ttm, drm_xe, Implement ttm_lru_walk_for_evict() using the guarded LRU iteration
2025-07-01 17:49 Dan Carpenter
@ 2025-07-21 14:10 ` Thomas Hellström
0 siblings, 0 replies; 4+ messages in thread
From: Thomas Hellström @ 2025-07-21 14:10 UTC (permalink / raw)
To: Dan Carpenter; +Cc: dri-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-07-21 14:35 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-29 3:51 [bug report] drm/ttm, drm_xe, Implement ttm_lru_walk_for_evict() using the guarded LRU iteration Dan Carpenter
2025-07-21 14:35 ` Thomas Hellström
-- strict thread matches above, loose matches on Subject: below --
2025-07-01 17:49 Dan Carpenter
2025-07-21 14:10 ` Thomas Hellström
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.