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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Am 2020-09-11 um 4:10 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.
>
> Signed-off-by: Philip Cox <Philip.Cox@xxxxxxx>
Some more comments inline.

Amber, could you review this one as well?


> ---
>  .../drm/amd/amdkfd/kfd_device_queue_manager.c |   9 ++
>  drivers/gpu/drm/amd/amdkfd/kfd_priv.h         |  15 ++-
>  drivers/gpu/drm/amd/amdkfd/kfd_process.c      | 109 ++++++++++++++++++
>  3 files changed, 131 insertions(+), 2 deletions(-)
>
> 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..468c69d22117 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,10 +692,20 @@ 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_restore_timestamp;

last_restore_timestamp is not used.


> +	unsigned long last_evict_timestamp;
> +	atomic64_t evict_duration_counter;
> +	struct attribute attr_evict;
>  };
>  
>  #define qpd_to_pdd(x) container_of(x, struct kfd_process_device, qpd)
>  
> +struct kobj_stats_list {
> +	struct list_head stats_list;
> +	struct kobject *kobj;
> +};

Why does this need to be a separate struct that gets dynamically
allocated with kzalloc? The kobj pointer should be in the pdd structure,
then you wouldn't need a list at all.


>  /* Process data */
>  struct kfd_process {
>  	/*
> @@ -766,13 +776,14 @@ struct kfd_process {
>  	/* seqno of the last scheduled eviction */
>  	unsigned int last_eviction_seqno;
>  	/* Approx. the last timestamp (in jiffies) when the process was
> -	 * restored after an eviction
> +	 * restored or evicted.

This comment should not be changed in this commit. There is no
functional change here.


>  	 */
>  	unsigned long last_restore_timestamp;
>  
>  	/* Kobj for our procfs */
>  	struct kobject *kobj;
>  	struct kobject *kobj_queues;
> +	struct kobj_stats_list stats;

This is probably not needed. See below.


>  	struct attribute attr_pasid;
>  };
>  
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index 1e15aa7d8ae8..d786ba80d656 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,67 @@ 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;
> +
> +	INIT_LIST_HEAD(&p->stats.stats_list);
> +	/*
> +	 * 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 kobj_stats_list *kobj_stats;
> +
> +		kobj_stats = kzalloc(sizeof(*kobj_stats),
> +				GFP_KERNEL);
> +		if (!kobj_stats)
> +			return -ENOMEM;
> +
> +		snprintf(stats_dir_filename, MAX_SYSFS_FILENAME_LEN,
> +				"stats_%u", pdd->dev->id);
> +		kobj_stats->kobj = kfd_alloc_struct(kobj_stats->kobj);
> +		if (!kobj_stats->kobj) {
> +			kfree(kobj_stats);
> +			return -ENOMEM;
> +		}
> +
> +		ret = kobject_init_and_add(kobj_stats->kobj,
> +						&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->kobj);
> +			kfree(kobj_stats);
> +			goto err;
> +		}
> +
> +		list_add(&kobj_stats->stats_list,
> +				&p->stats.stats_list);

I think you're only using this list to find all the stats for destroying
them in kfd_process_wq_release. But there should be exactly one per pdd.
So you don't need another list. Just add the kobj-stats into the pdd
structure.


> +		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->kobj, &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 +754,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",
> @@ -806,6 +905,7 @@ static void kfd_process_wq_release(struct work_struct *work)
>  					     release_work);
>  	struct kfd_process_device *pdd;
>  
> +	struct kobj_stats_list *stats;
>  	/* Remove the procfs files */
>  	if (p->kobj) {
>  		sysfs_remove_file(p->kobj, &p->attr_pasid);
> @@ -818,6 +918,14 @@ static void kfd_process_wq_release(struct work_struct *work)
>  			sysfs_remove_file(p->kobj, &pdd->attr_sdma);
>  		}
>  
> +		list_for_each_entry(stats,
> +				&p->stats.stats_list,
> +				stats_list) {
> +			sysfs_remove_file(p->kobj, &pdd->attr_evict);

This should be in the per-pdd loop above. The pdd-pointer is not valid here.


> +			kobject_del(stats->kobj);
> +			kobject_put(stats->kobj);

If you move the stats-kobj into the pdd structure, you could move this
into the per-pdd loop and you wouldn't need the stats list any more.

Regards,
  Felix


> +		}
> +
>  		kobject_del(p->kobj);
>  		kobject_put(p->kobj);
>  		p->kobj = NULL;
> @@ -1125,6 +1233,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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux