Re: [Intel-gfx] [RFC 2/5] drm/i915: Expose list of clients in sysfs

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-10-25 15:21:28)
>  int i915_gem_open(struct drm_i915_private *i915, struct drm_file *file)
>  {
> +   int ret = -ENOMEM;
> struct drm_i915_file_private *file_priv;
> -   int ret;
>  
> DRM_DEBUG("\n");
>  
> file_priv = kzalloc(sizeof(*file_priv), GFP_KERNEL);
> if (!file_priv)
> -   return -ENOMEM;
> +   goto err_alloc;
> +
> +   file_priv->client.id = atomic_inc_return(>clients.serial);

We should make this a cyclic ida to avoid reuse on wraparound. 32b
wraps will happen, and they will still have client 0 alive! :)

That will mean we need a lock.

(Of course you could use -EEXIST from add_client and keep incrementing
serial until you find a hole :)
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC 2/5] drm/i915: Expose list of clients in sysfs

[Tvrtko Ursulin]

On 14/02/2018 19:13, Chris Wilson wrote:

Quoting Tvrtko Ursulin (2018-02-14 18:50:32)

From: Tvrtko Ursulin 

Expose a list of clients with open file handles in sysfs.

This will be a basis for a top-like utility showing per-client and per-
engine GPU load.

Currently we only expose each client's pid and name under opaque numbered
directories in /sys/class/drm/card0/clients/.

For instance:

/sys/class/drm/card0/clients/3/name: Xorg
/sys/class/drm/card0/clients/3/pid: 5664

Signed-off-by: Tvrtko Ursulin 
---
  drivers/gpu/drm/i915/i915_drv.h   |  13 +
  drivers/gpu/drm/i915/i915_gem.c   | 112 +++---
  drivers/gpu/drm/i915/i915_sysfs.c |   8 +++
  3 files changed, 126 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 70cf289855af..2133e67f5fbc 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -345,6 +345,16 @@ struct drm_i915_file_private {
   */
  #define I915_MAX_CLIENT_CONTEXT_BANS 3
 atomic_t context_bans;
+


struct {


+   unsigned int client_sysfs_id;
+   unsigned int client_pid;
+   char *client_name;
+   struct kobject *client_root;


} client;


client_sysfs?


+
+   struct {
+   struct device_attribute pid;
+   struct device_attribute name;
+   } attr;


sysfs_attr?


Ok.


  };
  
  /* Interface history:

@@ -2365,6 +2375,9 @@ struct drm_i915_private {
  
 struct i915_pmu pmu;
  
+   struct kobject *clients_root;

+   atomic_t client_serial;


I'd start a new substruct { } clients;


Ok.


 /*
  * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
  * will be rejected. Instead look for a better place.
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index fc68b35854df..24607bce2efe 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5613,6 +5613,89 @@ int i915_gem_freeze_late(struct drm_i915_private 
*dev_priv)
 return 0;
  }
  
+static ssize_t

+show_client_name(struct device *kdev, struct device_attribute *attr, char *buf)
+{
+   struct drm_i915_file_private *file_priv =
+   container_of(attr, struct drm_i915_file_private, attr.name);
+
+   return snprintf(buf, PAGE_SIZE, "%s", file_priv->client_name);
+}
+
+static ssize_t
+show_client_pid(struct device *kdev, struct device_attribute *attr, char *buf)
+{
+   struct drm_i915_file_private *file_priv =
+   container_of(attr, struct drm_i915_file_private, attr.pid);
+
+   return snprintf(buf, PAGE_SIZE, "%u", file_priv->client_pid);
+}
+
+static int
+i915_gem_add_client(struct drm_i915_private *i915,
+   struct drm_i915_file_private *file_priv,
+   struct task_struct *task,
+   unsigned int serial)
+{
+   int ret = -ENOMEM;
+   struct device_attribute *attr;
+   char id[32];
+
+   file_priv->client_name = kstrdup(task->comm, GFP_KERNEL);
+   if (!file_priv->client_name)
+   goto err_name;
+
+   snprintf(id, sizeof(id), "%u", serial);
+   file_priv->client_root = kobject_create_and_add(id,
+   i915->clients_root);


Do we have to care about i915->clients_root being NULL here?


Yep, due our lax handling of sysfs registration failures. :/


+   if (!file_priv->client_root)
+   goto err_client;
+
+   attr = _priv->attr.name;
+   attr->attr.name = "name";
+   attr->attr.mode = 0444;
+   attr->show = show_client_name;
+
+   ret = sysfs_create_file(file_priv->client_root,
+   (struct attribute *)attr);
+   if (ret)
+   goto err_attr_name;
+
+   attr = _priv->attr.pid;
+   attr->attr.name = "pid";
+   attr->attr.mode = 0444;
+   attr->show = show_client_pid;
+
+   ret = sysfs_create_file(file_priv->client_root,
+   (struct attribute *)attr);
+   if (ret)
+   goto err_attr_pid;
+
+   file_priv->client_pid = pid_nr(get_task_pid(task, PIDTYPE_PID));


Are we before or after the "create_context get new pid"? Just wondering
aloud how that's going to tie in.


Before, create_context then updates it in a following patch. Which 
probably needs improving to avoid remove and re-add, and add a proper 
update helper.





+
+   return 0;
+
+err_attr_pid:
+   sysfs_remove_file(file_priv->client_root,
+ (struct attribute *)_priv->attr.name);
+err_attr_name:
+   kobject_put(file_priv->client_root);
+err_client:
+   kfree(file_priv->client_name);
+err_name:
+   return ret;
+}
+
+static void i915_gem_remove_client(struct drm_i915_file_private *file_priv)
+{
+   sysfs_remove_file(file_priv->client_root,
+ (struct 

Re: [Intel-gfx] [RFC 2/5] drm/i915: Expose list of clients in sysfs

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-02-14 18:50:32)
> From: Tvrtko Ursulin 
> 
> Expose a list of clients with open file handles in sysfs.
> 
> This will be a basis for a top-like utility showing per-client and per-
> engine GPU load.
> 
> Currently we only expose each client's pid and name under opaque numbered
> directories in /sys/class/drm/card0/clients/.
> 
> For instance:
> 
> /sys/class/drm/card0/clients/3/name: Xorg
> /sys/class/drm/card0/clients/3/pid: 5664
> 
> Signed-off-by: Tvrtko Ursulin 
> ---
>  drivers/gpu/drm/i915/i915_drv.h   |  13 +
>  drivers/gpu/drm/i915/i915_gem.c   | 112 
> +++---
>  drivers/gpu/drm/i915/i915_sysfs.c |   8 +++
>  3 files changed, 126 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 70cf289855af..2133e67f5fbc 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -345,6 +345,16 @@ struct drm_i915_file_private {
>   */
>  #define I915_MAX_CLIENT_CONTEXT_BANS 3
> atomic_t context_bans;
> +

struct {

> +   unsigned int client_sysfs_id;
> +   unsigned int client_pid;
> +   char *client_name;
> +   struct kobject *client_root;

} client;

> +
> +   struct {
> +   struct device_attribute pid;
> +   struct device_attribute name;
> +   } attr;

sysfs_attr?

>  };
>  
>  /* Interface history:
> @@ -2365,6 +2375,9 @@ struct drm_i915_private {
>  
> struct i915_pmu pmu;
>  
> +   struct kobject *clients_root;
> +   atomic_t client_serial;

I'd start a new substruct { } clients;

> /*
>  * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your 
> patch
>  * will be rejected. Instead look for a better place.
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index fc68b35854df..24607bce2efe 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -5613,6 +5613,89 @@ int i915_gem_freeze_late(struct drm_i915_private 
> *dev_priv)
> return 0;
>  }
>  
> +static ssize_t
> +show_client_name(struct device *kdev, struct device_attribute *attr, char 
> *buf)
> +{
> +   struct drm_i915_file_private *file_priv =
> +   container_of(attr, struct drm_i915_file_private, attr.name);
> +
> +   return snprintf(buf, PAGE_SIZE, "%s", file_priv->client_name);
> +}
> +
> +static ssize_t
> +show_client_pid(struct device *kdev, struct device_attribute *attr, char 
> *buf)
> +{
> +   struct drm_i915_file_private *file_priv =
> +   container_of(attr, struct drm_i915_file_private, attr.pid);
> +
> +   return snprintf(buf, PAGE_SIZE, "%u", file_priv->client_pid);
> +}
> +
> +static int
> +i915_gem_add_client(struct drm_i915_private *i915,
> +   struct drm_i915_file_private *file_priv,
> +   struct task_struct *task,
> +   unsigned int serial)
> +{
> +   int ret = -ENOMEM;
> +   struct device_attribute *attr;
> +   char id[32];
> +
> +   file_priv->client_name = kstrdup(task->comm, GFP_KERNEL);
> +   if (!file_priv->client_name)
> +   goto err_name;
> +
> +   snprintf(id, sizeof(id), "%u", serial);
> +   file_priv->client_root = kobject_create_and_add(id,
> +   i915->clients_root);

Do we have to care about i915->clients_root being NULL here?

> +   if (!file_priv->client_root)
> +   goto err_client;
> +
> +   attr = _priv->attr.name;
> +   attr->attr.name = "name";
> +   attr->attr.mode = 0444;
> +   attr->show = show_client_name;
> +
> +   ret = sysfs_create_file(file_priv->client_root,
> +   (struct attribute *)attr);
> +   if (ret)
> +   goto err_attr_name;
> +
> +   attr = _priv->attr.pid;
> +   attr->attr.name = "pid";
> +   attr->attr.mode = 0444;
> +   attr->show = show_client_pid;
> +
> +   ret = sysfs_create_file(file_priv->client_root,
> +   (struct attribute *)attr);
> +   if (ret)
> +   goto err_attr_pid;
> +
> +   file_priv->client_pid = pid_nr(get_task_pid(task, PIDTYPE_PID));

Are we before or after the "create_context get new pid"? Just wondering
aloud how that's going to tie in.

> +
> +   return 0;
> +
> +err_attr_pid:
> +   sysfs_remove_file(file_priv->client_root,
> + (struct attribute *)_priv->attr.name);
> +err_attr_name:
> +   kobject_put(file_priv->client_root);
> +err_client:
> +   kfree(file_priv->client_name);
> +err_name:
> +   return ret;
> +}
> +
> +static void i915_gem_remove_client(struct drm_i915_file_private *file_priv)
> +{
> +   sysfs_remove_file(file_priv->client_root,
> + (struct attribute *)_priv->attr.pid);
> +