On 10-07-2023 18:50, Tvrtko Ursulin wrote: > > On 10/07/2023 11:44, Iddamsetty, Aravind wrote: >> On 07-07-2023 18:32, Tvrtko Ursulin wrote: >>> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> >>> >>> In order to show per client memory usage lets add some infrastructure >>> which enables tracking buffer objects owned by clients. >>> >>> We add a per client list protected by a new per client lock and to >>> support >>> delayed destruction (post client exit) we make tracked objects hold >>> references to the owning client. >>> >>> Also, object memory region teardown is moved to the existing RCU free >>> callback to allow safe dereference from the fdinfo RCU read section. >>> >>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> >>> --- >>> drivers/gpu/drm/i915/gem/i915_gem_object.c | 13 +++++-- >>> .../gpu/drm/i915/gem/i915_gem_object_types.h | 12 +++++++ >>> drivers/gpu/drm/i915/i915_drm_client.c | 36 +++++++++++++++++++ >>> drivers/gpu/drm/i915/i915_drm_client.h | 32 +++++++++++++++++ >>> 4 files changed, 90 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c >>> b/drivers/gpu/drm/i915/gem/i915_gem_object.c >>> index 97ac6fb37958..3dc4fbb67d2b 100644 >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c >>> @@ -105,6 +105,10 @@ void i915_gem_object_init(struct >>> drm_i915_gem_object *obj, >>> INIT_LIST_HEAD(&obj->mm.link); >>> +#ifdef CONFIG_PROC_FS >>> + INIT_LIST_HEAD(&obj->client_link); >>> +#endif >>> + >>> INIT_LIST_HEAD(&obj->lut_list); >>> spin_lock_init(&obj->lut_lock); >>> @@ -292,6 +296,10 @@ void __i915_gem_free_object_rcu(struct >>> rcu_head *head) >>> container_of(head, typeof(*obj), rcu); >>> struct drm_i915_private *i915 = to_i915(obj->base.dev); >>> + /* We need to keep this alive for RCU read access from fdinfo. */ >>> + if (obj->mm.n_placements > 1) >>> + kfree(obj->mm.placements); >>> + >>> i915_gem_object_free(obj); >>> GEM_BUG_ON(!atomic_read(&i915->mm.free_count)); >>> @@ -388,9 +396,6 @@ void __i915_gem_free_object(struct >>> drm_i915_gem_object *obj) >>> if (obj->ops->release) >>> obj->ops->release(obj); >>> - if (obj->mm.n_placements > 1) >>> - kfree(obj->mm.placements); >>> - >>> if (obj->shares_resv_from) >>> i915_vm_resv_put(obj->shares_resv_from); >>> @@ -441,6 +446,8 @@ static void i915_gem_free_object(struct >>> drm_gem_object *gem_obj) >>> GEM_BUG_ON(i915_gem_object_is_framebuffer(obj)); >>> + i915_drm_client_remove_object(obj); >>> + >>> /* >>> * Before we free the object, make sure any pure RCU-only >>> * read-side critical sections are complete, e.g. >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h >>> b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h >>> index e72c57716bee..8de2b91b3edf 100644 >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h >>> @@ -300,6 +300,18 @@ struct drm_i915_gem_object { >>> */ >>> struct i915_address_space *shares_resv_from; >>> +#ifdef CONFIG_PROC_FS >>> + /** >>> + * @client: @i915_drm_client which created the object >>> + */ >>> + struct i915_drm_client *client; >>> + >>> + /** >>> + * @client_link: Link into @i915_drm_client.objects_list >>> + */ >>> + struct list_head client_link; >>> +#endif >>> + >>> union { >>> struct rcu_head rcu; >>> struct llist_node freed; >>> diff --git a/drivers/gpu/drm/i915/i915_drm_client.c >>> b/drivers/gpu/drm/i915/i915_drm_client.c >>> index 2a44b3876cb5..2e5e69edc0f9 100644 >>> --- a/drivers/gpu/drm/i915/i915_drm_client.c >>> +++ b/drivers/gpu/drm/i915/i915_drm_client.c >>> @@ -28,6 +28,10 @@ struct i915_drm_client *i915_drm_client_alloc(void) >>> kref_init(&client->kref); >>> spin_lock_init(&client->ctx_lock); >>> INIT_LIST_HEAD(&client->ctx_list); >>> +#ifdef CONFIG_PROC_FS >>> + spin_lock_init(&client->objects_lock); >>> + INIT_LIST_HEAD(&client->objects_list); >>> +#endif >>> return client; >>> } >>> @@ -108,4 +112,36 @@ void i915_drm_client_fdinfo(struct drm_printer >>> *p, struct drm_file *file) >>> for (i = 0; i < ARRAY_SIZE(uabi_class_names); i++) >>> show_client_class(p, i915, file_priv->client, i); >>> } >>> + >>> +void i915_drm_client_add_object(struct i915_drm_client *client, >>> + struct drm_i915_gem_object *obj) >>> +{ >>> + unsigned long flags; >>> + >>> + GEM_WARN_ON(obj->client); >>> + GEM_WARN_ON(!list_empty(&obj->client_link)); >>> + >>> + spin_lock_irqsave(&client->objects_lock, flags); >>> + obj->client = i915_drm_client_get(client); >>> + list_add_tail_rcu(&obj->client_link, &client->objects_list); >>> + spin_unlock_irqrestore(&client->objects_lock, flags); >>> +} >> >> would it be nice to mention that we use this client infra only to track >> internal objects. While the user created through file->object_idr added >> during handle creation time. > > In this series it is indeed only used for that. > > But it would be nicer to use it to track everything, so fdinfo readers > would not be hitting the idr lock, which would avoid injecting latency > to real DRM clients. > > The only fly in the ointment IMO is that I needed that drm core helper > to be able to track dmabuf imports. Possibly something for flink too, > did not look into that yet. wouldn't dmabuf be tracked via object_idr as a new handle is created for it. Thanks, Aravind. > > In the light of all that I can mention in the cover letter next time > round. It is a bit stale anyway (the cover letter). > > Regards, > > Tvrtko > >>> +bool i915_drm_client_remove_object(struct drm_i915_gem_object *obj) >>> +{ >>> + struct i915_drm_client *client = fetch_and_zero(&obj->client); >>> + unsigned long flags; >>> + >>> + /* Object may not be associated with a client. */ >>> + if (!client) >>> + return false; >>> + >>> + spin_lock_irqsave(&client->objects_lock, flags); >>> + list_del_rcu(&obj->client_link); >>> + spin_unlock_irqrestore(&client->objects_lock, flags); >>> + >>> + i915_drm_client_put(client); >>> + >>> + return true; >>> +} >>> #endif >>> diff --git a/drivers/gpu/drm/i915/i915_drm_client.h >>> b/drivers/gpu/drm/i915/i915_drm_client.h >>> index 67816c912bca..5f58fdf7dcb8 100644 >>> --- a/drivers/gpu/drm/i915/i915_drm_client.h >>> +++ b/drivers/gpu/drm/i915/i915_drm_client.h >>> @@ -12,6 +12,9 @@ >>> #include <uapi/drm/i915_drm.h> >>> +#include "i915_file_private.h" >>> +#include "gem/i915_gem_object_types.h" >>> + >>> #define I915_LAST_UABI_ENGINE_CLASS I915_ENGINE_CLASS_COMPUTE >>> struct drm_file; >>> @@ -25,6 +28,20 @@ struct i915_drm_client { >>> spinlock_t ctx_lock; /* For add/remove from ctx_list. */ >>> struct list_head ctx_list; /* List of contexts belonging to >>> client. */ >>> +#ifdef CONFIG_PROC_FS >>> + /** >>> + * @objects_lock: lock protecting @objects_list >>> + */ >>> + spinlock_t objects_lock; >>> + >>> + /** >>> + * @objects_list: list of objects created by this client >>> + * >>> + * Protected by @objects_lock. >>> + */ >>> + struct list_head objects_list; >>> +#endif >>> + >>> /** >>> * @past_runtime: Accumulation of pphwsp runtimes from closed >>> contexts. >>> */ >>> @@ -49,4 +66,19 @@ struct i915_drm_client *i915_drm_client_alloc(void); >>> void i915_drm_client_fdinfo(struct drm_printer *p, struct >>> drm_file *file); >>> +#ifdef CONFIG_PROC_FS >>> +void i915_drm_client_add_object(struct i915_drm_client *client, >>> + struct drm_i915_gem_object *obj); >>> +bool i915_drm_client_remove_object(struct drm_i915_gem_object *obj); >>> +#else >>> +static inline void i915_drm_client_add_object(struct i915_drm_client >>> *client, >>> + struct drm_i915_gem_object *obj) >>> +{ >>> +} >>> + >>> +static inline bool i915_drm_client_remove_object(struct >>> drm_i915_gem_object *obj) >>> +{ >>> +} >>> +#endif >>> + >>> #endif /* !__I915_DRM_CLIENT_H__ */