On 12/18/25 16:04, Tvrtko Ursulin wrote:
> IDR is deprecated so let's replace it with xarray.
>
> Conversion is mostly 1:1 apart from AMDGPU_BO_LIST_OP_UPDATE which was
> implemented with idr_replace, and has now been replaced with a sequence of
> xa_load and xa_cmpxchg. Should userspace attempt multi-threaded update
> operations on the same handle it could theoretically hit a new -ENOENT
> path. But I believe this is purely theoretical and still safe.
>
> Signed-off-by: Tvrtko Ursulin <[email protected]>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 4 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c | 72 ++++++++++-----------
> drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h | 2 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 11 ++--
> 4 files changed, 41 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 9f9774f58ce1..0a5b802bd3c3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -50,6 +50,7 @@
> #include <linux/hashtable.h>
> #include <linux/dma-fence.h>
> #include <linux/pci.h>
> +#include <linux/xarray.h>
>
> #include <drm/ttm/ttm_bo.h>
> #include <drm/ttm/ttm_placement.h>
> @@ -500,8 +501,7 @@ struct amdgpu_fpriv {
> struct amdgpu_bo_va *prt_va;
> struct amdgpu_bo_va *csa_va;
> struct amdgpu_bo_va *seq64_va;
> - struct mutex bo_list_lock;
> - struct idr bo_list_handles;
> + struct xarray bo_list_handles;
> struct amdgpu_ctx_mgr ctx_mgr;
> struct amdgpu_userq_mgr userq_mgr;
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
> index 66fb37b64388..628d32fd2fae 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
> @@ -149,31 +149,20 @@ int amdgpu_bo_list_create(struct amdgpu_device *adev,
> struct drm_file *filp,
>
> }
>
> -static void amdgpu_bo_list_destroy(struct amdgpu_fpriv *fpriv, int id)
> -{
> - struct amdgpu_bo_list *list;
> -
> - mutex_lock(&fpriv->bo_list_lock);
> - list = idr_remove(&fpriv->bo_list_handles, id);
> - mutex_unlock(&fpriv->bo_list_lock);
> - if (list)
> - kref_put(&list->refcount, amdgpu_bo_list_free);
> -}
> -
> -int amdgpu_bo_list_get(struct amdgpu_fpriv *fpriv, int id,
> +int amdgpu_bo_list_get(struct amdgpu_fpriv *fpriv, u32 id,
> struct amdgpu_bo_list **result)
> {
> - rcu_read_lock();
> - *result = idr_find(&fpriv->bo_list_handles, id);
> + struct amdgpu_bo_list *list;
>
> - if (*result && kref_get_unless_zero(&(*result)->refcount)) {
> - rcu_read_unlock();
> - return 0;
> - }
> + xa_lock(&fpriv->bo_list_handles);
> + list = xa_load(&fpriv->bo_list_handles, id);
> + if (list && !kref_get_unless_zero(&list->refcount))
> + list = NULL;
Since you have this protected by xa_lock() now you don't need the
kref_get_unless_zero() any more and can also drop the RCU handling in
amdgpu_bo_list_free().
Alternatively stop using xa_lock() and use the RCU read side lock instead, but
I clearly prefer this approach here.
Apart from that looks good to me.
Regards,
Christian.
> + xa_unlock(&fpriv->bo_list_handles);
>
> - rcu_read_unlock();
> - *result = NULL;
> - return -ENOENT;
> + *result = list;
> +
> + return list ? 0 : -ENOENT;
> }
>
> void amdgpu_bo_list_put(struct amdgpu_bo_list *list)
> @@ -219,12 +208,12 @@ int amdgpu_bo_create_list_entry_array(struct
> drm_amdgpu_bo_list_in *in,
> int amdgpu_bo_list_ioctl(struct drm_device *dev, void *data,
> struct drm_file *filp)
> {
> - struct amdgpu_device *adev = drm_to_adev(dev);
> struct amdgpu_fpriv *fpriv = filp->driver_priv;
> - union drm_amdgpu_bo_list *args = data;
> - uint32_t handle = args->in.list_handle;
> + struct amdgpu_device *adev = drm_to_adev(dev);
> struct drm_amdgpu_bo_list_entry *info = NULL;
> - struct amdgpu_bo_list *list, *old;
> + struct amdgpu_bo_list *list, *prev, *curr;
> + uint32_t handle = args->in.list_handle;
> + union drm_amdgpu_bo_list *args = data;
> int r;
>
> r = amdgpu_bo_create_list_entry_array(&args->in, &info);
> @@ -238,19 +227,19 @@ int amdgpu_bo_list_ioctl(struct drm_device *dev, void
> *data,
> if (r)
> goto error_free;
>
> - mutex_lock(&fpriv->bo_list_lock);
> - r = idr_alloc(&fpriv->bo_list_handles, list, 1, 0, GFP_KERNEL);
> - mutex_unlock(&fpriv->bo_list_lock);
> - if (r < 0) {
> + r = xa_alloc(&fpriv->bo_list_handles, &handle, list,
> + xa_limit_32b, GFP_KERNEL);
> + if (r)
> goto error_put_list;
> - }
>
> - handle = r;
> break;
>
> case AMDGPU_BO_LIST_OP_DESTROY:
> - amdgpu_bo_list_destroy(fpriv, handle);
> + list = xa_erase(&fpriv->bo_list_handles, handle);
> + if (list)
> + amdgpu_bo_list_put(list);
> handle = 0;
> +
> break;
>
> case AMDGPU_BO_LIST_OP_UPDATE:
> @@ -259,16 +248,23 @@ int amdgpu_bo_list_ioctl(struct drm_device *dev, void
> *data,
> if (r)
> goto error_free;
>
> - mutex_lock(&fpriv->bo_list_lock);
> - old = idr_replace(&fpriv->bo_list_handles, list, handle);
> - mutex_unlock(&fpriv->bo_list_lock);
> + curr = xa_load(&fpriv->bo_list_handles, handle);
> + if (!curr) {
> + r = -ENOENT;
> + goto error_put_list;
> + }
>
> - if (IS_ERR(old)) {
> - r = PTR_ERR(old);
> + prev = xa_cmpxchg(&fpriv->bo_list_handles, handle, curr, list,
> + GFP_KERNEL);
> + if (xa_is_err(prev)) {
> + r = xa_err(prev);
> + goto error_put_list;
> + } else if (prev != curr) {
> + r = -ENOENT;
> goto error_put_list;
> }
>
> - amdgpu_bo_list_put(old);
> + amdgpu_bo_list_put(curr);
> break;
>
> default:
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
> index 2b5e7c46a39d..0989f1090c63 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
> @@ -58,7 +58,7 @@ struct amdgpu_bo_list {
> struct amdgpu_bo_list_entry entries[] __counted_by(num_entries);
> };
>
> -int amdgpu_bo_list_get(struct amdgpu_fpriv *fpriv, int id,
> +int amdgpu_bo_list_get(struct amdgpu_fpriv *fpriv, u32 id,
> struct amdgpu_bo_list **result);
> void amdgpu_bo_list_put(struct amdgpu_bo_list *list);
> int amdgpu_bo_create_list_entry_array(struct drm_amdgpu_bo_list_in *in,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 6ee77f431d56..88f104041157 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -1445,8 +1445,7 @@ int amdgpu_driver_open_kms(struct drm_device *dev,
> struct drm_file *file_priv)
> if (r)
> goto error_vm;
>
> - mutex_init(&fpriv->bo_list_lock);
> - idr_init_base(&fpriv->bo_list_handles, 1);
> + xa_init_flags(&fpriv->bo_list_handles, XA_FLAGS_ALLOC1);
>
> r = amdgpu_userq_mgr_init(&fpriv->userq_mgr, file_priv, adev);
> if (r)
> @@ -1492,8 +1491,8 @@ void amdgpu_driver_postclose_kms(struct drm_device *dev,
> struct amdgpu_fpriv *fpriv = file_priv->driver_priv;
> struct amdgpu_bo_list *list;
> struct amdgpu_bo *pd;
> + unsigned long handle;
> u32 pasid;
> - int handle;
>
> if (!fpriv)
> return;
> @@ -1529,11 +1528,9 @@ void amdgpu_driver_postclose_kms(struct drm_device
> *dev,
> amdgpu_pasid_free_delayed(pd->tbo.base.resv, pasid);
> amdgpu_bo_unref(&pd);
>
> - idr_for_each_entry(&fpriv->bo_list_handles, list, handle)
> + xa_for_each(&fpriv->bo_list_handles, handle, list)
> amdgpu_bo_list_put(list);
> -
> - idr_destroy(&fpriv->bo_list_handles);
> - mutex_destroy(&fpriv->bo_list_lock);
> + xa_destroy(&fpriv->bo_list_handles);
>
> kfree(fpriv);
> file_priv->driver_priv = NULL;
