Re: [PATCH v5 4/8] drm/xe: Handle xe_user creation and removal

[Matthew Brost <matthew.brost@intel.com>]

On Mon, Oct 06, 2025 at 02:20:25PM +0000, Aakash Deep Sarkar wrote:
> We want our xe user structure to be created when a new
> user id opens the xe device node and to be destroyed
> when the final xe file with this uid is closed. In other
> words the xe_user structure for a uid should remain in
> scope as long as any process with this uid has an open
> xe file descriptor.
> 
> To implement this we maintain an xarray of xe user
> structures inside our xe device instance. Whenever a new
> xe file is created via an open call, we check if the
> calling process' uid is already present in our xarray.
> If so, we increment the refcount for the associated
> xe user and add this xe file to the list of xe files
> belonging to this xe user. Otherwise, we allocate a
> new xe user structure for this uid and initialize its
> file list with this xe file.
> 
> Whenever an xe file is destroyed, we decrement the
> refcount of the associated xe user. When the last
> xe file in the xe user's file list is destroyed,
> the xe user refcount should drop to zero and the
> xe user should be cleaned up. During the cleanup path
> we remove the xarray entry for this xe user in our
> xe device and free up its memory.
> 
> Signed-off-by: Aakash Deep Sarkar <aakash.deep.sarkar@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_device.c       | 21 ++++++++
>  drivers/gpu/drm/xe/xe_device_types.h | 16 ++++++
>  drivers/gpu/drm/xe/xe_user.c         | 77 +++++++++++++++++++++++++++-
>  drivers/gpu/drm/xe/xe_user.h         | 11 +++-
>  4 files changed, 123 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 386940323630..5a084fd39876 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -65,6 +65,7 @@
>  #include "xe_tile.h"
>  #include "xe_ttm_stolen_mgr.h"
>  #include "xe_ttm_sys_mgr.h"
> +#include "xe_user.h"
>  #include "xe_vm.h"
>  #include "xe_vm_madvise.h"
>  #include "xe_vram.h"
> @@ -82,7 +83,9 @@ static int xe_file_open(struct drm_device *dev, struct drm_file *file)
>  	struct xe_drm_client *client;
>  	struct xe_file *xef;
>  	int ret = -ENOMEM;
> +	int uid = -EINVAL;
>  	struct task_struct *task = NULL;
> +	const struct cred *cred = NULL;
>  
>  	xef = kzalloc(sizeof(*xef), GFP_KERNEL);
>  	if (!xef)
> @@ -107,8 +110,16 @@ static int xe_file_open(struct drm_device *dev, struct drm_file *file)
>  	file->driver_priv = xef;
>  	kref_init(&xef->refcount);
>  
> +	INIT_LIST_HEAD(&xef->user_link);
> +
>  	task = get_pid_task(rcu_access_pointer(file->pid), PIDTYPE_PID);
>  	if (task) {
> +		cred = get_task_cred(task);
> +		if (cred) {
> +			uid = (unsigned int) cred->euid.val;
> +			xe_user_init(xe, xef, uid);
> +			put_cred(cred);
> +		}
>  		xef->process_name = kstrdup(task->comm, GFP_KERNEL);
>  		xef->pid = task->pid;
>  		put_task_struct(task);
> @@ -128,6 +139,12 @@ static void xe_file_destroy(struct kref *ref)
>  
>  	xe_drm_client_put(xef->client);
>  	kfree(xef->process_name);
> +
> +	mutex_lock(&xef->user->filelist_lock);
> +	list_del(&xef->user_link);
> +	mutex_unlock(&xef->user->filelist_lock);
> +
> +	xe_user_put(xef->user);
>  	kfree(xef);
>  }
>  
> @@ -467,6 +484,10 @@ struct xe_device *xe_device_create(struct pci_dev *pdev,
>  
>  	xa_init_flags(&xe->usm.asid_to_vm, XA_FLAGS_ALLOC);
>  
> +	xa_init_flags(&xe->work_period.users, XA_FLAGS_ALLOC1);
> +
> +	mutex_init(&xe->work_period.lock);
> +
>  	if (IS_ENABLED(CONFIG_DRM_XE_DEBUG)) {
>  		/* Trigger a large asid and an early asid wrap. */
>  		u32 asid;
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index 54a612787289..4d4e9a63b3fd 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -613,6 +613,16 @@ struct xe_device {
>  	atomic_t g2g_test_count;
>  #endif
>  
> +	/**
> +	 * @xe_work_period: Support for GPU work period tracepoint
> +	 */
> +	struct xe_work_period {
> +		/** @users: list of users that have opened this xe device */
> +		struct xarray users;
> +		/** @lock: lock protecting this structure */
> +		struct mutex lock;
> +	} work_period;
> +
>  	/* private: */
>  
>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> @@ -684,6 +694,12 @@ struct xe_file {
>  	/** @active_duration_ns: total run time in ns for this xe file */
>  	u64 active_duration_ns;
>  
> +	/** @user: pointer to struct xe_user associated with this xe file */
> +	struct xe_user *user;
> +
> +	/** @user_link: link into xe_user::filelist */
> +	struct list_head user_link;
> +
>  	/** @client: drm client */
>  	struct xe_drm_client *client;
>  
> diff --git a/drivers/gpu/drm/xe/xe_user.c b/drivers/gpu/drm/xe/xe_user.c
> index f35e18776300..cb3de75aa497 100644
> --- a/drivers/gpu/drm/xe/xe_user.c
> +++ b/drivers/gpu/drm/xe/xe_user.c
> @@ -3,6 +3,8 @@
>   * Copyright © 2025 Intel Corporation
>   */
>  
> +#include <drm/drm_drv.h>
> +
>  #include "xe_user.h"
>  
>  
> @@ -60,7 +62,7 @@
>   *
>   * Return: pointer to user struct or NULL if can't allocate
>   */
> -struct xe_user *xe_user_alloc(void)
> +static struct xe_user *xe_user_alloc(void)
>  {
>  	struct xe_user *user;
>  
> @@ -71,6 +73,7 @@ struct xe_user *xe_user_alloc(void)
>  	kref_init(&user->refcount);
>  	mutex_init(&user->filelist_lock);
>  	INIT_LIST_HEAD(&user->filelist);
> +	INIT_WORK(&user->work, work_period_worker);
>  	return user;
>  }
>  
> @@ -84,6 +87,78 @@ void __xe_user_free(struct kref *kref)
>  {
>  	struct xe_user *user =
>  		container_of(kref, struct xe_user, refcount);
> +	struct xe_device *xe = user->xe;
> +	void *lookup;
> +
> +	mutex_lock(&xe->work_period.lock);

You don't need to take work_period.lock look here. Xa have there own
locking. work_period.lock should protect the reference upon lookup,
that's it.

> +	lookup = xa_erase(&xe->work_period.users, user->id);
> +	xe_assert(xe, lookup == user);
> +	mutex_unlock(&xe->work_period.lock);
>  
> +	drm_dev_put(&user->xe->drm);
>  	kfree(user);
>  }
> +
> +static struct xe_user *xe_user_lookup(struct xe_device *xe, u32 uid)
> +{
> +	struct xe_user *user = NULL;
> +	unsigned long i;
> +
> +	mutex_lock(&xe->work_period.lock);

guard(mutex)(&xe->work_period.lock) will work better here.

> +	xa_for_each(&xe->work_period.users, i, user) {
> +		if (user->uid == uid) {
> +			xe_user_get(user);
> +			mutex_unlock(&xe->work_period.lock);
> +			return user;
> +		}
> +	}
> +	mutex_unlock(&xe->work_period.lock);
> +
> +	return NULL;
> +}
> +
> +int xe_user_init(struct xe_device *xe, struct xe_file *xef, unsigned int uid)
> +{
> +	struct xe_user *user = NULL;
> +	int ret;
> +	u32 idx;
> +	/*
> +	 * Check if the calling process/uid has already been registered
> +	 * with the xe device during a previous open call. If so then
> +	 * take a reference to this xe user and add this xe file to the
> +	 * filelist belonging to this xe user
> +	 */
> +	user = xe_user_lookup(xe, uid);
> +	if (!user) {
> +		/*
> +		 * We couldn't find an existing xe user for the calling process.
> +		 * Allocate a new struct xe_user and register it with this xe
> +		 * device
> +		 */
> +		user = xe_user_alloc();
> +		if (!user)
> +			return -ENOMEM;
> +
> +
> +		user->uid = uid;
> +		user->last_timestamp_ns = ktime_get_raw_ns();
> +		user->xe = xe;
> +
> +		mutex_lock(&xe->work_period.lock);
> +		ret = xa_alloc(&xe->work_period.users, &idx, user, xa_limit_32b, GFP_KERNEL);

You don't lock here either.

Matt

> +		mutex_unlock(&xe->work_period.lock);
> +
> +		if (ret < 0)
> +			return ret;
> +
> +		user->id = idx;
> +		drm_dev_get(&xe->drm);
> +	}
> +
> +	mutex_lock(&user->filelist_lock);
> +	list_add(&xef->user_link, &user->filelist);
> +	mutex_unlock(&user->filelist_lock);
> +	xef->user = user;
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/xe/xe_user.h b/drivers/gpu/drm/xe/xe_user.h
> index 9628cc628a37..341200c55509 100644
> --- a/drivers/gpu/drm/xe/xe_user.h
> +++ b/drivers/gpu/drm/xe/xe_user.h
> @@ -6,6 +6,9 @@
>  #ifndef _XE_USER_H_
>  #define _XE_USER_H_
>  
> +#include "xe_device.h"
> +
> +
>  /**
>   * struct xe_user - xe user structure
>   *
> @@ -40,6 +43,11 @@ struct xe_user {
>  	 */
>  	struct work_struct work;
>  
> +	/**
> +	 * @id: index of this user into the xe device::users xarray
> +	 */
> +	u32 id;
> +
>  	/**
>  	 * @uid: UID of this xe_user
>  	 */
> @@ -58,7 +66,8 @@ struct xe_user {
>  	u64 last_timestamp_ns;
>  };
>  
> -struct xe_user *xe_user_alloc(void);
> +int xe_user_init(struct xe_device *xe, struct xe_file *xef, unsigned int uid);
> +
>  
>  static inline struct xe_user *
>  xe_user_get(struct xe_user *user)
> -- 
> 2.49.0
>