* [PATCH v3 2/3] drm/amdkfd: Add process eviction counters to sysfs @ 2020-09-16 18:42 Philip Cox 2020-09-16 22:46 ` Felix Kuehling 0 siblings, 1 reply; 4+ messages in thread From: Philip Cox @ 2020-09-16 18:42 UTC (permalink / raw) To: amd-gfx Cc: Jonathan.Kim, Felix.Kuehling, Ramesh.Errabolu, Laurent.Morichetti, Philip Cox, Tony.Tye Add per-process eviction counters to sysfs to keep track of how many eviction events have happened for each process. v2: rename the stats dir, and track all evictions per process, per device. v3: Simplify the stats kobject handling and cleanup. Signed-off-by: Philip Cox <Philip.Cox@amd.com> --- .../drm/amd/amdkfd/kfd_device_queue_manager.c | 9 ++ drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 9 +- drivers/gpu/drm/amd/amdkfd/kfd_process.c | 97 +++++++++++++++++++ 3 files changed, 114 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c index cafbc3aa980a..5b9e0df2a90e 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c @@ -653,6 +653,7 @@ static int evict_process_queues_nocpsch(struct device_queue_manager *dqm, pr_info_ratelimited("Evicting PASID 0x%x queues\n", pdd->process->pasid); + pdd->last_evict_timestamp = get_jiffies_64(); /* Mark all queues as evicted. Deactivate all active queues on * the qpd. */ @@ -714,6 +715,7 @@ static int evict_process_queues_cpsch(struct device_queue_manager *dqm, q->properties.is_active = false; decrement_queue_count(dqm, q->properties.type); } + pdd->last_evict_timestamp = get_jiffies_64(); retval = execute_queues_cpsch(dqm, qpd->is_debug ? KFD_UNMAP_QUEUES_FILTER_ALL_QUEUES : @@ -732,6 +734,7 @@ static int restore_process_queues_nocpsch(struct device_queue_manager *dqm, struct mqd_manager *mqd_mgr; struct kfd_process_device *pdd; uint64_t pd_base; + uint64_t eviction_duration; int retval, ret = 0; pdd = qpd_to_pdd(qpd); @@ -799,6 +802,8 @@ static int restore_process_queues_nocpsch(struct device_queue_manager *dqm, ret = retval; } qpd->evicted = 0; + eviction_duration = get_jiffies_64() - pdd->last_evict_timestamp; + atomic64_add(eviction_duration, &pdd->evict_duration_counter); out: if (mm) mmput(mm); @@ -812,6 +817,7 @@ static int restore_process_queues_cpsch(struct device_queue_manager *dqm, struct queue *q; struct kfd_process_device *pdd; uint64_t pd_base; + uint64_t eviction_duration; int retval = 0; pdd = qpd_to_pdd(qpd); @@ -845,6 +851,9 @@ static int restore_process_queues_cpsch(struct device_queue_manager *dqm, retval = execute_queues_cpsch(dqm, KFD_UNMAP_QUEUES_FILTER_DYNAMIC_QUEUES, 0); qpd->evicted = 0; + eviction_duration = get_jiffies_64() - pdd->last_evict_timestamp; + atomic64_add(eviction_duration, &pdd->evict_duration_counter); + out: dqm_unlock(dqm); return retval; diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h index 023629f28495..a500fe611b43 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h @@ -631,7 +631,7 @@ enum kfd_pdd_bound { PDD_BOUND_SUSPENDED, }; -#define MAX_SYSFS_FILENAME_LEN 11 +#define MAX_SYSFS_FILENAME_LEN 15 /* * SDMA counter runs at 100MHz frequency. @@ -692,6 +692,13 @@ struct kfd_process_device { uint64_t sdma_past_activity_counter; struct attribute attr_sdma; char sdma_filename[MAX_SYSFS_FILENAME_LEN]; + + /* Eviction activity tracking */ + unsigned long last_evict_timestamp; + atomic64_t evict_duration_counter; + struct attribute attr_evict; + + struct kobject *kobj_stats; }; #define qpd_to_pdd(x) container_of(x, struct kfd_process_device, qpd) diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c index 1e15aa7d8ae8..b4ba394ad599 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c @@ -344,6 +344,26 @@ static ssize_t kfd_procfs_queue_show(struct kobject *kobj, return 0; } +static ssize_t kfd_procfs_stats_show(struct kobject *kobj, + struct attribute *attr, char *buffer) +{ + if (strcmp(attr->name, "evicted_ms") == 0) { + struct kfd_process_device *pdd = container_of(attr, + struct kfd_process_device, + attr_evict); + uint64_t evict_jiffies; + + evict_jiffies = atomic64_read(&pdd->evict_duration_counter); + + return snprintf(buffer, + PAGE_SIZE, + "%llu\n", + jiffies64_to_msecs(evict_jiffies)); + } else + pr_err("Invalid attribute"); + + return 0; +} static struct attribute attr_queue_size = { .name = "size", @@ -376,6 +396,19 @@ static struct kobj_type procfs_queue_type = { .default_attrs = procfs_queue_attrs, }; +static const struct sysfs_ops procfs_stats_ops = { + .show = kfd_procfs_stats_show, +}; + +static struct attribute *procfs_stats_attrs[] = { + NULL +}; + +static struct kobj_type procfs_stats_type = { + .sysfs_ops = &procfs_stats_ops, + .default_attrs = procfs_stats_attrs, +}; + int kfd_procfs_add_queue(struct queue *q) { struct kfd_process *proc; @@ -417,6 +450,60 @@ static int kfd_sysfs_create_file(struct kfd_process *p, struct attribute *attr, return ret; } +static int kfd_procfs_add_sysfs_stats(struct kfd_process *p) +{ + int ret = 0; + struct kfd_process_device *pdd; + char stats_dir_filename[MAX_SYSFS_FILENAME_LEN]; + + if (!p) + return -EINVAL; + + if (!p->kobj) + return -EFAULT; + + /* + * Create sysfs files for each GPU: + * - proc/<pid>/stats_<gpuid>/ + * - proc/<pid>/stats_<gpuid>/evicted_ms + */ + list_for_each_entry(pdd, &p->per_device_data, per_device_list) { + struct kobject *kobj_stats; + + + snprintf(stats_dir_filename, MAX_SYSFS_FILENAME_LEN, + "stats_%u", pdd->dev->id); + kobj_stats = kfd_alloc_struct(kobj_stats); + if (!kobj_stats) { + kfree(kobj_stats); + return -ENOMEM; + } + + ret = kobject_init_and_add(kobj_stats, + &procfs_stats_type, + p->kobj, + stats_dir_filename); + + if (ret) { + pr_warn("Creating KFD proc/stats_%s folder failed", + stats_dir_filename); + kobject_put(kobj_stats); + goto err; + } + + pdd->kobj_stats = kobj_stats; + pdd->attr_evict.name = "evicted_ms"; + pdd->attr_evict.mode = KFD_SYSFS_FILE_MODE; + sysfs_attr_init(&pdd->attr_evict); + ret = sysfs_create_file(kobj_stats, &pdd->attr_evict); + if (ret) + pr_warn("Creating eviction stats for gpuid %d failed", + (int)pdd->dev->id); + } +err: + return ret; +} + static int kfd_procfs_add_sysfs_files(struct kfd_process *p) { int ret = 0; @@ -660,6 +747,11 @@ struct kfd_process *kfd_create_process(struct file *filep) if (!process->kobj_queues) pr_warn("Creating KFD proc/queues folder failed"); + ret = kfd_procfs_add_sysfs_stats(process); + if (ret) + pr_warn("Creating sysfs stats dir for pid %d failed", + (int)process->lead_thread->pid); + ret = kfd_procfs_add_sysfs_files(process); if (ret) pr_warn("Creating sysfs usage file for pid %d failed", @@ -816,6 +908,10 @@ static void kfd_process_wq_release(struct work_struct *work) list_for_each_entry(pdd, &p->per_device_data, per_device_list) { sysfs_remove_file(p->kobj, &pdd->attr_vram); sysfs_remove_file(p->kobj, &pdd->attr_sdma); + sysfs_remove_file(p->kobj, &pdd->attr_evict); + kobject_del(pdd->kobj_stats); + kobject_put(pdd->kobj_stats); + pdd->kobj_stats = NULL; } kobject_del(p->kobj); @@ -1125,6 +1221,7 @@ struct kfd_process_device *kfd_create_process_device_data(struct kfd_dev *dev, pdd->runtime_inuse = false; pdd->vram_usage = 0; pdd->sdma_past_activity_counter = 0; + atomic64_set(&pdd->evict_duration_counter, 0); list_add(&pdd->per_device_list, &p->per_device_data); /* Init idr used for memory handle translation */ -- 2.17.1 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3 2/3] drm/amdkfd: Add process eviction counters to sysfs 2020-09-16 18:42 [PATCH v3 2/3] drm/amdkfd: Add process eviction counters to sysfs Philip Cox @ 2020-09-16 22:46 ` Felix Kuehling 2020-09-17 13:11 ` Cox, Philip 0 siblings, 1 reply; 4+ messages in thread From: Felix Kuehling @ 2020-09-16 22:46 UTC (permalink / raw) To: Philip Cox, amd-gfx Cc: Tony.Tye, Ramesh.Errabolu, Laurent.Morichetti, Jonathan.Kim Some nit-picks and one more possible simplification inline. I want to make adding more stats later as painless as possible. Looks good otherwise. Am 2020-09-16 um 2:42 p.m. schrieb Philip Cox: > Add per-process eviction counters to sysfs to keep track of > how many eviction events have happened for each process. > > v2: rename the stats dir, and track all evictions per process, per device. > v3: Simplify the stats kobject handling and cleanup. > > Signed-off-by: Philip Cox <Philip.Cox@amd.com> > --- > .../drm/amd/amdkfd/kfd_device_queue_manager.c | 9 ++ > drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 9 +- > drivers/gpu/drm/amd/amdkfd/kfd_process.c | 97 +++++++++++++++++++ > 3 files changed, 114 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c > index cafbc3aa980a..5b9e0df2a90e 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c > @@ -653,6 +653,7 @@ static int evict_process_queues_nocpsch(struct device_queue_manager *dqm, > pr_info_ratelimited("Evicting PASID 0x%x queues\n", > pdd->process->pasid); > > + pdd->last_evict_timestamp = get_jiffies_64(); > /* Mark all queues as evicted. Deactivate all active queues on > * the qpd. > */ > @@ -714,6 +715,7 @@ static int evict_process_queues_cpsch(struct device_queue_manager *dqm, > q->properties.is_active = false; > decrement_queue_count(dqm, q->properties.type); > } > + pdd->last_evict_timestamp = get_jiffies_64(); > retval = execute_queues_cpsch(dqm, > qpd->is_debug ? > KFD_UNMAP_QUEUES_FILTER_ALL_QUEUES : > @@ -732,6 +734,7 @@ static int restore_process_queues_nocpsch(struct device_queue_manager *dqm, > struct mqd_manager *mqd_mgr; > struct kfd_process_device *pdd; > uint64_t pd_base; > + uint64_t eviction_duration; > int retval, ret = 0; > > pdd = qpd_to_pdd(qpd); > @@ -799,6 +802,8 @@ static int restore_process_queues_nocpsch(struct device_queue_manager *dqm, > ret = retval; > } > qpd->evicted = 0; > + eviction_duration = get_jiffies_64() - pdd->last_evict_timestamp; > + atomic64_add(eviction_duration, &pdd->evict_duration_counter); > out: > if (mm) > mmput(mm); > @@ -812,6 +817,7 @@ static int restore_process_queues_cpsch(struct device_queue_manager *dqm, > struct queue *q; > struct kfd_process_device *pdd; > uint64_t pd_base; > + uint64_t eviction_duration; > int retval = 0; > > pdd = qpd_to_pdd(qpd); > @@ -845,6 +851,9 @@ static int restore_process_queues_cpsch(struct device_queue_manager *dqm, > retval = execute_queues_cpsch(dqm, > KFD_UNMAP_QUEUES_FILTER_DYNAMIC_QUEUES, 0); > qpd->evicted = 0; > + eviction_duration = get_jiffies_64() - pdd->last_evict_timestamp; > + atomic64_add(eviction_duration, &pdd->evict_duration_counter); > + > out: > dqm_unlock(dqm); > return retval; > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h > index 023629f28495..a500fe611b43 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h > @@ -631,7 +631,7 @@ enum kfd_pdd_bound { > PDD_BOUND_SUSPENDED, > }; > > -#define MAX_SYSFS_FILENAME_LEN 11 > +#define MAX_SYSFS_FILENAME_LEN 15 > > /* > * SDMA counter runs at 100MHz frequency. > @@ -692,6 +692,13 @@ struct kfd_process_device { > uint64_t sdma_past_activity_counter; > struct attribute attr_sdma; > char sdma_filename[MAX_SYSFS_FILENAME_LEN]; > + > + /* Eviction activity tracking */ > + unsigned long last_evict_timestamp; get_jiffies_64 returns u64. You should use an equivalent type (uint64_t) here for consistency. > + atomic64_t evict_duration_counter; > + struct attribute attr_evict; > + > + struct kobject *kobj_stats; > }; > > #define qpd_to_pdd(x) container_of(x, struct kfd_process_device, qpd) > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c > index 1e15aa7d8ae8..b4ba394ad599 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c > @@ -344,6 +344,26 @@ static ssize_t kfd_procfs_queue_show(struct kobject *kobj, > > return 0; > } > +static ssize_t kfd_procfs_stats_show(struct kobject *kobj, > + struct attribute *attr, char *buffer) > +{ > + if (strcmp(attr->name, "evicted_ms") == 0) { > + struct kfd_process_device *pdd = container_of(attr, > + struct kfd_process_device, > + attr_evict); > + uint64_t evict_jiffies; > + > + evict_jiffies = atomic64_read(&pdd->evict_duration_counter); > + > + return snprintf(buffer, > + PAGE_SIZE, > + "%llu\n", > + jiffies64_to_msecs(evict_jiffies)); > + } else > + pr_err("Invalid attribute"); > + > + return 0; > +} > > static struct attribute attr_queue_size = { > .name = "size", > @@ -376,6 +396,19 @@ static struct kobj_type procfs_queue_type = { > .default_attrs = procfs_queue_attrs, > }; > > +static const struct sysfs_ops procfs_stats_ops = { > + .show = kfd_procfs_stats_show, > +}; > + > +static struct attribute *procfs_stats_attrs[] = { > + NULL > +}; We could probably use this to populate the attributes in stats automatically instead of calling sysfs_create_file and sysfs_remove_file manually. Then we may also not need the attr_evict attribute in the pdd. > + > +static struct kobj_type procfs_stats_type = { > + .sysfs_ops = &procfs_stats_ops, > + .default_attrs = procfs_stats_attrs, > +}; > + > int kfd_procfs_add_queue(struct queue *q) > { > struct kfd_process *proc; > @@ -417,6 +450,60 @@ static int kfd_sysfs_create_file(struct kfd_process *p, struct attribute *attr, > return ret; > } > > +static int kfd_procfs_add_sysfs_stats(struct kfd_process *p) > +{ > + int ret = 0; > + struct kfd_process_device *pdd; > + char stats_dir_filename[MAX_SYSFS_FILENAME_LEN]; > + > + if (!p) > + return -EINVAL; > + > + if (!p->kobj) > + return -EFAULT; > + > + /* > + * Create sysfs files for each GPU: > + * - proc/<pid>/stats_<gpuid>/ > + * - proc/<pid>/stats_<gpuid>/evicted_ms > + */ > + list_for_each_entry(pdd, &p->per_device_data, per_device_list) { > + struct kobject *kobj_stats; > + > + > + snprintf(stats_dir_filename, MAX_SYSFS_FILENAME_LEN, > + "stats_%u", pdd->dev->id); > + kobj_stats = kfd_alloc_struct(kobj_stats); > + if (!kobj_stats) { > + kfree(kobj_stats); If allocation failed, there is nothing to free. Regards, Felix > + return -ENOMEM; > + } > + > + ret = kobject_init_and_add(kobj_stats, > + &procfs_stats_type, > + p->kobj, > + stats_dir_filename); > + > + if (ret) { > + pr_warn("Creating KFD proc/stats_%s folder failed", > + stats_dir_filename); > + kobject_put(kobj_stats); > + goto err; > + } > + > + pdd->kobj_stats = kobj_stats; > + pdd->attr_evict.name = "evicted_ms"; > + pdd->attr_evict.mode = KFD_SYSFS_FILE_MODE; > + sysfs_attr_init(&pdd->attr_evict); > + ret = sysfs_create_file(kobj_stats, &pdd->attr_evict); > + if (ret) > + pr_warn("Creating eviction stats for gpuid %d failed", > + (int)pdd->dev->id); > + } > +err: > + return ret; > +} > + > static int kfd_procfs_add_sysfs_files(struct kfd_process *p) > { > int ret = 0; > @@ -660,6 +747,11 @@ struct kfd_process *kfd_create_process(struct file *filep) > if (!process->kobj_queues) > pr_warn("Creating KFD proc/queues folder failed"); > > + ret = kfd_procfs_add_sysfs_stats(process); > + if (ret) > + pr_warn("Creating sysfs stats dir for pid %d failed", > + (int)process->lead_thread->pid); > + > ret = kfd_procfs_add_sysfs_files(process); > if (ret) > pr_warn("Creating sysfs usage file for pid %d failed", > @@ -816,6 +908,10 @@ static void kfd_process_wq_release(struct work_struct *work) > list_for_each_entry(pdd, &p->per_device_data, per_device_list) { > sysfs_remove_file(p->kobj, &pdd->attr_vram); > sysfs_remove_file(p->kobj, &pdd->attr_sdma); > + sysfs_remove_file(p->kobj, &pdd->attr_evict); > + kobject_del(pdd->kobj_stats); > + kobject_put(pdd->kobj_stats); > + pdd->kobj_stats = NULL; > } > > kobject_del(p->kobj); > @@ -1125,6 +1221,7 @@ struct kfd_process_device *kfd_create_process_device_data(struct kfd_dev *dev, > pdd->runtime_inuse = false; > pdd->vram_usage = 0; > pdd->sdma_past_activity_counter = 0; > + atomic64_set(&pdd->evict_duration_counter, 0); > list_add(&pdd->per_device_list, &p->per_device_data); > > /* Init idr used for memory handle translation */ _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH v3 2/3] drm/amdkfd: Add process eviction counters to sysfs 2020-09-16 22:46 ` Felix Kuehling @ 2020-09-17 13:11 ` Cox, Philip 2020-09-17 14:32 ` Felix Kuehling 0 siblings, 1 reply; 4+ messages in thread From: Cox, Philip @ 2020-09-17 13:11 UTC (permalink / raw) To: Kuehling, Felix, amd-gfx@lists.freedesktop.org Cc: Tye, Tony, Errabolu, Ramesh, Morichetti, Laurent, Kim, Jonathan [AMD Official Use Only - Internal Distribution Only] > > +static struct attribute *procfs_stats_attrs[] = { > > + NULL > > +}; > > We could probably use this to populate the attributes in stats automatically instead of calling sysfs_create_file and sysfs_remove_file manually. Then we may also not need the attr_evict attribute in the pdd. We use the attr_evict as an anchor to locate the pdd in kfd_procfs_stats_show(). So, if we use the default attributes, and drop the calls to sysfs_create_file, and sysfs_remove_file, it makes kfd_procfs_stats_show() much more complicated, as we then need to find the correct pdd without using the anchor attr_evict. Also, if we create the file via the default attributes, as you suggest, and don't drop the attr_evict, we get incorrect results. The code is much cleaner I think leaving the calls to sysfs_create_file, and sysfs_remove_file() as they are, and leaving the default stats attributes NULL. If some other stats are added later, that don't require the pdd, then they can be added to this structure, but I don't think the eviction stats should be. -----Original Message----- From: Kuehling, Felix <Felix.Kuehling@amd.com> Sent: Wednesday, September 16, 2020 6:46 PM To: Cox, Philip <Philip.Cox@amd.com>; amd-gfx@lists.freedesktop.org Cc: Tye, Tony <Tony.Tye@amd.com>; Morichetti, Laurent <Laurent.Morichetti@amd.com>; Kim, Jonathan <Jonathan.Kim@amd.com>; Errabolu, Ramesh <Ramesh.Errabolu@amd.com> Subject: Re: [PATCH v3 2/3] drm/amdkfd: Add process eviction counters to sysfs Some nit-picks and one more possible simplification inline. I want to make adding more stats later as painless as possible. Looks good otherwise. Am 2020-09-16 um 2:42 p.m. schrieb Philip Cox: > Add per-process eviction counters to sysfs to keep track of how many > eviction events have happened for each process. > > v2: rename the stats dir, and track all evictions per process, per device. > v3: Simplify the stats kobject handling and cleanup. > > Signed-off-by: Philip Cox <Philip.Cox@amd.com> > --- > .../drm/amd/amdkfd/kfd_device_queue_manager.c | 9 ++ > drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 9 +- > drivers/gpu/drm/amd/amdkfd/kfd_process.c | 97 +++++++++++++++++++ > 3 files changed, 114 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c > b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c > index cafbc3aa980a..5b9e0df2a90e 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c > @@ -653,6 +653,7 @@ static int evict_process_queues_nocpsch(struct device_queue_manager *dqm, > pr_info_ratelimited("Evicting PASID 0x%x queues\n", > pdd->process->pasid); > > + pdd->last_evict_timestamp = get_jiffies_64(); > /* Mark all queues as evicted. Deactivate all active queues on > * the qpd. > */ > @@ -714,6 +715,7 @@ static int evict_process_queues_cpsch(struct device_queue_manager *dqm, > q->properties.is_active = false; > decrement_queue_count(dqm, q->properties.type); > } > + pdd->last_evict_timestamp = get_jiffies_64(); > retval = execute_queues_cpsch(dqm, > qpd->is_debug ? > KFD_UNMAP_QUEUES_FILTER_ALL_QUEUES : > @@ -732,6 +734,7 @@ static int restore_process_queues_nocpsch(struct device_queue_manager *dqm, > struct mqd_manager *mqd_mgr; > struct kfd_process_device *pdd; > uint64_t pd_base; > + uint64_t eviction_duration; > int retval, ret = 0; > > pdd = qpd_to_pdd(qpd); > @@ -799,6 +802,8 @@ static int restore_process_queues_nocpsch(struct device_queue_manager *dqm, > ret = retval; > } > qpd->evicted = 0; > + eviction_duration = get_jiffies_64() - pdd->last_evict_timestamp; > + atomic64_add(eviction_duration, &pdd->evict_duration_counter); > out: > if (mm) > mmput(mm); > @@ -812,6 +817,7 @@ static int restore_process_queues_cpsch(struct device_queue_manager *dqm, > struct queue *q; > struct kfd_process_device *pdd; > uint64_t pd_base; > + uint64_t eviction_duration; > int retval = 0; > > pdd = qpd_to_pdd(qpd); > @@ -845,6 +851,9 @@ static int restore_process_queues_cpsch(struct device_queue_manager *dqm, > retval = execute_queues_cpsch(dqm, > KFD_UNMAP_QUEUES_FILTER_DYNAMIC_QUEUES, 0); > qpd->evicted = 0; > + eviction_duration = get_jiffies_64() - pdd->last_evict_timestamp; > + atomic64_add(eviction_duration, &pdd->evict_duration_counter); > + > out: > dqm_unlock(dqm); > return retval; > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h > b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h > index 023629f28495..a500fe611b43 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h > @@ -631,7 +631,7 @@ enum kfd_pdd_bound { > PDD_BOUND_SUSPENDED, > }; > > -#define MAX_SYSFS_FILENAME_LEN 11 > +#define MAX_SYSFS_FILENAME_LEN 15 > > /* > * SDMA counter runs at 100MHz frequency. > @@ -692,6 +692,13 @@ struct kfd_process_device { > uint64_t sdma_past_activity_counter; > struct attribute attr_sdma; > char sdma_filename[MAX_SYSFS_FILENAME_LEN]; > + > + /* Eviction activity tracking */ > + unsigned long last_evict_timestamp; get_jiffies_64 returns u64. You should use an equivalent type (uint64_t) here for consistency. > + atomic64_t evict_duration_counter; > + struct attribute attr_evict; > + > + struct kobject *kobj_stats; > }; > > #define qpd_to_pdd(x) container_of(x, struct kfd_process_device, qpd) > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c > b/drivers/gpu/drm/amd/amdkfd/kfd_process.c > index 1e15aa7d8ae8..b4ba394ad599 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c > @@ -344,6 +344,26 @@ static ssize_t kfd_procfs_queue_show(struct > kobject *kobj, > > return 0; > } > +static ssize_t kfd_procfs_stats_show(struct kobject *kobj, > + struct attribute *attr, char *buffer) { > + if (strcmp(attr->name, "evicted_ms") == 0) { > + struct kfd_process_device *pdd = container_of(attr, > + struct kfd_process_device, > + attr_evict); > + uint64_t evict_jiffies; > + > + evict_jiffies = atomic64_read(&pdd->evict_duration_counter); > + > + return snprintf(buffer, > + PAGE_SIZE, > + "%llu\n", > + jiffies64_to_msecs(evict_jiffies)); > + } else > + pr_err("Invalid attribute"); > + > + return 0; > +} > > static struct attribute attr_queue_size = { > .name = "size", > @@ -376,6 +396,19 @@ static struct kobj_type procfs_queue_type = { > .default_attrs = procfs_queue_attrs, }; > > +static const struct sysfs_ops procfs_stats_ops = { > + .show = kfd_procfs_stats_show, > +}; > + > +static struct attribute *procfs_stats_attrs[] = { > + NULL > +}; We could probably use this to populate the attributes in stats automatically instead of calling sysfs_create_file and sysfs_remove_file manually. Then we may also not need the attr_evict attribute in the pdd. > + > +static struct kobj_type procfs_stats_type = { > + .sysfs_ops = &procfs_stats_ops, > + .default_attrs = procfs_stats_attrs, }; > + > int kfd_procfs_add_queue(struct queue *q) { > struct kfd_process *proc; > @@ -417,6 +450,60 @@ static int kfd_sysfs_create_file(struct kfd_process *p, struct attribute *attr, > return ret; > } > > +static int kfd_procfs_add_sysfs_stats(struct kfd_process *p) { > + int ret = 0; > + struct kfd_process_device *pdd; > + char stats_dir_filename[MAX_SYSFS_FILENAME_LEN]; > + > + if (!p) > + return -EINVAL; > + > + if (!p->kobj) > + return -EFAULT; > + > + /* > + * Create sysfs files for each GPU: > + * - proc/<pid>/stats_<gpuid>/ > + * - proc/<pid>/stats_<gpuid>/evicted_ms > + */ > + list_for_each_entry(pdd, &p->per_device_data, per_device_list) { > + struct kobject *kobj_stats; > + > + > + snprintf(stats_dir_filename, MAX_SYSFS_FILENAME_LEN, > + "stats_%u", pdd->dev->id); > + kobj_stats = kfd_alloc_struct(kobj_stats); > + if (!kobj_stats) { > + kfree(kobj_stats); If allocation failed, there is nothing to free. Regards, Felix > + return -ENOMEM; > + } > + > + ret = kobject_init_and_add(kobj_stats, > + &procfs_stats_type, > + p->kobj, > + stats_dir_filename); > + > + if (ret) { > + pr_warn("Creating KFD proc/stats_%s folder failed", > + stats_dir_filename); > + kobject_put(kobj_stats); > + goto err; > + } > + > + pdd->kobj_stats = kobj_stats; > + pdd->attr_evict.name = "evicted_ms"; > + pdd->attr_evict.mode = KFD_SYSFS_FILE_MODE; > + sysfs_attr_init(&pdd->attr_evict); > + ret = sysfs_create_file(kobj_stats, &pdd->attr_evict); > + if (ret) > + pr_warn("Creating eviction stats for gpuid %d failed", > + (int)pdd->dev->id); > + } > +err: > + return ret; > +} > + > static int kfd_procfs_add_sysfs_files(struct kfd_process *p) { > int ret = 0; > @@ -660,6 +747,11 @@ struct kfd_process *kfd_create_process(struct file *filep) > if (!process->kobj_queues) > pr_warn("Creating KFD proc/queues folder failed"); > > + ret = kfd_procfs_add_sysfs_stats(process); > + if (ret) > + pr_warn("Creating sysfs stats dir for pid %d failed", > + (int)process->lead_thread->pid); > + > ret = kfd_procfs_add_sysfs_files(process); > if (ret) > pr_warn("Creating sysfs usage file for pid %d failed", @@ -816,6 > +908,10 @@ static void kfd_process_wq_release(struct work_struct *work) > list_for_each_entry(pdd, &p->per_device_data, per_device_list) { > sysfs_remove_file(p->kobj, &pdd->attr_vram); > sysfs_remove_file(p->kobj, &pdd->attr_sdma); > + sysfs_remove_file(p->kobj, &pdd->attr_evict); > + kobject_del(pdd->kobj_stats); > + kobject_put(pdd->kobj_stats); > + pdd->kobj_stats = NULL; > } > > kobject_del(p->kobj); > @@ -1125,6 +1221,7 @@ struct kfd_process_device *kfd_create_process_device_data(struct kfd_dev *dev, > pdd->runtime_inuse = false; > pdd->vram_usage = 0; > pdd->sdma_past_activity_counter = 0; > + atomic64_set(&pdd->evict_duration_counter, 0); > list_add(&pdd->per_device_list, &p->per_device_data); > > /* Init idr used for memory handle translation */ _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3 2/3] drm/amdkfd: Add process eviction counters to sysfs 2020-09-17 13:11 ` Cox, Philip @ 2020-09-17 14:32 ` Felix Kuehling 0 siblings, 0 replies; 4+ messages in thread From: Felix Kuehling @ 2020-09-17 14:32 UTC (permalink / raw) To: Cox, Philip, amd-gfx@lists.freedesktop.org Cc: Tye, Tony, Errabolu, Ramesh, Morichetti, Laurent, Kim, Jonathan Am 2020-09-17 um 9:11 a.m. schrieb Cox, Philip: > [AMD Official Use Only - Internal Distribution Only] > >>> +static struct attribute *procfs_stats_attrs[] = { >>> + NULL >>> +}; >> We could probably use this to populate the attributes in stats automatically instead of calling sysfs_create_file and sysfs_remove_file manually. Then we may also not need the attr_evict attribute in the pdd. > > We use the attr_evict as an anchor to locate the pdd in kfd_procfs_stats_show(). So, if we use the default attributes, and drop the calls to sysfs_create_file, and sysfs_remove_file, it makes kfd_procfs_stats_show() much more complicated, as we then need to find the correct pdd without using the anchor attr_evict. > > Also, if we create the file via the default attributes, as you suggest, and don't drop the attr_evict, we get incorrect results. > > The code is much cleaner I think leaving the calls to sysfs_create_file, and sysfs_remove_file() as they are, and leaving the default stats attributes NULL. If some other stats are added later, that don't require the pdd, then they can be added to this structure, but I don't think the eviction stats should be. Thanks. Makes sense. I expect that all the stats will need the PDD. They are all per-process, per-device stats. With the small nit-picks fixed, the patch is Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com> Regards, Felix > > -----Original Message----- > From: Kuehling, Felix <Felix.Kuehling@amd.com> > Sent: Wednesday, September 16, 2020 6:46 PM > To: Cox, Philip <Philip.Cox@amd.com>; amd-gfx@lists.freedesktop.org > Cc: Tye, Tony <Tony.Tye@amd.com>; Morichetti, Laurent <Laurent.Morichetti@amd.com>; Kim, Jonathan <Jonathan.Kim@amd.com>; Errabolu, Ramesh <Ramesh.Errabolu@amd.com> > Subject: Re: [PATCH v3 2/3] drm/amdkfd: Add process eviction counters to sysfs > > Some nit-picks and one more possible simplification inline. I want to make adding more stats later as painless as possible. > > Looks good otherwise. > > > Am 2020-09-16 um 2:42 p.m. schrieb Philip Cox: >> Add per-process eviction counters to sysfs to keep track of how many >> eviction events have happened for each process. >> >> v2: rename the stats dir, and track all evictions per process, per device. >> v3: Simplify the stats kobject handling and cleanup. >> >> Signed-off-by: Philip Cox <Philip.Cox@amd.com> >> --- >> .../drm/amd/amdkfd/kfd_device_queue_manager.c | 9 ++ >> drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 9 +- >> drivers/gpu/drm/amd/amdkfd/kfd_process.c | 97 +++++++++++++++++++ >> 3 files changed, 114 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c >> b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c >> index cafbc3aa980a..5b9e0df2a90e 100644 >> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c >> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c >> @@ -653,6 +653,7 @@ static int evict_process_queues_nocpsch(struct device_queue_manager *dqm, >> pr_info_ratelimited("Evicting PASID 0x%x queues\n", >> pdd->process->pasid); >> >> + pdd->last_evict_timestamp = get_jiffies_64(); >> /* Mark all queues as evicted. Deactivate all active queues on >> * the qpd. >> */ >> @@ -714,6 +715,7 @@ static int evict_process_queues_cpsch(struct device_queue_manager *dqm, >> q->properties.is_active = false; >> decrement_queue_count(dqm, q->properties.type); >> } >> + pdd->last_evict_timestamp = get_jiffies_64(); >> retval = execute_queues_cpsch(dqm, >> qpd->is_debug ? >> KFD_UNMAP_QUEUES_FILTER_ALL_QUEUES : >> @@ -732,6 +734,7 @@ static int restore_process_queues_nocpsch(struct device_queue_manager *dqm, >> struct mqd_manager *mqd_mgr; >> struct kfd_process_device *pdd; >> uint64_t pd_base; >> + uint64_t eviction_duration; >> int retval, ret = 0; >> >> pdd = qpd_to_pdd(qpd); >> @@ -799,6 +802,8 @@ static int restore_process_queues_nocpsch(struct device_queue_manager *dqm, >> ret = retval; >> } >> qpd->evicted = 0; >> + eviction_duration = get_jiffies_64() - pdd->last_evict_timestamp; >> + atomic64_add(eviction_duration, &pdd->evict_duration_counter); >> out: >> if (mm) >> mmput(mm); >> @@ -812,6 +817,7 @@ static int restore_process_queues_cpsch(struct device_queue_manager *dqm, >> struct queue *q; >> struct kfd_process_device *pdd; >> uint64_t pd_base; >> + uint64_t eviction_duration; >> int retval = 0; >> >> pdd = qpd_to_pdd(qpd); >> @@ -845,6 +851,9 @@ static int restore_process_queues_cpsch(struct device_queue_manager *dqm, >> retval = execute_queues_cpsch(dqm, >> KFD_UNMAP_QUEUES_FILTER_DYNAMIC_QUEUES, 0); >> qpd->evicted = 0; >> + eviction_duration = get_jiffies_64() - pdd->last_evict_timestamp; >> + atomic64_add(eviction_duration, &pdd->evict_duration_counter); >> + >> out: >> dqm_unlock(dqm); >> return retval; >> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h >> b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h >> index 023629f28495..a500fe611b43 100644 >> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h >> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h >> @@ -631,7 +631,7 @@ enum kfd_pdd_bound { >> PDD_BOUND_SUSPENDED, >> }; >> >> -#define MAX_SYSFS_FILENAME_LEN 11 >> +#define MAX_SYSFS_FILENAME_LEN 15 >> >> /* >> * SDMA counter runs at 100MHz frequency. >> @@ -692,6 +692,13 @@ struct kfd_process_device { >> uint64_t sdma_past_activity_counter; >> struct attribute attr_sdma; >> char sdma_filename[MAX_SYSFS_FILENAME_LEN]; >> + >> + /* Eviction activity tracking */ >> + unsigned long last_evict_timestamp; > get_jiffies_64 returns u64. You should use an equivalent type (uint64_t) here for consistency. > > >> + atomic64_t evict_duration_counter; >> + struct attribute attr_evict; >> + >> + struct kobject *kobj_stats; >> }; >> >> #define qpd_to_pdd(x) container_of(x, struct kfd_process_device, qpd) >> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c >> b/drivers/gpu/drm/amd/amdkfd/kfd_process.c >> index 1e15aa7d8ae8..b4ba394ad599 100644 >> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c >> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c >> @@ -344,6 +344,26 @@ static ssize_t kfd_procfs_queue_show(struct >> kobject *kobj, >> >> return 0; >> } >> +static ssize_t kfd_procfs_stats_show(struct kobject *kobj, >> + struct attribute *attr, char *buffer) { >> + if (strcmp(attr->name, "evicted_ms") == 0) { >> + struct kfd_process_device *pdd = container_of(attr, >> + struct kfd_process_device, >> + attr_evict); >> + uint64_t evict_jiffies; >> + >> + evict_jiffies = atomic64_read(&pdd->evict_duration_counter); >> + >> + return snprintf(buffer, >> + PAGE_SIZE, >> + "%llu\n", >> + jiffies64_to_msecs(evict_jiffies)); >> + } else >> + pr_err("Invalid attribute"); >> + >> + return 0; >> +} >> >> static struct attribute attr_queue_size = { >> .name = "size", >> @@ -376,6 +396,19 @@ static struct kobj_type procfs_queue_type = { >> .default_attrs = procfs_queue_attrs, }; >> >> +static const struct sysfs_ops procfs_stats_ops = { >> + .show = kfd_procfs_stats_show, >> +}; >> + >> +static struct attribute *procfs_stats_attrs[] = { >> + NULL >> +}; > We could probably use this to populate the attributes in stats automatically instead of calling sysfs_create_file and sysfs_remove_file manually. Then we may also not need the attr_evict attribute in the pdd. > > >> + >> +static struct kobj_type procfs_stats_type = { >> + .sysfs_ops = &procfs_stats_ops, >> + .default_attrs = procfs_stats_attrs, }; >> + >> int kfd_procfs_add_queue(struct queue *q) { >> struct kfd_process *proc; >> @@ -417,6 +450,60 @@ static int kfd_sysfs_create_file(struct kfd_process *p, struct attribute *attr, >> return ret; >> } >> >> +static int kfd_procfs_add_sysfs_stats(struct kfd_process *p) { >> + int ret = 0; >> + struct kfd_process_device *pdd; >> + char stats_dir_filename[MAX_SYSFS_FILENAME_LEN]; >> + >> + if (!p) >> + return -EINVAL; >> + >> + if (!p->kobj) >> + return -EFAULT; >> + >> + /* >> + * Create sysfs files for each GPU: >> + * - proc/<pid>/stats_<gpuid>/ >> + * - proc/<pid>/stats_<gpuid>/evicted_ms >> + */ >> + list_for_each_entry(pdd, &p->per_device_data, per_device_list) { >> + struct kobject *kobj_stats; >> + >> + >> + snprintf(stats_dir_filename, MAX_SYSFS_FILENAME_LEN, >> + "stats_%u", pdd->dev->id); >> + kobj_stats = kfd_alloc_struct(kobj_stats); >> + if (!kobj_stats) { >> + kfree(kobj_stats); > If allocation failed, there is nothing to free. > > Regards, > Felix > > >> + return -ENOMEM; >> + } >> + >> + ret = kobject_init_and_add(kobj_stats, >> + &procfs_stats_type, >> + p->kobj, >> + stats_dir_filename); >> + >> + if (ret) { >> + pr_warn("Creating KFD proc/stats_%s folder failed", >> + stats_dir_filename); >> + kobject_put(kobj_stats); >> + goto err; >> + } >> + >> + pdd->kobj_stats = kobj_stats; >> + pdd->attr_evict.name = "evicted_ms"; >> + pdd->attr_evict.mode = KFD_SYSFS_FILE_MODE; >> + sysfs_attr_init(&pdd->attr_evict); >> + ret = sysfs_create_file(kobj_stats, &pdd->attr_evict); >> + if (ret) >> + pr_warn("Creating eviction stats for gpuid %d failed", >> + (int)pdd->dev->id); >> + } >> +err: >> + return ret; >> +} >> + >> static int kfd_procfs_add_sysfs_files(struct kfd_process *p) { >> int ret = 0; >> @@ -660,6 +747,11 @@ struct kfd_process *kfd_create_process(struct file *filep) >> if (!process->kobj_queues) >> pr_warn("Creating KFD proc/queues folder failed"); >> >> + ret = kfd_procfs_add_sysfs_stats(process); >> + if (ret) >> + pr_warn("Creating sysfs stats dir for pid %d failed", >> + (int)process->lead_thread->pid); >> + >> ret = kfd_procfs_add_sysfs_files(process); >> if (ret) >> pr_warn("Creating sysfs usage file for pid %d failed", @@ -816,6 >> +908,10 @@ static void kfd_process_wq_release(struct work_struct *work) >> list_for_each_entry(pdd, &p->per_device_data, per_device_list) { >> sysfs_remove_file(p->kobj, &pdd->attr_vram); >> sysfs_remove_file(p->kobj, &pdd->attr_sdma); >> + sysfs_remove_file(p->kobj, &pdd->attr_evict); >> + kobject_del(pdd->kobj_stats); >> + kobject_put(pdd->kobj_stats); >> + pdd->kobj_stats = NULL; >> } >> >> kobject_del(p->kobj); >> @@ -1125,6 +1221,7 @@ struct kfd_process_device *kfd_create_process_device_data(struct kfd_dev *dev, >> pdd->runtime_inuse = false; >> pdd->vram_usage = 0; >> pdd->sdma_past_activity_counter = 0; >> + atomic64_set(&pdd->evict_duration_counter, 0); >> list_add(&pdd->per_device_list, &p->per_device_data); >> >> /* Init idr used for memory handle translation */ _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-09-17 14:32 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-09-16 18:42 [PATCH v3 2/3] drm/amdkfd: Add process eviction counters to sysfs Philip Cox 2020-09-16 22:46 ` Felix Kuehling 2020-09-17 13:11 ` Cox, Philip 2020-09-17 14:32 ` Felix Kuehling
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox