Thanks! I'll wait to let Felix/others weigh in before pushing it in. Kent
-----Original Message----- From: Koenig, Christian <[email protected]> Sent: Wednesday, June 19, 2019 10:45 AM To: Russell, Kent <[email protected]>; [email protected] Subject: Re: [PATCH] drm/amdkfd: Add procfs-style information for KFD processes Ok, that's what I wanted to know. Feel free to add my Acked-by to the patch. Thanks, Christian. Am 19.06.19 um 16:42 schrieb Russell, Kent: > 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 <[email protected]> > Sent: Wednesday, June 19, 2019 10:35 AM > To: Russell, Kent <[email protected]>; > [email protected] > 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 <[email protected]> >> Sent: Wednesday, June 19, 2019 10:04 AM >> To: Russell, Kent <[email protected]>; >> [email protected] >> 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 <[email protected]> >>> --- >>> 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 [email protected] https://lists.freedesktop.org/mailman/listinfo/amd-gfx
