On Wed, May 28, 2025 at 11:13:00AM +0200, Simona Vetter wrote:
> Unlike idr_for_each_entry(), which terminates on the first NULL entry,
> idr_for_each passes them through. This fixes potential issues with the
> idr walk terminating prematurely due to transient NULL entries the
> exist when creating and destroying a handle.
>
> Note that transient NULL pointers in drm_file.object_idr have been a
> thing since f6cd7daecff5 ("drm: Release driver references to handle
> before making it available again"), this is a really old issue.
>
> Aside from temporarily inconsistent fdinfo statistic there's no other
> impact of this issue.
>
> Fixes: 686b21b5f6ca ("drm: Add fdinfo memory stats")
> Cc: Rob Clark <robdcl...@chromium.org>
> Cc: Emil Velikov <emil.l.veli...@gmail.com>
> Cc: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
> Cc: <sta...@vger.kernel.org> # v6.5+
> Signed-off-by: Simona Vetter <simona.vet...@intel.com>
> Signed-off-by: Simona Vetter <simona.vet...@ffwll.ch>
Ok I screwed up reading idr_for_each_entry() respectively
idr_get_next_ul() big time, it already copes with NULL entries entirely
fine.
Mea culpa.
-Sima
> ---
> drivers/gpu/drm/drm_file.c | 95 ++++++++++++++++++++++----------------
> 1 file changed, 55 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index 246cf845e2c9..428a4eb85e94 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -892,6 +892,58 @@ void drm_print_memory_stats(struct drm_printer *p,
> }
> EXPORT_SYMBOL(drm_print_memory_stats);
>
> +struct drm_bo_print_data {
> + struct drm_memory_stats status;
> + enum drm_gem_object_status supported_status;
> +};
> +
> +static int
> +drm_bo_memory_stats(int id, void *ptr, void *data)
> +{
> + struct drm_bo_print_data *drm_data;
> + struct drm_gem_object *obj = ptr;
> + enum drm_gem_object_status s = 0;
> + size_t add_size;
> +
> + if (!obj)
> + return 0;
> +
> + add_size = (obj->funcs && obj->funcs->rss) ?
> + obj->funcs->rss(obj) : obj->size;
> +
> + if (obj->funcs && obj->funcs->status) {
> + s = obj->funcs->status(obj);
> + drm_data->supported_status |= s;
> + }
> +
> + if (drm_gem_object_is_shared_for_memory_stats(obj))
> + drm_data->status.shared += obj->size;
> + else
> + drm_data->status.private += obj->size;
> +
> + if (s & DRM_GEM_OBJECT_RESIDENT) {
> + drm_data->status.resident += add_size;
> + } else {
> + /* If already purged or not yet backed by pages, don't
> + * count it as purgeable:
> + */
> + s &= ~DRM_GEM_OBJECT_PURGEABLE;
> + }
> +
> + if (!dma_resv_test_signaled(obj->resv, dma_resv_usage_rw(true))) {
> + drm_data->status.active += add_size;
> + drm_data->supported_status |= DRM_GEM_OBJECT_ACTIVE;
> +
> + /* If still active, don't count as purgeable: */
> + s &= ~DRM_GEM_OBJECT_PURGEABLE;
> + }
> +
> + if (s & DRM_GEM_OBJECT_PURGEABLE)
> + drm_data->status.purgeable += add_size;
> +
> + return 0;
> +}
> +
> /**
> * drm_show_memory_stats - Helper to collect and show standard fdinfo memory
> stats
> * @p: the printer to print output to
> @@ -902,50 +954,13 @@ EXPORT_SYMBOL(drm_print_memory_stats);
> */
> void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
> {
> - struct drm_gem_object *obj;
> - struct drm_memory_stats status = {};
> - enum drm_gem_object_status supported_status = 0;
> - int id;
> + struct drm_bo_print_data data = {};
>
> spin_lock(&file->table_lock);
> - idr_for_each_entry (&file->object_idr, obj, id) {
> - enum drm_gem_object_status s = 0;
> - size_t add_size = (obj->funcs && obj->funcs->rss) ?
> - obj->funcs->rss(obj) : obj->size;
> -
> - if (obj->funcs && obj->funcs->status) {
> - s = obj->funcs->status(obj);
> - supported_status |= s;
> - }
> -
> - if (drm_gem_object_is_shared_for_memory_stats(obj))
> - status.shared += obj->size;
> - else
> - status.private += obj->size;
> -
> - if (s & DRM_GEM_OBJECT_RESIDENT) {
> - status.resident += add_size;
> - } else {
> - /* If already purged or not yet backed by pages, don't
> - * count it as purgeable:
> - */
> - s &= ~DRM_GEM_OBJECT_PURGEABLE;
> - }
> -
> - if (!dma_resv_test_signaled(obj->resv,
> dma_resv_usage_rw(true))) {
> - status.active += add_size;
> - supported_status |= DRM_GEM_OBJECT_ACTIVE;
> -
> - /* If still active, don't count as purgeable: */
> - s &= ~DRM_GEM_OBJECT_PURGEABLE;
> - }
> -
> - if (s & DRM_GEM_OBJECT_PURGEABLE)
> - status.purgeable += add_size;
> - }
> + idr_for_each(&file->object_idr, &drm_bo_memory_stats, &data);
> spin_unlock(&file->table_lock);
>
> - drm_print_memory_stats(p, &status, supported_status, "memory");
> + drm_print_memory_stats(p, &data.status, data.supported_status,
> "memory");
> }
> EXPORT_SYMBOL(drm_show_memory_stats);
>
> --
> 2.49.0
>
--
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
