>>> With that in mind, I think your change kind of makes sense. I think the
>>> cleanest solution would be to just ignore the ctl_stack_size in the
>>> q_data and always get it from the topo_dev->node_props. You could even
>>> remove from the q_data completely. It's not needed. Just make sure you
>>> bump KFD_CRIU_PRIV_VERSION so that restoring an old checkpoint on a new
>>> driver would fail gracefully.
>>>
>>> Then you could also remove all those per-GPU generation
>>> get_checkpoint_info functions.
>>>
>> I think that cleanup makes sense. Is there any concern about bumping
>> the CRIU priv version and creating this incompatibility with old dump files
>> when we don't have to? (We could for instance just leave that field as
>> padding)
>
>Yeah, I wouldn't consider this an ABI breakage since the private
>checkpoint data is opaque to user mode. I think preserving checkpoint
>viability across driver versions is not a requirement. The version check
>is there to protect against an unlikely scenario. That said, we could
>leave it as padding to avoid the version bump. But that kind of clutter
>can accumulate over time. We'd want to remember to clean it up when we
>need to bump the version for some other reason in the future.
>
I was thrown off by a difference between staging and unified branches;
we are currently already setting qp->ctl_stack_size to always be the
topo dev value, and using the priv data ctl_stack_size to represent something
different
/* Per-XCC control stack size from topology, as create-queue requires */
if (qp.type == KFD_QUEUE_TYPE_COMPUTE && pdd->dev->kfd->cwsr_enabled) {
topo_dev = kfd_topology_device_by_id(pdd->dev->id);
if (!topo_dev) {
ret = -EINVAL;
goto exit;
}
qp.ctl_stack_size = topo_dev->node_props.ctl_stack_size;
}
if (q_data->ctl_stack_size > (u64)qp.ctl_stack_size *
NUM_XCC(pdd->dev->xcc_mask)) {
ret = -EINVAL;
goto exit;
}
*priv_data_offset += sizeof(*q_data);
q_extra_data_size = (uint64_t)q_data->ctl_stack_size + q_data->mqd_size;
Given that, it's probably best not to change set_queue_properties_from_criu
at all in this patch
________________________________________
From: Kuehling, Felix <[email protected]>
Sent: Monday, November 2, 2026 3:31 PM
To: Francis, David; [email protected]
Cc: Koenig, Christian
Subject: Re: [PATCH v2 1/3] drm/amdkfd: Acquire buffers in CRIU queue restore
(again)
On 2026-11-02 15:17, Francis, David wrote:
>>>> I'm curious what this case is needed for. In what circumstances would a
>>>> CRIU checkpoint contain 0 ctl_stack_size? And is that's the case,
>>>> wouldn't that break restore anyway?
>>>>
>>> A lot of mqd managers don't set mqd->get_checkpoint_info,
>>> so the ctl_stack_size ends up as 0.
>>>
>>> kfd_queue_acquire_buffers requires the ctl_stack_size to be equal to
>>> topo_dev->node_props.ctl_stack_size anyways, so I think it
>>> makes sense to default to that value if the input is 0.
>> I guess get_checkpoint_info is not implemented for GPUs that don't
>> allocate the control stack in kernel mode. Either way, the control stack
>> ends up being saved or copied into the user mode context save buffer, so
>> the control stack contents are saved regardless of the size reported here.
>>
>> With that in mind, I think your change kind of makes sense. I think the
>> cleanest solution would be to just ignore the ctl_stack_size in the
>> q_data and always get it from the topo_dev->node_props. You could even
>> remove from the q_data completely. It's not needed. Just make sure you
>> bump KFD_CRIU_PRIV_VERSION so that restoring an old checkpoint on a new
>> driver would fail gracefully.
>>
>> Then you could also remove all those per-GPU generation
>> get_checkpoint_info functions.
>>
> I think that cleanup makes sense. Is there any concern about bumping
> the CRIU priv version and creating this incompatibility with old dump files
> when we don't have to? (We could for instance just leave that field as
> padding)
Yeah, I wouldn't consider this an ABI breakage since the private
checkpoint data is opaque to user mode. I think preserving checkpoint
viability across driver versions is not a requirement. The version check
is there to protect against an unlikely scenario. That said, we could
leave it as padding to avoid the version bump. But that kind of clutter
can accumulate over time. We'd want to remember to clean it up when we
need to bump the version for some other reason in the future.
Regards,
Felix
>
> David Francis
>
> ________________________________________
> From: Kuehling, Felix <[email protected]>
> Sent: Monday, November 2, 2026 3:09 PM
> To: Francis, David; [email protected]
> Cc: Koenig, Christian
> Subject: Re: [PATCH v2 1/3] drm/amdkfd: Acquire buffers in CRIU queue restore
> (again)
>
> On 2026-11-02 09:39, Francis, David wrote:
>>>> +
>>>> + if (q_data->type == KFD_QUEUE_TYPE_COMPUTE) {
>>>> + if (q_data->ctl_stack_size)
>>>> + qp->ctl_stack_size = q_data->ctl_stack_size /
>>>> num_xcc;
>>>> + else if (topo_dev)
>>>> + qp->ctl_stack_size =
>>>> topo_dev->node_props.ctl_stack_size;
>>> I'm curious what this case is needed for. In what circumstances would a
>>> CRIU checkpoint contain 0 ctl_stack_size? And is that's the case,
>>> wouldn't that break restore anyway?
>>>
>> A lot of mqd managers don't set mqd->get_checkpoint_info,
>> so the ctl_stack_size ends up as 0.
>>
>> kfd_queue_acquire_buffers requires the ctl_stack_size to be equal to
>> topo_dev->node_props.ctl_stack_size anyways, so I think it
>> makes sense to default to that value if the input is 0.
> I guess get_checkpoint_info is not implemented for GPUs that don't
> allocate the control stack in kernel mode. Either way, the control stack
> ends up being saved or copied into the user mode context save buffer, so
> the control stack contents are saved regardless of the size reported here.
>
> With that in mind, I think your change kind of makes sense. I think the
> cleanest solution would be to just ignore the ctl_stack_size in the
> q_data and always get it from the topo_dev->node_props. You could even
> remove from the q_data completely. It's not needed. Just make sure you
> bump KFD_CRIU_PRIV_VERSION so that restoring an old checkpoint on a new
> driver would fail gracefully.
>
> Then you could also remove all those per-GPU generation
> get_checkpoint_info functions.
>
> Regards,
> Felix
>
>
>> David Francis
>>
>> ________________________________________
>> From: Kuehling, Felix <[email protected]>
>> Sent: Friday, October 30, 2026 5:15 PM
>> To: Francis, David; [email protected]
>> Cc: Koenig, Christian
>> Subject: Re: [PATCH v2 1/3] drm/amdkfd: Acquire buffers in CRIU queue
>> restore (again)
>>
>> On 2026-10-30 10:05, David Francis wrote:
>>> CRIU queue restore doesn't acquire the bo buffers in the same way
>>> normal queue creation does. In normal queue creation, the driver
>>> gets an additional reference on the queue's underlying buffers
>>> that lasts until the queue is destroyed; this prevents the
>>> queue's buffers from being removed from underneath it.
>>>
>>> Acquire them, but don't get the svm bo. SVM isn't yet fully
>>> restored by this point in CRIU and so there's no svm range to get.
>>> This will be addressed in the next patch.
>>>
>>> Signed-off-by: David Francis <[email protected]>
>>> ---
>>> drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 2 +-
>>> drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 2 +-
>>> .../amd/amdkfd/kfd_process_queue_manager.c | 34 +++++++++++++++++--
>>> drivers/gpu/drm/amd/amdkfd/kfd_queue.c | 8 ++++-
>>> 4 files changed, 41 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> index 6c710e4a5220c..d8233337b8d5e 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> @@ -433,7 +433,7 @@ static int kfd_ioctl_create_queue(struct file *filep,
>>> struct kfd_process *p,
>>> goto err_bind_process;
>>> }
>>>
>>> - err = kfd_queue_acquire_buffers(pdd, &q_properties);
>>> + err = kfd_queue_acquire_buffers(pdd, &q_properties, false);
>>> if (err) {
>>> pr_debug("failed to acquire user queue buffers\n");
>>> goto err_acquire_queue_buf;
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>> b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>> index f0ce5fc4a138f..eb4d35a42e917 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>> @@ -1381,7 +1381,7 @@ void print_queue(struct queue *q);
>>> int kfd_queue_buffer_get(struct amdgpu_vm *vm, void __user *addr,
>>> struct amdgpu_bo **pbo,
>>> u64 expected_size);
>>> void kfd_queue_buffer_put(struct amdgpu_bo **bo);
>>> -int kfd_queue_acquire_buffers(struct kfd_process_device *pdd, struct
>>> queue_properties *properties);
>>> +int kfd_queue_acquire_buffers(struct kfd_process_device *pdd, struct
>>> queue_properties *properties, bool is_criu);
>>> int kfd_queue_release_buffers(struct kfd_process_device *pdd, struct
>>> queue_properties *properties);
>>> void kfd_queue_unref_bo_va(struct amdgpu_vm *vm, struct amdgpu_bo **bo);
>>> int kfd_queue_unref_bo_vas(struct kfd_process_device *pdd,
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
>>> b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
>>> index 4c4a3a8587f46..509529af2dc3c 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
>>> @@ -985,8 +985,14 @@ int kfd_criu_checkpoint_queues(struct kfd_process *p,
>>> }
>>>
>>> static void set_queue_properties_from_criu(struct queue_properties *qp,
>>> - struct kfd_criu_queue_priv_data
>>> *q_data)
>>> + struct kfd_criu_queue_priv_data
>>> *q_data,
>>> + struct kfd_process_device *pdd)
>>> {
>>> + struct kfd_topology_device *topo_dev;
>>> + uint32_t num_xcc = NUM_XCC(pdd->dev->xcc_mask);
>>> +
>>> + topo_dev = kfd_topology_device_by_id(pdd->dev->id);
>>> +
>>> qp->is_interop = false;
>>> qp->queue_percent = q_data->q_percent;
>>> qp->priority = q_data->priority;
>>> @@ -998,6 +1004,16 @@ static void set_queue_properties_from_criu(struct
>>> queue_properties *qp,
>>> qp->eop_ring_buffer_size = q_data->eop_ring_buffer_size;
>>> qp->ctx_save_restore_area_address =
>>> q_data->ctx_save_restore_area_address;
>>> qp->ctx_save_restore_area_size = q_data->ctx_save_restore_area_size;
>>> +
>>> + if (q_data->type == KFD_QUEUE_TYPE_COMPUTE) {
>>> + if (q_data->ctl_stack_size)
>>> + qp->ctl_stack_size = q_data->ctl_stack_size / num_xcc;
>>> + else if (topo_dev)
>>> + qp->ctl_stack_size =
>>> topo_dev->node_props.ctl_stack_size;
>> I'm curious what this case is needed for. In what circumstances would a
>> CRIU checkpoint contain 0 ctl_stack_size? And is that's the case,
>> wouldn't that break restore anyway?
>>
>> Other than that, this patch looks good to me.
>>
>> Regards,
>> Felix
>>
>>
>>> + } else {
>>> + qp->ctl_stack_size = q_data->ctl_stack_size;
>>> + }
>>> +
>>> qp->type = q_data->type;
>>> qp->format = q_data->format;
>>> }
>>> @@ -1047,7 +1063,13 @@ int kfd_criu_restore_queue(struct kfd_process *p,
>>> }
>>>
>>> memset(&qp, 0, sizeof(qp));
>>> - set_queue_properties_from_criu(&qp, q_data);
>>> + set_queue_properties_from_criu(&qp, q_data, pdd);
>>> +
>>> + if (q_data->type == KFD_QUEUE_TYPE_COMPUTE && !qp.ctl_stack_size) {
>>> + pr_err("Failed to determine ctl_stack_size for compute
>>> queue\n");
>>> + ret = -EINVAL;
>>> + goto exit;
>>> + }
>>>
>>> /* Per-XCC control stack size from topology, as create-queue
>>> requires */
>>> if (qp.type == KFD_QUEUE_TYPE_COMPUTE &&
>>> pdd->dev->kfd->cwsr_enabled) {
>>> @@ -1095,9 +1117,17 @@ int kfd_criu_restore_queue(struct kfd_process *p,
>>>
>>> print_queue_properties(&qp);
>>>
>>> + ret = kfd_queue_acquire_buffers(pdd, &qp, true);
>>> + if (ret) {
>>> + pr_err("Failed to acquire user queue buffers err:%d\n", ret);
>>> + goto exit;
>>> + }
>>> +
>>> ret = pqm_create_queue(&p->pqm, pdd->dev, &qp, &queue_id, q_data,
>>> mqd, ctl_stack, NULL);
>>> if (ret) {
>>> pr_err("Failed to create new queue err:%d\n", ret);
>>> + kfd_queue_unref_bo_vas(pdd, &qp);
>>> + kfd_queue_release_buffers(pdd, &qp);
>>> goto exit;
>>> }
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_queue.c
>>> b/drivers/gpu/drm/amd/amdkfd/kfd_queue.c
>>> index 6010a5d7a7eb9..18d24ddb455eb 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_queue.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_queue.c
>>> @@ -239,7 +239,7 @@ static void kfd_queue_put_bos(struct queue_properties
>>> *properties)
>>> kfd_queue_buffer_put(&properties->cwsr_bo);
>>> }
>>>
>>> -int kfd_queue_acquire_buffers(struct kfd_process_device *pdd, struct
>>> queue_properties *properties)
>>> +int kfd_queue_acquire_buffers(struct kfd_process_device *pdd, struct
>>> queue_properties *properties, bool is_criu)
>>> {
>>> struct kfd_topology_device *topo_dev;
>>> u64 expected_queue_size;
>>> @@ -334,6 +334,12 @@ int kfd_queue_acquire_buffers(struct
>>> kfd_process_device *pdd, struct queue_prope
>>>
>>> amdgpu_bo_unreserve(vm->root.bo);
>>>
>>> + /* CRIU svm buffers are restored at a later stage than queues.
>>> They'll be in
>>> + * before the queue is actually used, so no risk here
>>> + */
>>> + if (is_criu)
>>> + return 0;
>>> +
>>> err = kfd_queue_buffer_svm_get(pdd,
>>> properties->ctx_save_restore_area_address,
>>> total_cwsr_size);
>>> if (err)
