diff mbox series

[1/5] drm/i915: Add ability for tracking buffer objects per client

Message ID 20230707130220.3966836-2-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series fdinfo memory stats | expand

Commit Message

Tvrtko Ursulin July 7, 2023, 1:02 p.m. UTC
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

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@intel.com>
---
 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(-)

Comments

Iddamsetty, Aravind July 10, 2023, 10:44 a.m. UTC | #1
On 07-07-2023 18:32, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> 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@intel.com>
> ---
>  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.

Thanks,
Aravind.
> +
> +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__ */
Tvrtko Ursulin July 10, 2023, 1:20 p.m. UTC | #2
On 10/07/2023 11:44, Iddamsetty, Aravind wrote:
> On 07-07-2023 18:32, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> 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@intel.com>
>> ---
>>   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.

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__ */
Iddamsetty, Aravind July 11, 2023, 7:48 a.m. UTC | #3
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@intel.com>
>>>
>>> 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@intel.com>
>>> ---
>>>   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__ */
Tvrtko Ursulin July 11, 2023, 9:39 a.m. UTC | #4
On 11/07/2023 08:48, Iddamsetty, Aravind wrote:
> 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@intel.com>
>>>>
>>>> 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@intel.com>
>>>> ---
>>>>    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.

Yes it is/would. I was talking about hypothetically not using object_idr 
and instead tracking everything via the mechanism introduced in this 
patch, which would allow for lockless fdinfo reads for everything. If 
you remember I had that approach in an earlier version but it needed a 
patch to drm code to split the prime helpers (or so) and also did not 
cover the question on how to handle flink.

Regards,

Tvrtko

> 
> 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__ */
diff mbox series

Patch

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);
+}
+
+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__ */