Re: [PATCH 2/8] drm/fdinfo: Switch to idr_for_each() in drm_show_memory_stats()

[Simona Vetter <simona.vetter@ffwll.ch>]

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 <robdclark@chromium.org>
> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: <stable@vger.kernel.org> # v6.5+
> Signed-off-by: Simona Vetter <simona.vetter@intel.com>
> Signed-off-by: Simona Vetter <simona.vetter@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