I'd rather it be in debugfs too, but the requirements are that it be exposed through the SMI. And whenever we do anything that requires root for reading in the SMI, people complain (they expect root for writing, but I had dozens of complaints/bug reports when reporting voltage via debugfs required root). That's why we did things like moving the voltage, memory usage, etc to sysfs.
So unfortunately it can't go in debugfs, even though that's where I would have preferred it. I know that it kind of locks us in interface-wise though.
Kent
-----Original Message-----
From: Koenig, Christian <Christian.Koenig@xxxxxxx>
Sent: Wednesday, June 19, 2019 10:35 AM
To: Russell, Kent <Kent.Russell@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
Subject: Re: [PATCH] drm/amdkfd: Add procfs-style information for KFD processes
Do we need a stable interface? Would debugfs do as well?
I mean in general looks good for sysfs as well, just want to double check.
Christian.
Am 19.06.19 um 16:28 schrieb Russell, Kent:
> Right now the use case would be to list which processes were created in a KFD context, but it would allow for further expansion to include things like the GPU associated with the PID, memory usage, etc. For now, the use case is listing KFD-related PIDs, but will be expanded later to include memory usage for sure (plus other things that I expect will requested later on).
>
> Kent
>
> -----Original Message-----
> From: Christian König <ckoenig.leichtzumerken@xxxxxxxxx>
> Sent: Wednesday, June 19, 2019 10:04 AM
> To: Russell, Kent <Kent.Russell@xxxxxxx>;
> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH] drm/amdkfd: Add procfs-style information for KFD
> processes
>
> Am 19.06.19 um 16:01 schrieb Russell, Kent:
>> Add a folder structure to /sys/class/kfd/kfd/ called proc which
>> contains subfolders, each representing an active KFD process' PID,
>> containing 1
>> file: pasid.
> What is the use case of that information? In other words would it be maybe better to create debugfs entries instead?
>
> Christian.
>
>> Change-Id: Id3dfab8a6250264434b34ccddbcdb459d1da7478
>> Signed-off-by: Kent Russell <kent.russell@xxxxxxx>
>> ---
>> drivers/gpu/drm/amd/amdkfd/kfd_module.c | 6 ++
>> drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 9 ++
>> drivers/gpu/drm/amd/amdkfd/kfd_process.c | 100 ++++++++++++++++++++++-
>> 3 files changed, 113 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_module.c
>> b/drivers/gpu/drm/amd/amdkfd/kfd_module.c
>> index 932007eb9168..986ff52d5750 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_module.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_module.c
>> @@ -56,6 +56,11 @@ static int kfd_init(void)
>> if (err < 0)
>> goto err_create_wq;
>>
>> + /* Ignore the return value, so that we can continue
>> + * to init the KFD, even if procfs isn't craated
>> + */
>> + kfd_procfs_init();
>> +
>> kfd_debugfs_init();
>>
>> return 0;
>> @@ -72,6 +77,7 @@ static void kfd_exit(void)
>> {
>> kfd_debugfs_fini();
>> kfd_process_destroy_wq();
>> + kfd_procfs_shutdown();
>> kfd_topology_shutdown();
>> kfd_chardev_exit();
>> }
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>> b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>> index da589ee1366c..bd01396c8cea 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>> @@ -35,6 +35,7 @@
>> #include <linux/kfifo.h>
>> #include <linux/seq_file.h>
>> #include <linux/kref.h>
>> +#include <linux/sysfs.h>
>> #include <kgd_kfd_interface.h>
>>
>> #include "amd_shared.h"
>> @@ -718,6 +719,10 @@ struct kfd_process {
>> * restored after an eviction
>> */
>> unsigned long last_restore_timestamp;
>> +
>> + /* Kobj for our procfs */
>> + struct kobject *kobj;
>> + struct attribute attr_pasid;
>> };
>>
>> #define KFD_PROCESS_TABLE_SIZE 5 /* bits: 32 entries */ @@ -820,6
>> +825,10 @@ int kfd_gtt_sa_free(struct kfd_dev *kfd, struct
>> +kfd_mem_obj
>> *mem_obj);
>>
>> extern struct device *kfd_device;
>>
>> +/* KFD's procfs */
>> +void kfd_procfs_init(void);
>> +void kfd_procfs_shutdown(void);
>> +
>> /* Topology */
>> int kfd_topology_init(void);
>> void kfd_topology_shutdown(void);
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> index 4bdae78bab8e..ed2d83f93fd8 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> @@ -68,6 +68,68 @@ static struct kfd_process *create_process(const struct task_struct *thread,
>> static void evict_process_worker(struct work_struct *work);
>> static void restore_process_worker(struct work_struct *work);
>>
>> +struct kfd_procfs_tree {
>> + struct kobject *kobj;
>> +};
>> +
>> +static struct kfd_procfs_tree procfs;
>> +
>> +static ssize_t kfd_procfs_show(struct kobject *kobj, struct attribute *attr,
>> + char *buffer)
>> +{
>> + int val = 0;
>> +
>> + if (strcmp(attr->name, "pasid") == 0) {
>> + struct kfd_process *p = container_of(attr, struct kfd_process,
>> + attr_pasid);
>> + val = p->pasid;
>> + } else {
>> + pr_err("Invalid attribute");
>> + return -EINVAL;
>> + }
>> +
>> + return snprintf(buffer, PAGE_SIZE, "%d\n", val); }
>> +
>> +static void kfd_procfs_kobj_release(struct kobject *kobj) {
>> + kfree(kobj);
>> +}
>> +
>> +static const struct sysfs_ops kfd_procfs_ops = {
>> + .show = kfd_procfs_show,
>> +};
>> +
>> +static struct kobj_type procfs_type = {
>> + .release = kfd_procfs_kobj_release,
>> + .sysfs_ops = &kfd_procfs_ops,
>> +};
>> +
>> +void kfd_procfs_init(void)
>> +{
>> + int ret = 0;
>> +
>> + procfs.kobj = kfd_alloc_struct(procfs.kobj);
>> + if (!procfs.kobj)
>> + return;
>> +
>> + ret = kobject_init_and_add(procfs.kobj, &procfs_type,
>> + &kfd_device->kobj, "proc");
>> + if (ret) {
>> + pr_warn("Could not create procfs proc folder");
>> + /* If we fail to create the procfs, clean up */
>> + kfd_procfs_shutdown();
>> + }
>> +}
>> +
>> +void kfd_procfs_shutdown(void)
>> +{
>> + if (procfs.kobj) {
>> + kobject_del(procfs.kobj);
>> + kobject_put(procfs.kobj);
>> + procfs.kobj = NULL;
>> + }
>> +}
>>
>> int kfd_process_create_wq(void)
>> {
>> @@ -206,6 +268,7 @@ struct kfd_process *kfd_create_process(struct file *filep)
>> {
>> struct kfd_process *process;
>> struct task_struct *thread = current;
>> + int ret;
>>
>> if (!thread->mm)
>> return ERR_PTR(-EINVAL);
>> @@ -223,11 +286,36 @@ struct kfd_process *kfd_create_process(struct
>> file *filep)
>>
>> /* A prior open of /dev/kfd could have already created the process. */
>> process = find_process(thread);
>> - if (process)
>> + if (process) {
>> pr_debug("Process already found\n");
>> - else
>> + } else {
>> process = create_process(thread, filep);
>>
>> + if (!procfs.kobj)
>> + goto out;
>> +
>> + process->kobj = kfd_alloc_struct(process->kobj);
>> + if (!process->kobj) {
>> + pr_warn("Creating procfs kobject failed");
>> + goto out;
>> + }
>> + ret = kobject_init_and_add(process->kobj, &procfs_type,
>> + procfs.kobj, "%d",
>> + (int)process->lead_thread->pid);
>> + if (ret) {
>> + pr_warn("Creating procfs pid directory failed");
>> + goto out;
>> + }
>> +
>> + process->attr_pasid.name = "pasid";
>> + process->attr_pasid.mode = KFD_SYSFS_FILE_MODE;
>> + sysfs_attr_init(&process->attr_pasid);
>> + ret = sysfs_create_file(process->kobj, &process->attr_pasid);
>> + if (ret)
>> + pr_warn("Creating pasid for pid %d failed",
>> + (int)process->lead_thread->pid);
>> + }
>> +out:
>> mutex_unlock(&kfd_processes_mutex);
>>
>> return process;
>> @@ -355,6 +443,14 @@ static void kfd_process_wq_release(struct work_struct *work)
>> struct kfd_process *p = container_of(work, struct kfd_process,
>> release_work);
>>
>> + /* Remove the procfs files */
>> + if (p->kobj) {
>> + sysfs_remove_file(p->kobj, &p->attr_pasid);
>> + kobject_del(p->kobj);
>> + kobject_put(p->kobj);
>> + p->kobj = NULL;
>> + }
>> +
>> kfd_iommu_unbind_process(p);
>>
>> kfd_process_free_outstanding_kfd_bos(p);
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
