> > Looks complicated. Have you looked at how I've done the rename with no > external lock?
I may have entirely missed the xas_ interface. That does make things much easier. You have to admit the poison queue thing is clever though. Will revise and simplify in next version ________________________________________ From: Tvrtko Ursulin <[email protected]> Sent: Monday, September 7, 2026 9:31 AM To: Francis, David; [email protected] Subject: Re: [PATCH v4 2/2] drm/amdgpu: Add CHANGE_ID option for USERQ ioctl On 27/08/2026 19:59, David Francis wrote: > Add a new option to the USERQ ioctl, which is called with > the queue_id of an existing user queue and an unused queue_id, > and changes that queue's id to the new value. > > Calling with an invalid new handle will fail. Calling with new_handle > = handle will succeed if that queue exists but not do anything. > > A poison queue object is inserted at the old handle during the > rekey to prevent concurrent FREE from destroying the queue or the > old id being reused mid-operation. Looks complicated. Have you looked at how I've done the rename with no external lock? https://lore.kernel.org/amd-gfx/[email protected]/ I'll have a read through your approach but I can't say I am a fan. > Performing this operation on a queue with signals or waits > outstanding is fine, as those hold not the queue_id but a > direct reference to the queue object. > > v3: Poison queue and misc fixes > > Signed-off-by: David Francis <[email protected]> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 60 +++++++++++++++++++++++ > include/uapi/drm/amdgpu_drm.h | 17 +++++-- > 2 files changed, 74 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c > index 32d1787aa7e2..699838042bda 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c > @@ -35,6 +35,8 @@ > #include "amdgpu_userq_fence.h" > #include "amdgpu_trace.h" > > +struct amdgpu_usermode_queue poison_queue; > + > u32 amdgpu_userq_get_supported_ip_mask(struct amdgpu_device *adev) > { > int i; > @@ -852,6 +854,11 @@ static int amdgpu_userq_input_args_validate(struct > drm_device *dev, > break; > case AMDGPU_USERQ_OP_LIST: > break; > + case AMDGPU_USERQ_OP_CHANGE_ID: > + if (!args->change_in.new_queue_id || > + args->change_in.new_queue_id > AMDGPU_MAX_USERQ_COUNT) > + return -EINVAL; > + break; > default: > return -EINVAL; > } > @@ -1010,6 +1017,51 @@ amdgpu_userq_list(struct drm_file *filp, union > drm_amdgpu_userq *args) > return ret; > } > > +static int amdgpu_userq_change_id(struct drm_file *filp, union > drm_amdgpu_userq *args) > +{ > + struct amdgpu_fpriv *fpriv = filp->driver_priv; > + struct amdgpu_userq_mgr *uq_mgr = &fpriv->userq_mgr; > + struct amdgpu_usermode_queue *queue; > + int ret = 0; > + > + if (args->change_in.new_queue_id == args->change_in.queue_id) { > + if (xa_load(&uq_mgr->userq_xa, args->change_in.queue_id)) > + return 0; > + return -ENOENT; Why is is important to allow users to query for existence of an id like this ie. why wouldn't be be EINVAL if new == old? > + } > + > + /* Poison the old handle so it doesn't get freed or reused. */ > + queue = xa_store(&uq_mgr->userq_xa, args->change_in.queue_id, > &poison_queue, GFP_KERNEL); What if userspace is silly and looks up this handle from a racing thread? It gets the poison queue and things explode? > + if (!queue) { > + xa_erase(&uq_mgr->userq_xa, args->change_in.queue_id); > + return -ENOENT; > + } > + if (queue == &poison_queue) > + return -EINVAL; Why is this EINVAL? Userspace races with itself I get it, but I think it shows the weakness of the poison entry multi-stage approach. > + > + ret = xa_insert(&uq_mgr->userq_xa, args->change_in.new_queue_id, queue, > GFP_KERNEL); > + if (ret == -EBUSY) { > + queue = xa_store(&uq_mgr->userq_xa, args->change_in.queue_id, > queue, GFP_KERNEL); > + if (queue != &poison_queue) { > + drm_err_once(adev_to_drm(uq_mgr->adev), > + "Expected poison queue to remain untouched > during userqueue change id"); Do you expect drm_err to be reachable by silly userspace or just unexpected internal error? > + return -EINVAL; > + } > + } > + if (ret == -ENOMEM) > + return -ENOMEM; Is there another possibily from xa_insert other than EBUSY and ENOMEM? > + > + /* remove the poison */ > + queue = xa_erase(&uq_mgr->userq_xa, args->change_in.queue_id); > + if (queue != &poison_queue) { > + drm_err_once(adev_to_drm(uq_mgr->adev), > + "Expected poison queue to remain untouched during > userqueue change id"); Same question as the previous drm_err_once. Sorry I don't like this at all. I would much rather you try to punch a hole in my approach or confirm it works fine. I adapted that from some existing driver.. can't remember which now after more than a month. But the approach sounds sane to me - do the rename atomically under the lock using GFP_NOWAIT first and if that fails drop the lock to pre-allocate space and retry. Regards, Tvrtko > + return -EINVAL; > + } > + > + return 0; > +} > + > bool amdgpu_userq_enabled(struct drm_device *dev) > { > struct amdgpu_device *adev = drm_to_adev(dev); > @@ -1046,6 +1098,11 @@ int amdgpu_userq_ioctl(struct drm_device *dev, void > *data, > > case AMDGPU_USERQ_OP_FREE: > xa_lock(&fpriv->userq_mgr.userq_xa); > + queue = xa_load(&fpriv->userq_mgr.userq_xa, args->in.queue_id); > + if (queue == &poison_queue) { > + xa_unlock(&fpriv->userq_mgr.userq_xa); > + return -EINVAL; > + } > queue = __xa_erase(&fpriv->userq_mgr.userq_xa, > args->in.queue_id); > xa_unlock(&fpriv->userq_mgr.userq_xa); > if (!queue) > @@ -1056,6 +1113,9 @@ int amdgpu_userq_ioctl(struct drm_device *dev, void > *data, > case AMDGPU_USERQ_OP_LIST: > r = amdgpu_userq_list(filp, args); > break; > + case AMDGPU_USERQ_OP_CHANGE_ID: > + r = amdgpu_userq_change_id(filp, args); > + break; > default: > drm_dbg_driver(dev, "Invalid user queue op specified: %d\n", > args->in.op); > return -EINVAL; > diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h > index 92738d630eea..37d0efd1eda4 100644 > --- a/include/uapi/drm/amdgpu_drm.h > +++ b/include/uapi/drm/amdgpu_drm.h > @@ -330,9 +330,10 @@ union drm_amdgpu_ctx { > }; > > /* user queue IOCTL operations */ > -#define AMDGPU_USERQ_OP_CREATE 1 > -#define AMDGPU_USERQ_OP_FREE 2 > -#define AMDGPU_USERQ_OP_LIST 3 > +#define AMDGPU_USERQ_OP_CREATE 1 > +#define AMDGPU_USERQ_OP_FREE 2 > +#define AMDGPU_USERQ_OP_LIST 3 > +#define AMDGPU_USERQ_OP_CHANGE_ID 4 > > /* queue priority levels */ > /* low < normal low < normal high < high */ > @@ -464,10 +465,20 @@ struct drm_amdgpu_userq_list_in_out { > __u64 entries; > }; > > +struct drm_amdgpu_userq_change_id_in { > + /** AMDGPU_USERQ_OP_CHANGE_ID */ > + __u32 op; > + /** Queue id of some queue */ > + __u32 queue_id; > + /** Queue id to change that queue to */ > + __u32 new_queue_id; > +}; > + > union drm_amdgpu_userq { > struct drm_amdgpu_userq_in in; > struct drm_amdgpu_userq_out out; > struct drm_amdgpu_userq_list_in_out list_in_out; > + struct drm_amdgpu_userq_change_id_in change_in; > }; > > /* GFX V11 IP specific MQD parameters */
