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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 14/02/2018 19:13, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-02-14 18:50:32)
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> 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 <tvrtko.ursulin@intel.com>
>> ---
>>   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 = &file_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 = &file_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 *)&file_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 *)&file_priv->attr.pid);
>> +       sysfs_remove_file(file_priv->client_root,
>> +                         (struct attribute *)&file_priv->attr.name);
>> +       kobject_put(file_priv->client_root);
>> +       kfree(file_priv->client_name);
>> +}
>> +
>>   void i915_gem_release(struct drm_device *dev, struct drm_file *file)
>>   {
>>          struct drm_i915_file_private *file_priv = file->driver_priv;
>> @@ -5626,32 +5709,47 @@ void i915_gem_release(struct drm_device *dev, struct drm_file *file)
>>          list_for_each_entry(request, &file_priv->mm.request_list, client_link)
>>                  request->file_priv = NULL;
>>          spin_unlock(&file_priv->mm.lock);
>> +
>> +       i915_gem_remove_client(file_priv);
>>   }
>>   
>>   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_sysfs_id = atomic_inc_return(&i915->client_serial);
> 
> What's the use for client_sysfd_id? I don't see it used again.

Used from context create pid update, but its weak.. I'll eliminate it 
after above mentioned improvements.

Regards,

Tvrtko

> 
>> +       ret = i915_gem_add_client(i915, file_priv, current,
>> +                                 file_priv->client_sysfs_id);
>> +       if (ret)
>> +               goto err_client;
>>   
>>          file->driver_priv = file_priv;
>> +       ret = i915_gem_context_open(i915, file);
>> +       if (ret)
>> +               goto err_context;
>> +
>>          file_priv->dev_priv = i915;
>>          file_priv->file = file;
>> +       file_priv->bsd_engine = -1;
>>   
>>          spin_lock_init(&file_priv->mm.lock);
>>          INIT_LIST_HEAD(&file_priv->mm.request_list);
>>   
>> -       file_priv->bsd_engine = -1;
>> -
>> -       ret = i915_gem_context_open(i915, file);
>> -       if (ret)
>> -               kfree(file_priv);
>> +       return 0;
>>   
>> +err_context:
>> +       i915_gem_remove_client(file_priv);
>> +err_client:
>> +       atomic_dec(&i915->client_serial);
>> +       kfree(file_priv);
>> +err_alloc:
>>          return ret;
>>   }
>>   
>> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
>> index b33d2158c234..6cf032bc1573 100644
>> --- a/drivers/gpu/drm/i915/i915_sysfs.c
>> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
>> @@ -575,6 +575,11 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv)
>>          struct device *kdev = dev_priv->drm.primary->kdev;
>>          int ret;
>>   
>> +       dev_priv->clients_root =
>> +               kobject_create_and_add("clients", &kdev->kobj);
>> +       if (!dev_priv->clients_root)
>> +               DRM_ERROR("Per-client sysfs setup failed\n");
>> +
>>   #ifdef CONFIG_PM
>>          if (HAS_RC6(dev_priv)) {
>>                  ret = sysfs_merge_group(&kdev->kobj,
>> @@ -635,4 +640,7 @@ void i915_teardown_sysfs(struct drm_i915_private *dev_priv)
>>          sysfs_unmerge_group(&kdev->kobj, &rc6_attr_group);
>>          sysfs_unmerge_group(&kdev->kobj, &rc6p_attr_group);
>>   #endif
>> +
>> +       if (dev_priv->clients_root)
>> +               kobject_put(dev_priv->clients_root);
>>   }
>> -- 
>> 2.14.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx