[PATCH v3 2/3] drm/amdkfd: Add process eviction counters to sysfs

[PATCH v3 2/3] drm/amdkfd: Add process eviction counters to sysfs

[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;
+	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

Re: [PATCH v3 2/3] drm/amdkfd: Add process eviction counters to sysfs

[Felix Kuehling]

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

RE: [PATCH v3 2/3] drm/amdkfd: Add process eviction counters to sysfs

[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.

-----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

Re: [PATCH v3 2/3] drm/amdkfd: Add process eviction counters to sysfs

[Felix Kuehling]

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