Re: [PATCH V5 08/18] amdkfd: identify a secondary kfd process by its id

[Felix Kuehling <felix.kuehling@amd.com>]

On 2025-10-17 04:42, Zhu Lingshan wrote:
> This commit introduces a new id field for
> struct kfd process, which helps identify
> a kfd process among multiple contexts that
> all belong to a single user space program.
>
> The sysfs entry of a secondary kfd process
> is placed under the sysfs entry folder of
> its primary kfd process.
>
> The naming format of the sysfs entry of a secondary
> kfd process is "context_%u" where %u is the process id.
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_priv.h    |  6 ++
>   drivers/gpu/drm/amd/amdkfd/kfd_process.c | 78 +++++++++++++++++++++++-
>   2 files changed, 81 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index 919510f18249..9de658119cd9 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@ -1021,10 +1021,16 @@ struct kfd_process {
>   
>   	/*kfd context id */
>   	u16 context_id;
> +
> +	/* The primary kfd_process allocating IDs for its secondary kfd_process, 0 for primary kfd_process */
> +	struct ida id_table;
> +
>   };
>   
>   #define KFD_PROCESS_TABLE_SIZE 8 /* bits: 256 entries */
>   #define KFD_CONTEXT_ID_PRIMARY	0xFFFF
> +#define KFD_CONTEXT_ID_MIN 0
> +#define KFD_CONTEXT_ID_WIDTH 16
>   
>   extern DECLARE_HASHTABLE(kfd_processes_table, KFD_PROCESS_TABLE_SIZE);
>   extern struct srcu_struct kfd_processes_srcu;
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index 995d27be06e3..157145c94314 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -827,6 +827,7 @@ static void kfd_process_device_destroy_ib_mem(struct kfd_process_device *pdd)
>   
>   int kfd_create_process_sysfs(struct kfd_process *process)
>   {
> +	struct kfd_process *primary_process;
>   	int ret;
>   
>   	if (process->kobj) {
> @@ -839,9 +840,22 @@ int kfd_create_process_sysfs(struct kfd_process *process)
>   		pr_warn("Creating procfs kobject failed");
>   		return -ENOMEM;
>   	}
> -	ret = kobject_init_and_add(process->kobj, &procfs_type,
> -				   procfs.kobj, "%d",
> -				   (int)process->lead_thread->pid);
> +
> +	if (process->context_id == KFD_CONTEXT_ID_PRIMARY)
> +		ret = kobject_init_and_add(process->kobj, &procfs_type,
> +					   procfs.kobj, "%d",
> +					   (int)process->lead_thread->pid);
> +	else {
> +		primary_process = kfd_lookup_process_by_mm(process->lead_thread->mm);
> +		if (!primary_process)
> +			return -ESRCH;
> +
> +		ret = kobject_init_and_add(process->kobj, &procfs_type,
> +					   primary_process->kobj, "context_%u",
> +					   process->context_id);
> +		kfd_unref_process(primary_process);
> +	}
> +
>   	if (ret) {
>   		pr_warn("Creating procfs pid directory failed");
>   		kobject_put(process->kobj);
> @@ -863,6 +877,50 @@ int kfd_create_process_sysfs(struct kfd_process *process)
>   	return 0;
>   }
>   
> +static int kfd_process_alloc_id(struct kfd_process *process)
> +{
> +	int ret;
> +	struct kfd_process *primary_process;
> +
> +	/* already assign 0xFFFF when create */
> +	if (process->context_id == KFD_CONTEXT_ID_PRIMARY)
> +		return 0;
> +
> +	primary_process = kfd_lookup_process_by_mm(process->lead_thread->mm);
> +	if (!primary_process)
> +		return -ESRCH;
> +
> +	/* id range: KFD_CONTEXT_ID_MIN to 0xFFFE */
> +	ret = ida_alloc_range(&primary_process->id_table, KFD_CONTEXT_ID_MIN,
> +	     (1 << KFD_CONTEXT_ID_WIDTH) - 2, GFP_KERNEL);

This would be safer and more obvious if you just set the upper limit as 
KFD_CONTEXT_ID_PRIMARY - 1. Then you don't need KFD_CONTEXT_ID_WIDTH at all.

Regards,
   Felix


> +	if (ret < 0)
> +		goto out;
> +
> +	process->context_id = ret;
> +	ret = 0;
> +
> +out:
> +	kfd_unref_process(primary_process);
> +
> +	return ret;
> +}
> +
> +static void kfd_process_free_id(struct kfd_process *process)
> +{
> +	struct kfd_process *primary_process;
> +
> +	if (process->context_id != KFD_CONTEXT_ID_PRIMARY)
> +		return;
> +
> +	primary_process = kfd_lookup_process_by_mm(process->lead_thread->mm);
> +	if (!primary_process)
> +		return;
> +
> +	ida_free(&primary_process->id_table, process->context_id);
> +
> +	kfd_unref_process(primary_process);
> +}
> +
>   struct kfd_process *kfd_create_process(struct task_struct *thread)
>   {
>   	struct kfd_process *process;
> @@ -1195,6 +1253,11 @@ static void kfd_process_wq_release(struct work_struct *work)
>   	if (ef)
>   		dma_fence_signal(ef);
>   
> +	if (p->context_id != KFD_CONTEXT_ID_PRIMARY)
> +		kfd_process_free_id(p);
> +	else
> +		ida_destroy(&p->id_table);
> +
>   	kfd_process_remove_sysfs(p);
>   	kfd_debugfs_remove_process(p);
>   
> @@ -1601,6 +1664,13 @@ static struct kfd_process *create_process(const struct task_struct *thread, bool
>   			goto err_register_notifier;
>   		}
>   		BUG_ON(mn != &process->mmu_notifier);
> +		ida_init(&process->id_table);
> +	}
> +
> +	err = kfd_process_alloc_id(process);
> +	if (err) {
> +		pr_err("Creating kfd process: failed to alloc an id\n");
> +		goto err_alloc_id;
>   	}
>   
>   	kfd_unref_process(process);
> @@ -1610,6 +1680,8 @@ static struct kfd_process *create_process(const struct task_struct *thread, bool
>   
>   	return process;
>   
> +err_alloc_id:
> +	kfd_process_free_id(process);
>   err_register_notifier:
>   	hash_del_rcu(&process->kfd_processes);
>   	svm_range_list_fini(process);