Re: [PATCH V5 18/18] amdkfd: introduce new ioctl AMDKFD_IOC_CREATE_PROCESS

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

[-- Attachment #1: Type: text/plain, Size: 6407 bytes --]

On 2025-10-17 04:42, Zhu Lingshan wrote:
> This commit implemetns a new ioctl AMDKFD_IOC_CREATE_PROCESS
> that creates a new secondary kfd_progress on the FD.
>
> To keep backward compatibility, userspace programs need to invoke
> this ioctl explicitly on a FD to create a secondary
> kfd_process which replacing its primary kfd_process.
>
> This commit bumps ioctl minor version.
>
> Signed-off-by: Zhu Lingshan<lingshan.zhu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 41 ++++++++++++++++++++++++
>   drivers/gpu/drm/amd/amdkfd/kfd_priv.h    |  1 +
>   drivers/gpu/drm/amd/amdkfd/kfd_process.c |  3 +-
>   include/uapi/linux/kfd_ioctl.h           |  8 +++--
>   4 files changed, 49 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index 7c02e8473622..8fe58526bc3a 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -3140,6 +3140,44 @@ static int kfd_ioctl_set_debug_trap(struct file *filep, struct kfd_process *p, v
>   	return r;
>   }
>   
> +/* userspace programs need to invoke this ioctl explicitly on a FD to
> + * create a secondary kfd_process which replacing its primary kfd_process
> + */
> +static int kfd_ioctl_create_process(struct file *filep, struct kfd_process *p, void *data)
> +{
> +	struct kfd_process *process;
> +	int ret;
> +
> +	if (!filep->private_data || !p)
> +		return -EINVAL;
> +
> +	if (p != filep->private_data)
> +		return -EINVAL;
> +
> +	/* Each FD owns only one kfd_process */
> +	if (p->context_id != KFD_CONTEXT_ID_PRIMARY)
> +		return -EINVAL;
> +
> +	mutex_lock(&kfd_processes_mutex);
> +	process = create_process(current, false);
> +	mutex_unlock(&kfd_processes_mutex);

There could be race conditions between multiple threads calling this 
ioctl on the same primary fd concurrently. We could use the 
kfd_processes_mutex to protect against this. You'd need to do some of 
the above checks and the assignment to filep->private_data under the 
lock to be safe, to allow only one thread to successfully create the 
secondary context. I think this would work:

	/* Atomically create the secondary context and assign it to filep->private_data */
	mutex_lock(&kfd_processes_mutex);
	if (p != filep->private_data) {
		/* Another thread already created a secondary context */
		mutex_unlock(&kfd_processes_mutex);
		return -EINVAL;
	}
	secondary = create_process(current, false);
	if (!IS_ERR(process))
		filep->private_data = secondary;
	mutex_unlock(&kfd_processes_mutex);

	if (IS_ERR(secondary))
		return PTR_ERR(secondary);

	/* Release the process reference that was held by filep->private_data */
	kfd_unref_process(p);

	...

Regards,
   Felix


> +
> +	if (IS_ERR(process))
> +		return PTR_ERR(process);
> +
> +	/* Each open() increases kref of the primary kfd_process,
> +	 * so we need to reduce it here before we create a new secondary process replacing it
> +	 */
> +	kfd_unref_process(p);
> +
> +	filep->private_data = process;
> +	ret = kfd_create_process_sysfs(process);
> +	if (ret)
> +		pr_warn("Failed to create sysfs entry for the kfd_process");
> +
> +	return 0;
> +}
> +
>   #define AMDKFD_IOCTL_DEF(ioctl, _func, _flags) \
>   	[_IOC_NR(ioctl)] = {.cmd = ioctl, .func = _func, .flags = _flags, \
>   			    .cmd_drv = 0, .name = #ioctl}
> @@ -3258,6 +3296,9 @@ static const struct amdkfd_ioctl_desc amdkfd_ioctls[] = {
>   
>   	AMDKFD_IOCTL_DEF(AMDKFD_IOC_DBG_TRAP,
>   			kfd_ioctl_set_debug_trap, 0),
> +
> +	AMDKFD_IOCTL_DEF(AMDKFD_IOC_CREATE_PROCESS,
> +			kfd_ioctl_create_process, 0),
>   };
>   
>   #define AMDKFD_CORE_IOCTL_COUNT	ARRAY_SIZE(amdkfd_ioctls)
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index 4237c859050d..578f9f217e16 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@ -1055,6 +1055,7 @@ struct amdkfd_ioctl_desc {
>   };
>   bool kfd_dev_is_large_bar(struct kfd_node *dev);
>   
> +struct kfd_process *create_process(const struct task_struct *thread, bool primary);
>   int kfd_process_create_wq(void);
>   void kfd_process_destroy_wq(void);
>   void kfd_cleanup_processes(void);
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index 629a706e2a13..bbb72a77bed5 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -68,7 +68,6 @@ static struct workqueue_struct *kfd_restore_wq;
>   static struct kfd_process *find_process(const struct task_struct *thread,
>   					bool ref);
>   static void kfd_process_ref_release(struct kref *ref);
> -static struct kfd_process *create_process(const struct task_struct *thread, bool primary);
>   
>   static void evict_process_worker(struct work_struct *work);
>   static void restore_process_worker(struct work_struct *work);
> @@ -1578,7 +1577,7 @@ void kfd_process_set_trap_debug_flag(struct qcm_process_device *qpd,
>    * On return the kfd_process is fully operational and will be freed when the
>    * mm is released
>    */
> -static struct kfd_process *create_process(const struct task_struct *thread, bool primary)
> +struct kfd_process *create_process(const struct task_struct *thread, bool primary)
>   {
>   	struct kfd_process *process;
>   	struct mmu_notifier *mn;
> diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h
> index 5d1727a6d040..84aa24c02715 100644
> --- a/include/uapi/linux/kfd_ioctl.h
> +++ b/include/uapi/linux/kfd_ioctl.h
> @@ -44,9 +44,10 @@
>    * - 1.16 - Add contiguous VRAM allocation flag
>    * - 1.17 - Add SDMA queue creation with target SDMA engine ID
>    * - 1.18 - Rename pad in set_memory_policy_args to misc_process_flag
> + * - 1.19 - Add a new ioctl to craete secondary kfd processes
>    */
>   #define KFD_IOCTL_MAJOR_VERSION 1
> -#define KFD_IOCTL_MINOR_VERSION 18
> +#define KFD_IOCTL_MINOR_VERSION 19
>   
>   struct kfd_ioctl_get_version_args {
>   	__u32 major_version;	/* from KFD */
> @@ -1671,7 +1672,10 @@ struct kfd_ioctl_dbg_trap_args {
>   #define AMDKFD_IOC_DBG_TRAP			\
>   		AMDKFD_IOWR(0x26, struct kfd_ioctl_dbg_trap_args)
>   
> +#define AMDKFD_IOC_CREATE_PROCESS		\
> +		AMDKFD_IO(0x27)
> +
>   #define AMDKFD_COMMAND_START		0x01
> -#define AMDKFD_COMMAND_END		0x27
> +#define AMDKFD_COMMAND_END		0x28
>   
>   #endif

[-- Attachment #2: Type: text/html, Size: 6813 bytes --]