Re: [PATCH 2/5] nvmet: cq: prepare for completion queue sharing

[Damien Le Moal <dlemoal@kernel.org>]

On 4/24/25 2:13 PM, Wilfred Mallawa wrote:
> From: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> 
> For the PCI transport, the NVMe specification allows submission queues to
> share completion queues, however, this is not supported in the current
> NVMe target implementation. This is a preparatory patch to allow for
> completion queue (CQ) sharing between different submission queues (SQ).
> 
> To support queue sharing, reference counting completion queues is
> required. This patch adds the refcount_t field ref to struct nvmet_cq
> coupled with respective nvmet_cq_init(), nvmet_cq_get(), nvmet_cq_put(),
> nvmet_cq_is_deletable() and nvmet_cq_destroy() functions.
> 
> A CQ reference count is initialized with nvmet_cq_init() when a CQ is
> created. Using nvmet_cq_get(), a reference to a CQ is taken when an SQ is
> created that uses the respective CQ. Similarly. when an SQ is destroyed,
> the reference count to the respective CQ from the SQ being destroyed is
> decremented with nvmet_cq_put(). The last reference to a CQ is dropped
> on a CQ deletion using nvmet_cq_put(), which invokes nvmet_cq_destroy()
> to fully cleanup after the CQ. The helper function nvmet_cq_in_use() is
> used to determine if any SQs are still using the CQ pending deletion.
> In which case, the CQ must not be deleted. This should protect scenarios
> where a bad host may attempt to delete a CQ without first having deleted
> SQ(s) using that CQ.
> 
> Additionally, this patch adds an array of struct nvmet_cq to the
> nvmet_ctrl structure. This allows for the controller to keep track of CQs
> as they are created and destroyed, similar to the current tracking done
> for SQs. The memory for this array is freed when the controller is freed.
> A struct nvmet_ctrl reference is also added to the nvmet_cq structure to
> allow for CQs to be removed from the controller whilst keeping the new API
> similar to the existing API for SQs.

Looks good to me, modulo one nit below.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

> diff --git a/drivers/nvme/target/pci-epf.c b/drivers/nvme/target/pci-epf.c
> index 7fab7f3d79b7..7dda4156d86c 100644
> --- a/drivers/nvme/target/pci-epf.c
> +++ b/drivers/nvme/target/pci-epf.c
> @@ -1346,6 +1346,7 @@ static u16 nvmet_pci_epf_delete_cq(struct nvmet_ctrl *tctrl, u16 cqid)
>  	nvmet_pci_epf_drain_queue(cq);
>  	nvmet_pci_epf_remove_irq_vector(ctrl, cq->vector);
>  	nvmet_pci_epf_mem_unmap(ctrl->nvme_epf, &cq->pci_map);
> +	tctrl->cqs[cqid] = NULL;

I do not think we need for this hunk in this patch as we are not yet using the
cqs array, and patch 4 removes this line to change it with a call to
nvmet_cq_put().

-- 
Damien Le Moal
Western Digital Research