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@xxxxxxx>
Regards,
Felix
>
> -----Original Message-----
> From: Kuehling, Felix <Felix.Kuehling@xxxxxxx>
> Sent: Wednesday, September 16, 2020 6:46 PM
> To: Cox, Philip <Philip.Cox@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Tye, Tony <Tony.Tye@xxxxxxx>; Morichetti, Laurent <Laurent.Morichetti@xxxxxxx>; Kim, Jonathan <Jonathan.Kim@xxxxxxx>; Errabolu, Ramesh <Ramesh.Errabolu@xxxxxxx>
> 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@xxxxxxx>
>> ---
>> .../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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
