On 12/19/25 14:42, 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.
>
> Also, since we have removed the RCU protection around the handle lookup we
> also removed the RCU freeing of the list.
>
> Signed-off-by: Tvrtko Ursulin <[email protected]>
> ---
> v2:
> * Dropped RCU freeing of the list.
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 4 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c | 81 +++++++++------------
> drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h | 3 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 11 +--
> 4 files changed, 42 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 80dba6276aa8..ec3cbe70012a 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>
> @@ -499,8 +500,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 fbac929f711c..59def86cdc04 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
> @@ -37,14 +37,6 @@
> #define AMDGPU_BO_LIST_MAX_PRIORITY 32u
> #define AMDGPU_BO_LIST_NUM_BUCKETS (AMDGPU_BO_LIST_MAX_PRIORITY + 1)
>
> -static void amdgpu_bo_list_free_rcu(struct rcu_head *rcu)
> -{
> - struct amdgpu_bo_list *list = container_of(rcu, struct amdgpu_bo_list,
> - rhead);
> -
> - kvfree(list);
> -}
> -
> static void amdgpu_bo_list_free(struct kref *ref)
> {
> struct amdgpu_bo_list *list = container_of(ref, struct amdgpu_bo_list,
> @@ -53,7 +45,8 @@ static void amdgpu_bo_list_free(struct kref *ref)
>
> amdgpu_bo_list_for_each_entry(e, list)
> amdgpu_bo_unref(&e->bo);
> - call_rcu(&list->rhead, amdgpu_bo_list_free_rcu);
> +
> + kvfree(list);
> }
>
> static int amdgpu_bo_list_entry_cmp(const void *_a, const void *_b)
> @@ -146,31 +139,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(&list->refcount);
> + 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)
> @@ -215,12 +197,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;
> + struct amdgpu_device *adev = drm_to_adev(dev);
> + struct drm_amdgpu_bo_list_entry *info = NULL;
> + struct amdgpu_bo_list *list, *prev, *curr;
> union drm_amdgpu_bo_list *args = data;
> uint32_t handle = args->in.list_handle;
> - struct drm_amdgpu_bo_list_entry *info = NULL;
> - struct amdgpu_bo_list *list, *old;
> int r;
>
> r = amdgpu_bo_create_list_entry_array(&args->in, &info);
> @@ -234,19 +216,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);
It's usually good practice to make *_put() functions take NULL as argument.
Apart from that looks good to me.
Regards,
Christian.
> handle = 0;
> +
> break;
>
> case AMDGPU_BO_LIST_OP_UPDATE:
> @@ -255,16 +237,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 1acf53f8b2f9..cf127bc66f53 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
> @@ -43,7 +43,6 @@ struct amdgpu_bo_list_entry {
> };
>
> struct amdgpu_bo_list {
> - struct rcu_head rhead;
> struct kref refcount;
> struct amdgpu_bo *gds_obj;
> struct amdgpu_bo *gws_obj;
> @@ -54,7 +53,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;
